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,