Revert "fix: removes opcache_hook again (#2564)" - #2621
Conversation
This reverts 3f43199 and adds a regression test. Without the hook, an opcache restart is performed while worker threads are still executing code that lives in opcache shared memory, and the process dies with SIGSEGV. opcache defers a restart until accel_is_inactive(), which probes for a conflicting lock using fcntl F_GETLK on its own lock file. POSIX fcntl locks belong to the process and never conflict with locks that process already holds, so the probe reports "inactive" no matter how many threads of this process are inside a request. That check works for prefork SAPIs, where the accessors are other processes, and kill_all_lockers() makes the assumption explicit by signalling the locking pid. It does nothing for a single-process threaded server. Once the restart is carried out, accel_interned_strings_restore_state() memsets every string interned past the startup watermark and zend_shared_alloc_restore_state() rewinds each segment position, so the op_array a worker is executing is handed back out to the next compile. Rebooting threads when the restart is scheduled is what gets them out of shared memory before the rewind happens. The hook fires at most once per restart cycle, because zend_accel_schedule_restart() returns early while restart_pending is already set. Measured on dunglas/frankenphp:builder-php8.5 (PHP 8.5.9 ZTS): with the hook the new test passes and logs a thread reboot, without it the test binary dies with "signal: segmentation fault (core dumped)".
Addresses the concern that motivated php#2564: a runtime that invalidates heavily could restart opcache over and over, and rebooting the threads every time would turn that into a reboot loop. Reboots are not throttled. opcache rewinds its shared memory whether or not the threads are ready, so declining a reboot brings back the segfault the hook exists to prevent. Skipping is not a safe trade. What the loop actually means is that the compiled code does not fit in opcache.memory_consumption, so opcache can never settle: it fills, it overflows, it restarts, it fills again. Today that same configuration already thrashes, just silently. This makes it visible with an error naming the two ini settings that fix it, logged once per window rather than on every restart. go_schedule_opcache_reset() now knows whether the restart came from opcache or from a userland opcache_reset(). Only the automatic path is counted, so an operator calling opcache_reset() in a loop does not trigger the message. Reboot coalescing already exists: rebootAllThreads() returns early when a reboot is in flight. frankenphp_opcache_restart_hook() moves behind the same guard as the assignment in php_main(), which is now its only caller. Without that, builds for PHP 8.2 and 8.3 fail with -Werror=unused-function.
29b2bc8 to
ed1898f
Compare
|
the issue here is that we weren't able to reproduce the issues where the server grinded to a halt with this logic in place. if we want to bring it back (and I think the better place to handle this is in php-src itself, given how complicated it turns out to be here), we better find a way to reproduce and verify this fixes it |
|
Doesn't the attached test provide the reproducer we need? |
I don't think this would be the issue because after a restart it would've cleared back to zero, not permanently stayed slow after. |
|
Ah you mean that reproducer about the slowness. I don't know about it, but maybe the added log line will help figure out if restart is involved. What I know is that without the restart hook, threads segfault at will because their opcache pointers become stale. |
|
I think the issue is that a single request might do so much opcache invalidation that you can end up in a restart loop. Apps that do this are likely not very performant or well written to begin with, but a crash is probably less frustrating than endless hanging. I agree with @hernderkes that the fix probably lies somewhere in php-src, there are some PRs like php/php-src#22281 trying to do this, but it's probably multiple race conditions. |
3 participants
Reverts #2564, with a regression test and a fix for the concern that motivated the removal.
Without the hook, an opcache restart takes the whole process down with SIGSEGV.
Why the hook matters
opcache defers a restart until
accel_is_inactive(), which probes for a conflicting lock withfcntl F_GETLKon its own lock file. POSIX fcntl locks belong to the process and never conflict with locks that process already holds, so the probe reports "inactive" no matter how many threads are inside a request:The last line is the control: the mechanism does work across processes, which is what it was designed for, and
kill_all_lockers()makes that assumption explicit by signalling the locking pid. It does nothing for a single-process threaded server.So the restart gets carried out at the next
ZEND_RINIT_FUNCTION(zend_accelerator)on any thread, while workers are still running.accel_interned_strings_restore_state()memsets every string interned past the startup watermark and rewindstop, andzend_shared_alloc_restore_state()rewinds each segment position. The op_array a worker is executing is handed back out to the next compile.Workers are the worst case. They never reach
accel_post_deactivate, because the worker request cycle only runsMODULES_TO_RELOADand notzend_activate_modules(), so they hold shared memory references for their whole life instead of for one request.Reproducer
On
dunglas/frankenphp:latest(PHP 8.5.9 ZTS, FrankenPHP 1.12.7): a worker holding an interned literal and aconstarray, plus a second route on regular threads that compiles and invalidates scripts to overflow the opcache hash.memory_consumption=8,max_accelerated_files=200max_wasted_percentage=50memory_consumption=128,max_accelerated_files=20000memory_consumption=128,max_accelerated_files=200Run C is the control: same workload and same file churn, the only difference being that nothing can overflow. opcache's own log carries
Restart Scheduled! Reason: hash overflowfollowed byRestarting!in exactly the runs that die.Run D is worth a look on its own. With a 128 MB pool and
max_wasted_percentage=50the gate inzend_accel_schedule_restart_if_necessary()should need 64 MB of waste, but the restart fired at 222 KB. The startup log showsopcache.memory_consumption cannot be changed when OPcache is already set uponce per thread, which suggestsZCG(accel_directives).memory_consumptionstays 0 on those threads and the gate divides by zero. That would mean restarts are far easier to hit under ZTS than the documented 5 % suggests. Happy to split that out if it is worth chasing separately.Commit 1: revert plus test
TestOpcacheRestartKeepsWorkerThreadsAlivesets a small opcache, starts a worker holding those references, compiles until a restart has actually been performed (the counters only move once it is carried out, not when it is scheduled), then asserts the worker still answers.Checked in both directions on
dunglas/frankenphp:builder-php8.5, same tree, only the hook differing:--- PASS (0.24s), and the log showsrebooting all PHP threadsthenthread reboot finished num_threads=48signal: segmentation fault (core dumped),FAILThe test skips when opcache is unavailable or when a restart cannot be forced. Note it does not fail cleanly without the hook, it takes the test binary down, which is the nature of the bug.
Commit 2: repeated restarts
The concern in #2564 is real: a runtime that invalidates heavily could restart opcache over and over, and rebooting the threads each time would turn that into a reboot loop.
The reboots are not throttled. opcache rewinds its shared memory whether or not the threads are ready for it, so declining a reboot brings the segfault straight back. Skipping is not a safe trade.
A sustained loop means the compiled code does not fit in
opcache.memory_consumption, so opcache can never settle: it fills, overflows, restarts and fills again. That configuration already thrashes today, just silently. So the loop is now reported instead of hidden, with an error naming the two settings that fix it, logged once per window rather than on every restart.go_schedule_opcache_reset()now knows whether the restart came from opcache or from a userlandopcache_reset(). Only the automatic path is counted, so callingopcache_reset()in a loop does not produce the message. Reboot coalescing already exists,rebootAllThreads()returns early while a reboot is in flight.Test results
Full library suite passes with both commits applied:
go test ., 23.9 s, onbuilder-php8.5.Scope
The hook is
#if defined(ZTS) && PHP_VERSION_ID >= 80400, so 8.2 and 8.3 have no protection either way. The root cause is upstream opcache behaviour under ZTS rather than a FrankenPHP bug, and it is not specific to workers, but workers are where the exposure is permanent. I can take it upstream too if you prefer that venue.