diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index b8961b96d8..3e26229ec6 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -3,6 +3,7 @@ package caddy_test import ( "bytes" "fmt" + "math/rand/v2" "net/http" "os" "path/filepath" @@ -1730,6 +1731,74 @@ func TestDd(t *testing.T) { ) } +// test to force the opcache segfault race condition under concurrency (~1.7s) +func TestOpcacheReset(t *testing.T) { + tester := caddytest.NewTester(t) + tester.Client.Timeout = 60 * time.Second + tester.InitServer(` + { + skip_install_trust + admin localhost:2999 + http_port `+testPort+` + metrics + + frankenphp { + num_threads 40 + php_ini { + opcache.enable 1 + opcache.log_verbosity_level 4 + max_execution_time 30s + } + } + } + + localhost:`+testPort+` { + php { + root ../testdata + worker { + file sleep.php + match /sleep* + num 20 + } + } + } + `, "caddyfile") + + wg := sync.WaitGroup{} + numRequests := 500 // increase if test is flaky to get consistent failures + wg.Add(numRequests) + for i := 0; i < numRequests; i++ { + // introduce some random delay + if rand.IntN(10) > 8 { + time.Sleep(time.Millisecond * 10) + } + + go func() { + defer wg.Done() + // randomly call opcache_reset + if i%10 > 7 { + tester.AssertGetResponse( + "http://localhost:"+testPort+"/opcache_reset.php", + http.StatusOK, + "opcache reset done", + ) + return + } + + // otherwise call sleep.php with random sleep and work values + sleep := i % 100 + work := i % 100 + tester.AssertGetResponse( + fmt.Sprintf("http://localhost:%s/sleep.php?sleep=%d&work=%d", testPort, sleep, work), + http.StatusOK, + fmt.Sprintf("slept for %d ms and worked for %d iterations", sleep, work), + ) + }() + } + + wg.Wait() +} + func TestLog(t *testing.T) { tester := caddytest.NewTester(t) initServer(t, tester, ` diff --git a/frankenphp.c b/frankenphp.c index e441a4ccd2..754adb0df2 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -93,6 +93,13 @@ __thread uintptr_t thread_index; __thread bool is_worker_thread = false; __thread HashTable *sandboxed_env = NULL; +zif_handler orig_opcache_reset; + +#if PHP_VERSION_ID < 80300 +pthread_mutex_t opcache_reset_mutex_php_82; +PHP_FUNCTION(frankenphp_opcache_reset); +#endif + #ifndef PHP_WIN32 static bool is_forked_child = false; static void frankenphp_fork_child(void) { is_forked_child = true; } @@ -226,6 +233,20 @@ static void frankenphp_update_request_context() { /* let PHP handle basic auth */ php_handle_auth_data(authorization_header); + +/* On PHP 8.2 and under opcache_reset needs to be reset on every request. TODO: + * remove this once we drop support for PHP 8.2 */ +#if PHP_VERSION_ID < 80300 + zend_function *func = zend_hash_str_find_ptr( + CG(function_table), "opcache_reset", sizeof("opcache_reset") - 1); + if (func != NULL && func->type == ZEND_INTERNAL_FUNCTION) { + pthread_mutex_lock(&opcache_reset_mutex_php_82); + orig_opcache_reset = ((zend_internal_function *)func)->handler; + ((zend_internal_function *)func)->handler = + ZEND_FN(frankenphp_opcache_reset); + pthread_mutex_unlock(&opcache_reset_mutex_php_82); + } +#endif } static void frankenphp_free_request_context() { @@ -582,6 +603,13 @@ PHP_FUNCTION(frankenphp_getenv) { } } /* }}} */ +/* {{{ thread-safe opcache reset */ +PHP_FUNCTION(frankenphp_opcache_reset) { + go_schedule_opcache_reset(thread_index); + + RETVAL_TRUE; +} /* }}} */ + /* {{{ Fetch all HTTP request headers */ PHP_FUNCTION(frankenphp_request_headers) { ZEND_PARSE_PARAMETERS_NONE(); @@ -916,7 +944,9 @@ static zend_module_entry frankenphp_module = { static int frankenphp_startup(sapi_module_struct *sapi_module) { php_import_environment_variables = get_full_env; - return php_module_startup(sapi_module, &frankenphp_module); + int result = php_module_startup(sapi_module, &frankenphp_module); + + return result; } static int frankenphp_deactivate(void) { return SUCCESS; } @@ -1443,6 +1473,21 @@ static void *php_main(void *arg) { should_filter_var = default_filter != NULL; original_user_abort_setting = PG(ignore_user_abort); + /* Override opcache_reset for a thread-safe reset */ + zend_function *func = zend_hash_str_find_ptr( + CG(function_table), "opcache_reset", sizeof("opcache_reset") - 1); + if (func != NULL && func->type == ZEND_INTERNAL_FUNCTION && + ((zend_internal_function *)func)->handler != + ZEND_FN(frankenphp_opcache_reset)) { + orig_opcache_reset = ((zend_internal_function *)func)->handler; + ((zend_internal_function *)func)->handler = + ZEND_FN(frankenphp_opcache_reset); + } + +#if PHP_VERSION_ID < 80300 + pthread_mutex_init(&opcache_reset_mutex_php_82, NULL); +#endif + go_frankenphp_main_thread_is_ready(); /* channel closed, shutdown gracefully. drainPHPThreads has already @@ -1640,12 +1685,12 @@ int frankenphp_execute_script_cli(char *script, int argc, char **argv, } int frankenphp_reset_opcache(void) { - zend_function *opcache_reset = - zend_hash_str_find_ptr(CG(function_table), ZEND_STRL("opcache_reset")); - if (opcache_reset) { - zend_call_known_function(opcache_reset, NULL, NULL, NULL, 0, NULL, NULL); - } - + zend_execute_data execute_data; + zval retval; + memset(&execute_data, 0, sizeof(execute_data)); + ZVAL_UNDEF(&retval); + orig_opcache_reset(&execute_data, &retval); + zval_ptr_dtor(&retval); return 0; } diff --git a/frankenphp.go b/frankenphp.go index 52246d01c7..5fefc0106f 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -32,11 +32,14 @@ import ( "runtime" "strings" "sync" + "sync/atomic" "syscall" "time" "unsafe" // debug on Linux //_ "github.com/ianlancetaylor/cgosymbolizer" + + "github.com/dunglas/frankenphp/internal/state" ) type contextKeyStruct struct{} @@ -56,8 +59,9 @@ var ( contextKey = contextKeyStruct{} serverHeader = []string{"FrankenPHP"} - isRunning bool - onServerShutdown []func() + isRunning bool + threadsAreRestarting atomic.Bool + onServerShutdown []func() // Set default values to make Shutdown() idempotent globalMu sync.Mutex @@ -756,6 +760,131 @@ func go_is_context_done(threadIndex C.uintptr_t) C.bool { return C.bool(phpThreads[threadIndex].frankenPHPContext().isDone) } +//export go_schedule_opcache_reset +func go_schedule_opcache_reset(threadIndex C.uintptr_t) { + if threadsAreRestarting.CompareAndSwap(false, true) { + go func() { + restartThreadsAndOpcacheReset(true) + threadsAreRestarting.Store(false) + }() + } +} + +// opcacheResetOnce ensures only one thread calls the actual opcache_reset. +// Multiple threads calling it concurrently can race on shared memory. +var opcacheResetOnce sync.Once + +// restart all threads for an opcache_reset +func restartThreadsAndOpcacheReset(withRegularThreads bool) { + // disallow scaling threads while restarting workers + scalingMu.Lock() + defer scalingMu.Unlock() + + threadsToRestart := drainThreads(withRegularThreads) + + opcacheResetOnce = sync.Once{} + opcacheResetWg := sync.WaitGroup{} + for _, thread := range threadsToRestart { + thread.state.Set(state.OpcacheResetting) + opcacheResetWg.Go(func() { + thread.state.WaitFor(state.OpcacheResettingDone) + }) + } + opcacheResetWg.Wait() + + for _, thread := range threadsToRestart { + thread.drainChan = make(chan struct{}) + thread.state.Set(state.Ready) + } +} + +func drainThreads(withRegularThreads bool) []*phpThread { + var ( + ready sync.WaitGroup + drainedThreads []*phpThread + ) + + for _, worker := range workers { + worker.threadMutex.RLock() + ready.Add(len(worker.threads)) + + for _, thread := range worker.threads { + if !thread.state.RequestSafeStateChange(state.Restarting) { + ready.Done() + + // no state change allowed == thread is shutting down + // we'll proceed to restart all other threads anyway + continue + } + close(thread.drainChan) + drainedThreads = append(drainedThreads, thread) + + go func(thread *phpThread) { + thread.state.WaitFor(state.Yielding) + ready.Done() + }(thread) + } + + worker.threadMutex.RUnlock() + } + + if withRegularThreads { + regularThreadMu.RLock() + ready.Add(len(regularThreads)) + + for _, thread := range regularThreads { + if !thread.state.RequestSafeStateChange(state.Restarting) { + ready.Done() + + // no state change allowed == thread is shutting down + // we'll proceed to restart all other threads anyway + continue + } + close(thread.drainChan) + drainedThreads = append(drainedThreads, thread) + + go func(thread *phpThread) { + thread.state.WaitFor(state.Yielding) + ready.Done() + }(thread) + } + + regularThreadMu.RUnlock() + } + + // wait for all threads, force kill any thread still stuck in a blocking syscall + done := make(chan struct{}) + go func() { + ready.Wait() + close(done) + }() + + select { + case <-done: + case <-time.After(drainGracePeriod): + // Force-kill any thread still stuck in a blocking syscall, then + // keep waiting unconditionally. On platforms where force-kill + // cannot interrupt the syscall (macOS, Windows non-alertable + // Sleep) the thread exits when the syscall completes naturally. + for _, thread := range drainedThreads { + if !thread.state.Is(state.Yielding) { + thread.forceKillMu.RLock() + C.frankenphp_force_kill_thread(thread.forceKill) + thread.forceKillMu.RUnlock() + } + } + <-done + } + + return drainedThreads +} + +func scheduleOpcacheReset(thread *phpThread) { + opcacheResetOnce.Do(func() { + C.frankenphp_reset_opcache() + }) +} + func convertArgs(args []string) (C.int, []*C.char) { argc := C.int(len(args)) argv := make([]*C.char, argc) diff --git a/internal/state/state.go b/internal/state/state.go index f8d2b3acb7..28dd6a30f7 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -25,6 +25,8 @@ const ( // States necessary for restarting workers Restarting Yielding + OpcacheResetting + OpcacheResettingDone // States necessary for transitioning between different handlers TransitionRequested @@ -57,6 +59,10 @@ func (s State) String() string { return "restarting" case Yielding: return "yielding" + case OpcacheResetting: + return "opcache resetting" + case OpcacheResettingDone: + return "opcache reset done" case TransitionRequested: return "transition requested" case TransitionInProgress: diff --git a/testdata/opcache_reset.php b/testdata/opcache_reset.php new file mode 100644 index 0000000000..b19a80bf43 --- /dev/null +++ b/testdata/opcache_reset.php @@ -0,0 +1,9 @@ +