diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 5f95c0ab3..bd10ff779 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" @@ -11,6 +12,7 @@ import ( "sync" "sync/atomic" "testing" + "time" "github.com/caddyserver/caddy/v2" "github.com/caddyserver/caddy/v2/caddytest" @@ -1472,3 +1474,70 @@ func TestDd(t *testing.T) { "dump123", ) } + +// test to force the opcache segfault race condition under concurrency (~1.7s) +func TestOpcacheReset(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(` + { + skip_install_trust + admin localhost:2999 + http_port `+testPort+` + metrics + + frankenphp { + num_threads 40 + php_ini { + opcache.enable 1 + zend_extension opcache.so + opcache.log_verbosity_level 4 + } + } + } + + localhost:`+testPort+` { + php { + root ../testdata + worker { + file sleep.php + match /sleep* + num 20 + } + } + } + `, "caddyfile") + + wg := sync.WaitGroup{} + numRequests := 100 + 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() { + // randomly call opcache_reset + if rand.IntN(10) > 5 { + tester.AssertGetResponse( + "http://localhost:"+testPort+"/opcache_reset.php", + http.StatusOK, + "opcache reset done", + ) + wg.Done() + return + } + + // otherwise call sleep.php with random sleep and work values + tester.AssertGetResponse( + fmt.Sprintf("http://localhost:%s/sleep.php?sleep=%d&work=%d", testPort, i, i), + http.StatusOK, + fmt.Sprintf("slept for %d ms and worked for %d iterations", i, i), + ) + wg.Done() + }() + } + + wg.Wait() +} diff --git a/frankenphp.c b/frankenphp.c index 04782a9b6..7b59d24fa 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -73,6 +73,7 @@ bool should_filter_var = 0; __thread uintptr_t thread_index; __thread bool is_worker_thread = false; __thread zval *os_environment = NULL; +zif_handler orig_opcache_reset; void frankenphp_update_local_thread_context(bool is_worker) { is_worker_thread = is_worker; @@ -340,6 +341,15 @@ PHP_FUNCTION(frankenphp_getenv) { } } /* }}} */ +/* {{{ thread-safe opcache reset */ +PHP_FUNCTION(frankenphp_opcache_reset) { + if (go_schedule_opcache_reset(thread_index)) { + orig_opcache_reset(INTERNAL_FUNCTION_PARAM_PASSTHRU); + } + + RETVAL_FALSE; +} /* }}} */ + /* {{{ Fetch all HTTP request headers */ PHP_FUNCTION(frankenphp_request_headers) { ZEND_PARSE_PARAMETERS_NONE(); @@ -570,6 +580,15 @@ PHP_MINIT_FUNCTION(frankenphp) { php_error(E_WARNING, "Failed to find built-in getenv function"); } + // Override opcache_reset + func = zend_hash_str_find_ptr(CG(function_table), "opcache_reset", + sizeof("opcache_reset") - 1); + if (func != NULL && func->type == ZEND_INTERNAL_FUNCTION) { + orig_opcache_reset = ((zend_internal_function *)func)->handler; + ((zend_internal_function *)func)->handler = + ZEND_FN(frankenphp_opcache_reset); + } + return SUCCESS; } diff --git a/frankenphp.go b/frankenphp.go index da0bdead3..09b87cf53 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,10 @@ var ( contextKey = contextKeyStruct{} serverHeader = []string{"FrankenPHP"} - isRunning bool - onServerShutdown []func() + isRunning bool + isOpcacheResetting atomic.Bool + threadsAreRestarting atomic.Bool + onServerShutdown []func() // Set default values to make Shutdown() idempotent globalMu sync.Mutex @@ -698,6 +703,61 @@ 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) C.bool { + if isOpcacheResetting.CompareAndSwap(false, true) { + restartThreadsForOpcacheReset(nil) + return C.bool(true) + } + + return C.bool(phpThreads[threadIndex].state.Is(state.Restarting)) +} + +// restart all threads for an opcache_reset +func restartThreadsForOpcacheReset(exceptThisThread *phpThread) { + if threadsAreRestarting.Load() { + // ignore reloads while a restart is already ongoing + return + } + + // disallow scaling threads while restarting workers + scalingMu.Lock() + defer scalingMu.Unlock() + + // drain workers + globalLogger.Info("Restarting all PHP threads for opcache_reset") + threadsToRestart := drainWorkerThreads() + + // drain regular threads + globalLogger.Info("Draining regular PHP threads for opcache_reset") + wg := sync.WaitGroup{} + for _, thread := range regularThreads { + if thread.state.Is(state.Ready) { + threadsToRestart = append(threadsToRestart, thread) + thread.state.Set(state.Restarting) + close(thread.drainChan) + + wg.Go(func() { + thread.state.WaitFor(state.Yielding) + }) + } + } + + // other threads may not parse new scripts while this thread is scheduling an opcache_reset + // sleeping a bit here makes this much less likely to happen + // waiting for all other threads to drain first can potentially deadlock + time.Sleep(100 * time.Millisecond) + + go func() { + wg.Wait() + for _, thread := range threadsToRestart { + thread.drainChan = make(chan struct{}) + thread.state.Set(state.Ready) + isOpcacheResetting.Store(false) + } + }() +} + // ExecuteScriptCLI executes the PHP script passed as parameter. // It returns the exit status code of the script. func ExecuteScriptCLI(script string, args []string) int { diff --git a/phpmainthread_test.go b/phpmainthread_test.go index b777c3394..a57021511 100644 --- a/phpmainthread_test.go +++ b/phpmainthread_test.go @@ -97,7 +97,7 @@ func TestTransitionThreadsWhileDoingRequests(t *testing.T) { var ( isDone atomic.Bool - wg sync.WaitGroup + wg sync.WaitGroup ) numThreads := 10 diff --git a/testdata/opcache_reset.php b/testdata/opcache_reset.php new file mode 100644 index 000000000..b19a80bf4 --- /dev/null +++ b/testdata/opcache_reset.php @@ -0,0 +1,9 @@ +