Fixing OPcache issues with autoloading with FileReadTrapStreamWrapper #801
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 Sadly, even just turning off OPcache temporarily can't be done, as |
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 |
|
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 |
|
It looks like It looks like the only ways to fix this problem are:
|
|
I should also point out that this can happen without OPcache when |
|
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. |
|
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 |
|
Even if TL;DR: This is not PHP's fault. I can't find a right solution to this problem :( |
|
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 |
|
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.