From 0301b79815730579663b9bbec1f775ea25bf946a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Mon, 27 Jul 2026 23:44:05 +0200 Subject: [PATCH 1/2] fix: throttle opcache_reset()-triggered thread pool reboots 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 https://github.com/php/frankenphp/issues/2553. --- frankenphp.go | 19 +++++++++++++++++-- frankenphp_test.go | 29 +++++++++++++++++++++++++++++ phpmainthread.go | 9 +++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/frankenphp.go b/frankenphp.go index ad2dedc42a..2a568b3373 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -784,9 +784,24 @@ func go_is_context_done(threadIndex C.uintptr_t) C.bool { //export go_schedule_opcache_reset func go_schedule_opcache_reset(threadIndex C.uintptr_t) { - if mainThread != nil { - go mainThread.rebootAllThreads() + if mainThread == nil { + return } + + // Application code (e.g. some WordPress plugins) can call opcache_reset() + // on every request. Since a reboot pauses request handling for every + // thread, coalesce calls that arrive within the cooldown into a single + // reboot instead of pausing the whole pool on every call. + mainThread.opcacheResetMu.Lock() + now := time.Now() + if now.Sub(mainThread.lastOpcacheResetAt) < opcacheResetCooldown { + mainThread.opcacheResetMu.Unlock() + return + } + mainThread.lastOpcacheResetAt = now + mainThread.opcacheResetMu.Unlock() + + go mainThread.rebootAllThreads() } func convertArgs(args []string) (C.int, []*C.char) { diff --git a/frankenphp_test.go b/frankenphp_test.go index 409e644634..4e851a96d4 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -184,6 +184,35 @@ func testFinishRequest(t *testing.T, opts *testOptions) { }, opts) } +// TestOpcacheResetIsThrottled reproduces https://github.com/php/frankenphp/issues/2553: +// application code (e.g. some WordPress plugins) can call opcache_reset() on +// every request, and each call reboots the whole PHP thread pool. Without a +// cooldown, a burst of concurrent calls reboots the pool once per call, +// pausing request handling for the whole site on every one of them. +func TestOpcacheResetIsThrottled(t *testing.T) { + var buf fmt.Stringer + opts := &testOptions{nbParallelRequests: 1} + opts.logger, buf = newTestLogger(t) + + runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) { + // Calls spaced out enough that, without a cooldown, each one lands + // after the previous reboot already finished (so each would trigger + // its own reboot), but still well within the cooldown window. + for n := 0; n < 10; n++ { + body, _ := testGet("http://example.com/opcache_reset.php", handler, t) + assert.Equal(t, "opcache reset done", body) + time.Sleep(50 * time.Millisecond) + } + }, opts) + + require.Eventually(t, func() bool { + return strings.Contains(buf.String(), "thread reboot finished") + }, 2*time.Second, 10*time.Millisecond, "the throttled reboot should still eventually run") + + count := strings.Count(buf.String(), "rebooting all PHP threads") + assert.Equal(t, 1, count, "opcache_reset() calls within the cooldown must be coalesced into a single reboot") +} + func TestServerVariable_module(t *testing.T) { testServerVariable(t, nil) } diff --git a/phpmainthread.go b/phpmainthread.go index 175460ac1e..1b2d1046da 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -26,6 +26,12 @@ type phpMainThread struct { maxThreads int phpIni map[string]string isRebooting atomic.Bool + + // throttles opcache_reset()-triggered reboots (see go_schedule_opcache_reset): + // application code can call opcache_reset() on every request, and each call + // would otherwise pause request handling for a full thread pool reboot + opcacheResetMu sync.Mutex + lastOpcacheResetAt time.Time } var ( @@ -36,6 +42,9 @@ var ( // timeouts to wait for threads to yield before arming force-kill shutDownGracePeriod = 30 * time.Second rebootGracePeriod = 6 * time.Second + + // minimum interval between two opcache_reset()-triggered reboots + opcacheResetCooldown = 5 * time.Second ) // initPHPThreads starts the main PHP thread, From 73d4974dd80a25eda5038da315adc3a590349e44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Wed, 29 Jul 2026 14:59:26 +0200 Subject: [PATCH 2/2] fix: coalesce opcache_reset() calls within the cooldown instead of dropping them @alexandre-daubois caught two real bugs in review: 1. A call landing inside the cooldown did nothing at all, yet frankenphp_opcache_reset() unconditionally reports success to PHP. If a burst ends on such a call, the reset it wanted never happens - and since the same hook fires on an internal opcache restart (OOM), a dropped one could leave the accelerator disabled indefinitely. 2. lastOpcacheResetAt was stamped before rebootAllThreads() ran, whose return value was discarded. rebootAllThreads() can no-op (already rebooting, or the pool isn't Ready), so a no-op call could still arm a fresh cooldown window and swallow the next, legitimate request. Fix: track whether a reboot is scheduled, in flight, or in its post-reboot cooldown hold, and coalesce any calls arriving during that whole span into the one pending/just-finished reboot instead of dropping them. Only stamp lastOpcacheResetAt after rebootAllThreads() actually returns true. Holding the cooldown through a full window after completion (not just until the reboot itself finishes) matters here: threads can yield well within a grace period, so without it, a call landing in the gap between "reboot done" and "cooldown elapsed" would open a second window instead of joining the first. That trailing hold introduced its own bug, caught by -race: the goroutine holding it can outlive Shutdown() by design (it's coalescing calls that might still arrive), but it read the package-level mainThread var again after waking up - which a later, unrelated test's Init() had by then reassigned to a different instance, racing on that instance's own fields. Fixed by capturing the *phpMainThread once and using only that local copy for the goroutine's lifetime, and by selecting on its done channel to give up immediately once its specific instance starts shutting down rather than waiting out an arbitrary delay first. --- frankenphp.go | 75 ++++++++++++++++++++++++++++++++++++++++++------ phpmainthread.go | 10 +++++-- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/frankenphp.go b/frankenphp.go index 2a568b3373..6253d4a22c 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -784,24 +784,81 @@ func go_is_context_done(threadIndex C.uintptr_t) C.bool { //export go_schedule_opcache_reset func go_schedule_opcache_reset(threadIndex C.uintptr_t) { - if mainThread == nil { + // Capture the current *phpMainThread once: this goroutine can outlive + // this specific instance (e.g. Shutdown() runs while it's still holding + // the trailing cooldown below). mainThread is reassigned to a brand new + // instance by the next Init(), and phpThreads is reset with it; a stale + // goroutine that read the package-level mainThread var again later would + // operate on a different, unrelated, possibly concurrently-running + // instance instead of quietly becoming a no-op. + mt := mainThread + if mt == nil { return } // Application code (e.g. some WordPress plugins) can call opcache_reset() - // on every request. Since a reboot pauses request handling for every + // on every request, and the same hook fires on an internal opcache + // restart (OOM, etc.). Since a reboot pauses request handling for every // thread, coalesce calls that arrive within the cooldown into a single // reboot instead of pausing the whole pool on every call. - mainThread.opcacheResetMu.Lock() - now := time.Now() - if now.Sub(mainThread.lastOpcacheResetAt) < opcacheResetCooldown { - mainThread.opcacheResetMu.Unlock() + // + // A call inside the cooldown must still result in a reboot once the + // cooldown ends, not be dropped: frankenphp_opcache_reset() always + // reports success to PHP regardless of what happens here, and a dropped + // OOM-triggered restart would leave the accelerator disabled until + // threads happen to reboot for an unrelated reason. opcacheResetScheduled + // coalesces any calls arriving while one is already pending, in flight, + // or in its post-reboot cooldown hold into that single reboot, rather + // than silently discarding them. + mt.opcacheResetMu.Lock() + if mt.opcacheResetScheduled { + mt.opcacheResetMu.Unlock() return } - mainThread.lastOpcacheResetAt = now - mainThread.opcacheResetMu.Unlock() + mt.opcacheResetScheduled = true + wait := opcacheResetCooldown - time.Since(mt.lastOpcacheResetAt) + mt.opcacheResetMu.Unlock() + + go func() { + defer func() { + mt.opcacheResetMu.Lock() + mt.opcacheResetScheduled = false + mt.opcacheResetMu.Unlock() + }() + + if wait > 0 { + select { + case <-time.After(wait): + case <-mt.done: + // This instance is shutting down: give up rather than call + // rebootAllThreads on a stopped pool after an arbitrary delay. + return + } + } - go mainThread.rebootAllThreads() + // rebootAllThreads can no-op (already rebooting from an unrelated + // trigger, or the pool isn't Ready, e.g. mid-shutdown): only hold + // the cooldown, and only stamp lastOpcacheResetAt, when a reboot + // actually happened. Charging a cooldown against a reset that never + // occurred would silently swallow the next legitimate request too. + if !mt.rebootAllThreads() { + return + } + + // Keep coalescing calls through a full cooldown measured from here, + // not just until the reboot itself finishes: threads can yield well + // within a grace period, so a call landing in the gap between + // "reboot done" and "cooldown elapsed" would otherwise open a brand + // new window instead of joining this one, turning one burst into + // two reboots. + select { + case <-time.After(opcacheResetCooldown): + mt.opcacheResetMu.Lock() + mt.lastOpcacheResetAt = time.Now() + mt.opcacheResetMu.Unlock() + case <-mt.done: + } + }() } func convertArgs(args []string) (C.int, []*C.char) { diff --git a/phpmainthread.go b/phpmainthread.go index 1b2d1046da..337e271e7e 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -29,9 +29,13 @@ type phpMainThread struct { // throttles opcache_reset()-triggered reboots (see go_schedule_opcache_reset): // application code can call opcache_reset() on every request, and each call - // would otherwise pause request handling for a full thread pool reboot - opcacheResetMu sync.Mutex - lastOpcacheResetAt time.Time + // would otherwise pause request handling for a full thread pool reboot. + // lastOpcacheResetAt is only ever set after a reboot actually happens, and + // opcacheResetScheduled coalesces (rather than drops) calls that arrive + // while one is already pending or in flight. + opcacheResetMu sync.Mutex + lastOpcacheResetAt time.Time + opcacheResetScheduled bool } var (