fix: throttle opcache_reset()-triggered thread pool reboots - #2570
fix: throttle opcache_reset()-triggered thread pool reboots#2570dunglas wants to merge 1 commit into
Conversation
opcache_reset() was rebooting the whole PHP thread pool on every single call, unthrottled. Some applications (e.g. WordPress plugins) call opcache_reset() on nearly every request; each reboot pauses request handling for the whole pool, which compounds into sustained, severe slowdowns and dropped connections under real traffic. Coalesce calls into at most one reboot per 5s cooldown instead. Fixes #2553.
|
I don't think this would help as at least one of the cases I was able to reproduce this with was a client stall during go_post_read |
|
Also not sure this would help, #2553 says rebooting periodically fixes the issue and opcache_reset basically does the same thing as a reboot currently |
| // reboot instead of pausing the whole pool on every call. | ||
| mainThread.opcacheResetMu.Lock() | ||
| now := time.Now() | ||
| if now.Sub(mainThread.lastOpcacheResetAt) < opcacheResetCooldown { |
There was a problem hiding this comment.
Since frankenphp_opcache_reset only calls the hook and never touches the opcache, a call landing inside the cooldown resets nothing at all yet still returns true, with no log line at any level. That makes this a drop rather than a coalesce: if the burst ends on a dropped call, the code that call wanted refreshed never is, and since the same hook is zend_accel_schedule_restart_hook a dropped OOM restart also leaves accelerator_enabled = false until threads happen to go idle.
| mainThread.opcacheResetMu.Unlock() | ||
| return | ||
| } | ||
| mainThread.lastOpcacheResetAt = now |
There was a problem hiding this comment.
lastOpcacheResetAt is stamped before the goroutine runs and the bool from rebootAllThreads is discarded, but that function returns false without rebooting when isRebooting is already set or the main thread isn't Ready. A reboot can outlast the 5s window since it waits up to rebootGracePeriod for straggling threads, so a call arriving after the cooldown can hit the CAS, do nothing, and still arm a fresh window; RestartWorkers() conversely reboots without stamping at all. We can only stamp when a reboot actually happened.
4 participants
Summary
go_schedule_opcache_reset()spawned a full PHP thread pool reboot on every singleopcache_reset()call, with no rate limiting. A reboot pauses request handling across the whole pool (drains every thread, force-kills stragglers after a grace period, recreates the main thread).Some applications call
opcache_reset()on nearly every request — the reporter of #2553 found several WordPress plugins doing exactly this across 19 sites on one host. Each call triggers its own reboot, and the reboots compound into sustained, severe slowdowns (write error,stream closed,client disconnectedin the logs) rather than a single blip.Fix
Coalesce
opcache_reset()-triggered reboots into at most one per 5s cooldown window. The first call in a burst still reboots immediately; calls arriving within the cooldown are skipped rather than each pausing the pool again. This only throttles the automaticopcache_reset()hook —RestartWorkers()(used by the watcher/hot-reload and any explicit caller) is untouched and always takes effect immediately.Tests
TestOpcacheResetIsThrottled: 10 sequentialopcache_reset()calls 50ms apart (well within the cooldown, but each landing after the previous reboot would have already finished) — fails with 10 reboots on the unpatched code, passes with 1 reboot with the fix.TestOpcacheResetstress test (500 concurrent requests spammingopcache_reset()under load) still passes.Fixes #2553.