fix: safe opcache_reset

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()
+}

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;
 }
 

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 {

phpmainthread_test.go

@@ -97,7 +97,7 @@ func TestTransitionThreadsWhileDoingRequests(t *testing.T) {
 
 	var (
 		isDone atomic.Bool
-		wg sync.WaitGroup
+		wg     sync.WaitGroup
 	)
 
 	numThreads := 10

testdata/opcache_reset.php

@@ -0,0 +1,9 @@
+<?php
+
+require_once __DIR__.'/_executor.php';
+
+return function () {
+    require __DIR__ .'/require.php';
+    opcache_reset();
+    echo "opcache reset done";
+};

testdata/require.php

@@ -0,0 +1,6 @@
+<?php
+
+// dummy require file for opcache_reset test
+return function (){
+    echo "";
+};

threadregular.go

@@ -1,5 +1,7 @@
 package frankenphp
 
+// #include "frankenphp.h"
+import "C"
 import (
 	"context"
 	"runtime"
@@ -46,6 +48,10 @@ func (handler *regularThread) beforeScriptExecution() string {
 		handler.state.Set(state.Ready)
 
 		return handler.waitForRequest()
+	case state.Restarting:
+		handler.state.Set(state.Yielding)
+		handler.state.WaitFor(state.Ready, state.ShuttingDown)
+		return handler.beforeScriptExecution()
 
 	case state.Ready:
 		return handler.waitForRequest()

watcher.go

@@ -10,7 +10,7 @@ import (
 )
 
 type hotReloadOpt struct {
-	hotReload   []*watcher.PatternGroup
+	hotReload []*watcher.PatternGroup
 }
 
 var restartWorkers atomic.Bool

worker.go

@@ -213,6 +213,8 @@ func drainWorkerThreads() []*phpThread {
 // RestartWorkers attempts to restart all workers gracefully
 // All workers must be restarted at the same time to prevent issues with opcache resetting.
 func RestartWorkers() {
+	threadsAreRestarting.Store(true)
+	defer threadsAreRestarting.Store(false)
 	// disallow scaling threads while restarting workers
 	scalingMu.Lock()
 	defer scalingMu.Unlock()