diff --git a/frankenphp.go b/frankenphp.go index ad2dedc42a..6253d4a22c 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -784,9 +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 { - go mainThread.rebootAllThreads() + // 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, 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. + // + // 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 + } + 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 + } + } + + // 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/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..337e271e7e 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -26,6 +26,16 @@ 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. + // 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 ( @@ -36,6 +46,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,