-
Notifications
You must be signed in to change notification settings - Fork 545
Fixing OPcache issues with autoloading with FileReadTrapStreamWrapper #801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
It's not OK to return an empty string, because OPcache will remember it and won't read the actual file the next time the file is asked for. So, to make this work in all scenarios, we need to actually implement file operations so that the class can be loaded for OPcache correctly.
Since we allow autoloading to proceed as normal, loading one class might cause more classes to be loaded. We only want to capture the first one.
This one fails because it used the cache from the previous broken build https://github.com/phpstan/phpstan-src/runs/4317359673?check_suite_focus=true
seems like an issue with the CI ...
Will fix the other issues shortly.
I see this causes a bunch of issues with static reflection. Not sure what the best way to solve those is, short of creating two implementations (obviously not ideal).
I attempted to use opcache_reset() and opcache_invalidate() to get this issue to gtfo, but opcache just won't cooperate. reset has no effect, and invalidate() returns false because it doesn't understand realpath() on phar:// (https://bugs.php.net/bug.php?id=78316). This is a really infuriating issue.
I think the only bulletproof solution to this problem is to do ini_set("opcache.enable", "off") somewhere. Since static reflection may anyway be used when dynamic reflection is enabled, there's no reliable way to know whether the actual code should be loaded or not, and if it's not loaded, this problem will occur because OPcache will grab the fake data.
Sadly, even just turning off OPcache temporarily can't be done, as opcache.enable can only be disabled at runtime. Once it's disabled, it can't be reenabled.
when using dynamic reflection (which is the default), any time static reflection comes into play, bad shit starts to happen because of FileReadTrapStreamWrapper. I attempted to fix these issues (phpstan/phpstan-src#801) and failed miserably. So, to save the hassle, it's time to just remove OPcache from the picture (which, unfortunately, also means that PHPStan will not benefit from JIT).
disabling opcache causes increased memory usage and lower performance
Yeah, I'm aware. It also means PHPStan can't benefit from JIT. But until this bug is resolved, it's the only option I have available.
Trouble is, I don't see any way to fix this problem. If opcache_invalidate() worked, we might be OK, but even Nikita was nervous to fix it due to unknowns. Until static reflection is forced for everyone, this will continue to be a problem.
The php-src issue seems fixable, let's try to fix it there first, wait for bugfix releases and disable opcache for the older/unmaitaned php versions.
I doubt it will get fixed upstream. This issue with opcache_invalidate() has been known since >2y and no activity on it due to low demand. I've made some suggestions and hopefully it gets fixed, but I'm not hopeful.
It looks like opcache_invalidate() does not do anything in the current request context (the script will not be recompiled, even when opcache_invalidate() succeeds). It only forces a recompile in subsequent requests.
It looks like the only ways to fix this problem are:
- Patch php-src to make opcache not cache scripts which are empty (weird special case, doubtful it would be accepted upstream).
- Disable opcache during PHPStan execution.
I should also point out that this can happen without OPcache when require_once is used. @ondrejmirtes this is a really broken hack :/
Not sure its related: there is another open PR to this file in #722
maybe thats something which works on the same problem your are trying to fix.
I didn‘t know, just remembered the other PR and figured maybe its helpful for you
No, that PR isn't related to this bug.
Thank you for this effort. Would you be able to report a bug to php-src? Or it isn't PHP's fault at all? Looks to me like opcache_invalidate ought to always work.
Even if opcache_invalidate() worked on phar://, it wouldn't work for this case anyway, since it only forces the file to be recompiled in the next request. I tested it with regular non-phar files also (the bug happens with require_once without opcache too: #801 (comment))
TL;DR: This is not PHP's fault. I can't find a right solution to this problem :(
97c2f21 to
a03a78f
Compare
Just dealt with this in the obvious way - let it actually autoload the file! e30f446
PHPStan should now work even with OPCache enabled, please test dev-master.
Great, thanks.
I suppose that fix should be fine in most cases since autoloaded classes usually won't directly execute code anyway.
One catch though - I had to revert to the old way of doing things when disableRuntimeReflectionProvider=true: c2d2c2f
In that case it should be fine anyway, since all classes are reflected statically. The issue only occurred when mixing static and dynamic reflection.
Maybe it's this obvious: ae53760
I tried that already, it doesn't work for this case.
It helped to solve my issues. If you comment it out in this PR, some tests fail (with OPCache enabled): #1265
The problems I had arose when using phars, since opcache_invalidate() doesn't like phar:// paths.
These issues were caused by #491, because it made stream_read() return an empty string.
OPcache then cached this empty string, leading to failures to load files from phars or file:// locations.
This PR changes FileReadTrapStreamWrapper to allow autoloading to proceed as normal, and only intervenes to grab the first loaded filename. For everything else, it passes back to the regular functions.