-
Notifications
You must be signed in to change notification settings - Fork 7.9k
opcache: fix revalidation in long running CLI scripts #18671
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
base: master
Are you sure you want to change the base?
Conversation
8d75fcd
to
702c189
Compare
702c189
to
e210c08
Compare
|
||
double revalidate_reference_time = 0.0; | ||
|
||
if (ZCG(cli_mode)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.php.net/manual/en/function.php-sapi-name.php - maybe there is a better way to detect cli /wo extra property.
Also the time getter can be probably deduplicated to something like microtime(true)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion.
We considered using php_sapi_name()
for detecting the environment at runtime, but opted to precompute this value once during initialization and store it in a cli_mode
flag for performance reasons.
The motivation is that validate_timestamp_and_record()
is invoked on every include or require, not just on actual cache invalidations. Using strcmp()
or a similar runtime check each time would introduce measurable overhead. By storing this as a precomputed boolean, we ensure this logic has minimal runtime cost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding microtime(true)
, I believe there may be some confusion here. The microtime()
function referenced is a userland PHP function and not directly available within the engine context.
The implementation I used is based on the internal php_time()
logic, with slight adjustments to return a double for better precision.
I also avoided using microtime()
as-is because it's not available consistently across all platforms.
If there’s a preferred helper or abstraction for obtaining a double-precision current time in the engine, I’m happy to switch to that but I wanted to avoid introducing inconsistencies across platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, it looks like this part of PR may be weak, and I'm happy for any suggestions to it. Maybe I can drop double precision logic and switch to php_time()
here.
It looks like opcache was initially designed with short-lived requests in mind, so when checking if the file should be re-validated, the code uses 'request time' instead of 'now', where request time is calculated once on the script startup. Which basically means that in CLI this check will never succeed.
This PR containing the patch to fix this issue covered with a test that passes in this branch but not in PHP-8.3.
The issue itself is older, but I rebased the patch to PHP-8.3 as mentioned in CONTRIBUTING doc.
Not sure if I must fill the issue separately, but will be happy to do if needed.