diff --git a/frankenphp.c b/frankenphp.c index b15507f69d..f9927354b1 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -1017,9 +1017,21 @@ PHP_FUNCTION(frankenphp_log) { } } +/* Scheduled by opcache itself, on shared memory exhaustion or hash overflow. + * The reason is ignored: opcache_reset() is already intercepted below, so only + * the automatic reasons reach this hook. Guarded like the assignment in + * php_main(), which is its only caller, so builds without the hook do not trip + * -Werror=unused-function. */ +#if defined(ZTS) && PHP_VERSION_ID >= 80400 +static void frankenphp_opcache_restart_hook(int reason) { + (void)reason; + go_schedule_opcache_reset(frankenphp_thread_index(), true); +} +#endif + /* {{{ thread-safe opcache reset */ PHP_FUNCTION(frankenphp_opcache_reset) { - go_schedule_opcache_reset(frankenphp_thread_index()); + go_schedule_opcache_reset(frankenphp_thread_index(), false); RETVAL_TRUE; } /* }}} */ @@ -1705,6 +1717,13 @@ static void *php_main(void *arg) { frankenphp_sapi_module.startup(&frankenphp_sapi_module); +#if defined(ZTS) && PHP_VERSION_ID >= 80400 + /* Also restart everything on opcache memory overflow or similar events, the + * hook is triggered right before an opcache reset is scheduled + */ + zend_accel_schedule_restart_hook = frankenphp_opcache_restart_hook; +#endif + /* check if a default filter is set in php.ini and only filter if * it is, this is deprecated and will be removed in PHP 9 */ char *default_filter; diff --git a/frankenphp.go b/frankenphp.go index 79b135b808..92b4b07a72 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -773,11 +773,58 @@ func go_is_context_done(threadIndex C.uintptr_t) C.bool { return C.bool(phpThreads[threadIndex].handler.frankenPHPContext().isDone) } +const ( + // Bounds on how often opcache is expected to restart by itself. Beyond + // this the compiled code does not fit in the configured shared memory and + // opcache is thrashing, which only the operator can fix. + automaticOpcacheRestartWindow = time.Minute + automaticOpcacheRestartThreshold = 5 +) + +var automaticOpcacheRestarts struct { + sync.Mutex + windowStart time.Time + count int +} + +// recordAutomaticOpcacheRestart counts the restarts opcache schedules on its +// own and reports true the first time the threshold is crossed in a window, +// so the caller logs once per window instead of on every restart. +func recordAutomaticOpcacheRestart(now time.Time) bool { + automaticOpcacheRestarts.Lock() + defer automaticOpcacheRestarts.Unlock() + + if now.Sub(automaticOpcacheRestarts.windowStart) > automaticOpcacheRestartWindow { + automaticOpcacheRestarts.windowStart = now + automaticOpcacheRestarts.count = 0 + } + + automaticOpcacheRestarts.count++ + + return automaticOpcacheRestarts.count == automaticOpcacheRestartThreshold +} + //export go_schedule_opcache_reset -func go_schedule_opcache_reset(threadIndex C.uintptr_t) { - if mainThread != nil { - go mainThread.rebootAllThreads() +func go_schedule_opcache_reset(threadIndex C.uintptr_t, automatic C.bool) { + if mainThread == nil { + return } + + // A reboot is never skipped, not even when they pile up: opcache rewinds + // its shared memory whether or not the threads are ready for it, so + // declining to reboot brings back the crash the hook exists to prevent. + // Repeated automatic restarts are reported instead, since the fix is to + // give opcache more room. rebootAllThreads() already ignores a call made + // while a reboot is in flight. + if bool(automatic) && recordAutomaticOpcacheRestart(time.Now()) && globalLogger.Enabled(globalCtx, slog.LevelError) { + globalLogger.LogAttrs(globalCtx, slog.LevelError, + "opcache keeps restarting and PHP threads are rebooting each time, raise opcache.memory_consumption and opcache.max_accelerated_files", + slog.Int("restarts", automaticOpcacheRestartThreshold), + slog.Duration("window", automaticOpcacheRestartWindow), + ) + } + + go mainThread.rebootAllThreads() } func convertArgs(args []string) (C.int, []*C.char) { diff --git a/opcache_test.go b/opcache_test.go new file mode 100644 index 0000000000..fe706e1bd4 --- /dev/null +++ b/opcache_test.go @@ -0,0 +1,75 @@ +package frankenphp_test + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestOpcacheRestartKeepsWorkerThreadsAlive guards the opcache restart hook. +// +// A worker holds references into opcache shared memory for its whole life. An +// opcache restart rewinds that memory to a startup watermark while the worker +// is still executing, because opcache defers a restart until +// accel_is_inactive(), which is an fcntl probe that cannot see threads of the +// calling process. The hook reboots every thread before that happens. +// +// Without the hook this test does not fail, it takes the process down with +// SIGSEGV, which is the regression being guarded. +func TestOpcacheRestartKeepsWorkerThreadsAlive(t *testing.T) { + t.Cleanup(func() { + _ = os.RemoveAll(filepath.Join(os.TempDir(), "frankenphp-opcache-restart-test")) + }) + + runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) { + get := func(path string) string { + w := httptest.NewRecorder() + handler(w, httptest.NewRequest("GET", "http://example.com"+path, nil)) + + return strings.TrimSpace(w.Body.String()) + } + + if body := get("/opcache/trigger.php?b=0"); body == "NOOPCACHE" { + t.Skip("opcache is not available in this build") + } + + require.Equal(t, "OK", get("/opcache/worker.php"), "worker must be healthy before the restart") + + // Compile until opcache performs a restart. The counters only move + // once a restart has actually been carried out, not merely scheduled. + restarted := false + for b := 1; b <= 20 && !restarted; b++ { + body := get("/opcache/trigger.php?b=" + strconv.Itoa(b)) + if n, err := strconv.Atoi(body); err == nil && n > 0 { + restarted = true + } + } + + if !restarted { + t.Skip("could not force an opcache restart in this environment") + } + + assert.Equal(t, "OK", get("/opcache/worker.php"), "worker must survive an opcache restart") + }, &testOptions{ + workerScript: "opcache/worker.php", + nbWorkers: 1, + nbParallelRequests: 1, + phpIni: map[string]string{ + "opcache.enable": "1", + "opcache.enable_cli": "1", + "opcache.memory_consumption": "8", + "opcache.interned_strings_buffer": "1", + "opcache.max_accelerated_files": "200", + "opcache.file_update_protection": "0", + "opcache.validate_timestamps": "1", + "opcache.revalidate_freq": "0", + }, + }) +} diff --git a/opcacherestart_test.go b/opcacherestart_test.go new file mode 100644 index 0000000000..4701399dd1 --- /dev/null +++ b/opcacherestart_test.go @@ -0,0 +1,46 @@ +package frankenphp + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestAutomaticOpcacheRestartsAreReportedOncePerWindow(t *testing.T) { + reset := func() { + automaticOpcacheRestarts.Lock() + automaticOpcacheRestarts.windowStart = time.Time{} + automaticOpcacheRestarts.count = 0 + automaticOpcacheRestarts.Unlock() + } + + t.Run("stays quiet below the threshold", func(t *testing.T) { + reset() + now := time.Now() + for i := 1; i < automaticOpcacheRestartThreshold; i++ { + assert.False(t, recordAutomaticOpcacheRestart(now), "restart %d must not report", i) + } + }) + + t.Run("reports exactly once when the threshold is crossed", func(t *testing.T) { + reset() + now := time.Now() + reported := 0 + for i := 0; i < automaticOpcacheRestartThreshold*3; i++ { + if recordAutomaticOpcacheRestart(now) { + reported++ + } + } + assert.Equal(t, 1, reported, "a burst must be reported once, not on every restart") + }) + + t.Run("restarts spread out never reach the threshold", func(t *testing.T) { + reset() + now := time.Now() + for i := 0; i < automaticOpcacheRestartThreshold*3; i++ { + now = now.Add(automaticOpcacheRestartWindow + time.Second) + assert.False(t, recordAutomaticOpcacheRestart(now), "occasional restarts are normal") + } + }) +} diff --git a/testdata/opcache/trigger.php b/testdata/opcache/trigger.php new file mode 100644 index 0000000000..b621e69ee6 --- /dev/null +++ b/testdata/opcache/trigger.php @@ -0,0 +1,36 @@ + 1, 'b' => [2, 3], 'c' => 'literal']; + +$str = 'opcache_canary_interned_string'; +$arr = OPCACHE_CANARY; + +// Fingerprints are plain integers on the thread heap, so a rewind cannot +// touch them. +$expLen = strlen($str); +$expCrc = crc32($str); +$expArr = crc32(serialize($arr)); + +$handler = static function () use (&$str, &$arr, $expLen, $expCrc, $expArr) { + if (strlen($str) !== $expLen || crc32($str) !== $expCrc || crc32(serialize($arr)) !== $expArr) { + echo 'CORRUPT'; + + return; + } + + echo 'OK'; +}; + +for ($running = true; $running;) { + $running = frankenphp_handle_request($handler); +}