Review by AlliBalliBaba

Author: nicolas-grekas

docs/config.md

@@ -191,7 +191,6 @@ php_server [<matcher>] {
 		watch <path> # Sets the path to watch for file changes. Can be specified more than once for multiple paths.
 		env <key> <value> # Sets an extra environment variable to the given value. Can be specified more than once for multiple environment variables. Environment variables for this worker are also inherited from the php_server parent, but can be overwritten here.
 		match <path> # match the worker to a path pattern. Overrides try_files and can only be used in the php_server directive.
-		max_requests <num> # Sets the maximum number of requests a worker thread will handle before restarting, useful for mitigating memory leaks. Default: 0 (unlimited).
 	}
 	worker <other_file> <num> # Can also use the short form like in the global frankenphp block.
 }
@@ -285,7 +284,7 @@ The `max_requests` setting in the global `frankenphp` block applies to all PHP t
 ```
 
 When a thread reaches the limit, the underlying C thread is fully restarted,
-cleaning up all ZTS thread-local storage, including any memory leaked by PHP extensions.
+cleaning up all memory and state, including any memory leaked by PHP extensions.
 Other threads continue to serve requests during the restart, so there is no downtime.
 
 Set to `0` (default) to disable the limit and let threads run indefinitely.

frankenphp.go

@@ -371,13 +371,6 @@ func Shutdown() {
 
 	drainWatchers()
 	drainAutoScaling()
-
-	// signal restart goroutines to stop spawning and wait for in-flight ones
-	shutdownInProgress.Store(true)
-	for restartingThreads.Load() > 0 {
-		runtime.Gosched()
-	}
-
 	drainPHPThreads()
 
 	metrics.Shutdown()
@@ -796,6 +789,5 @@ func resetGlobals() {
 	watcherIsEnabled = false
 	maxIdleTime = defaultMaxIdleTime
 	maxRequestsPerThread = 0
-	shutdownInProgress.Store(false)
 	globalMu.Unlock()
 }

frankenphp_test.go

@@ -23,8 +23,8 @@ import (
 	"os"
 	"os/exec"
 	"os/user"
-	"runtime"
 	"path/filepath"
+	"runtime"
 	"strconv"
 	"strings"
 	"sync"
@@ -46,7 +46,6 @@ type testOptions struct {
 	realServer         bool
 	logger             *slog.Logger
 	initOpts           []frankenphp.Option
-	workerOpts         []frankenphp.WorkerOption
 	requestOpts        []frankenphp.RequestOption
 	phpIni             map[string]string
 }
@@ -68,7 +67,6 @@ func runTest(t *testing.T, test func(func(http.ResponseWriter, *http.Request), *
 			frankenphp.WithWorkerEnv(opts.env),
 			frankenphp.WithWorkerWatchMode(opts.watch),
 		}
-		workerOpts = append(workerOpts, opts.workerOpts...)
 		initOpts = append(initOpts, frankenphp.WithWorkers("workerName", testDataDir+opts.workerScript, opts.nbWorkers, workerOpts...))
 	}
 	initOpts = append(initOpts, opts.initOpts...)

internal/state/state.go

@@ -30,6 +30,11 @@ const (
 	TransitionRequested
 	TransitionInProgress
 	TransitionComplete
+
+	// thread is exiting the C loop for a full ZTS restart (max_requests)
+	Rebooting
+	// C thread has exited and ZTS state is cleaned up, ready to spawn a new C thread
+	RebootReady
 )
 
 func (s State) String() string {
@@ -58,6 +63,10 @@ func (s State) String() string {
 		return "transition in progress"
 	case TransitionComplete:
 		return "transition complete"
+	case Rebooting:
+		return "rebooting"
+	case RebootReady:
+		return "reboot ready"
 	default:
 		return "unknown"
 	}

maxrequests_regular_test.go

@@ -1,18 +1,15 @@
 package frankenphp_test
 
 import (
-	"io"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"sync"
 	"testing"
-	"time"
 
 	"github.com/dunglas/frankenphp"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 // TestModuleMaxRequests verifies that regular (non-worker) PHP threads restart
@@ -24,30 +21,19 @@ func TestModuleMaxRequests(t *testing.T) {
 	var buf syncBuffer
 	logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))
 
-	runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, _ int) {
-		require.NotNil(t, ts)
-		client := &http.Client{Timeout: 5 * time.Second}
-
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
 		for i := 0; i < totalRequests; i++ {
-			resp, err := client.Get(ts.URL + "/index.php")
-			require.NoError(t, err, "request %d should succeed", i)
-
-			body, err := io.ReadAll(resp.Body)
-			require.NoError(t, err)
-			_ = resp.Body.Close()
-
-			assert.Equal(t, 200, resp.StatusCode, "request %d should return 200, got body: %s", i, string(body))
-			assert.Contains(t, string(body), "I am by birth a Genevese",
-				"request %d should return correct body", i)
+			body, resp := testGet("http://example.com/index.php", handler, t)
+			assert.Equal(t, 200, resp.StatusCode)
+			assert.Contains(t, body, "I am by birth a Genevese")
 		}
 
 		restartCount := strings.Count(buf.String(), "max requests reached, restarting thread")
 		t.Logf("Thread restarts observed: %d", restartCount)
 		assert.GreaterOrEqual(t, restartCount, 2,
 			"with maxRequests=%d and %d requests on 2 threads, at least 2 restarts should occur", maxRequests, totalRequests)
 	}, &testOptions{
-		realServer: true,
-		logger:     logger,
+		logger: logger,
 		initOpts: []frankenphp.Option{
 			frankenphp.WithNumThreads(2),
 			frankenphp.WithMaxRequests(maxRequests),
@@ -60,69 +46,24 @@ func TestModuleMaxRequests(t *testing.T) {
 func TestModuleMaxRequestsConcurrent(t *testing.T) {
 	const maxRequests = 10
 	const totalRequests = 200
-	const concurrency = 20
 
-	runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, _ int) {
-		require.NotNil(t, ts)
-		client := &http.Client{Timeout: 10 * time.Second}
-
-		var successCount int
-		var mu sync.Mutex
-		sem := make(chan struct{}, concurrency)
+	runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) {
 		var wg sync.WaitGroup
 
 		for i := 0; i < totalRequests; i++ {
 			wg.Add(1)
-			sem <- struct{}{}
-			go func(i int) {
-				defer func() { <-sem; wg.Done() }()
-
-				resp, err := client.Get(ts.URL + "/index.php")
-				if err != nil {
-					return
-				}
-				body, _ := io.ReadAll(resp.Body)
-				_ = resp.Body.Close()
-
-				if resp.StatusCode == 200 && strings.Contains(string(body), "I am by birth a Genevese") {
-					mu.Lock()
-					successCount++
-					mu.Unlock()
-				}
-			}(i)
+			go func() {
+				defer wg.Done()
+				body, resp := testGet("http://example.com/index.php", handler, t)
+				assert.Equal(t, 200, resp.StatusCode)
+				assert.Contains(t, body, "I am by birth a Genevese")
+			}()
 		}
 		wg.Wait()
-
-		t.Logf("Success: %d/%d", successCount, totalRequests)
-		assert.GreaterOrEqual(t, successCount, int(totalRequests*0.95),
-			"at least 95%% of requests should succeed despite regular thread restarts")
 	}, &testOptions{
-		realServer: true,
 		initOpts: []frankenphp.Option{
 			frankenphp.WithNumThreads(8),
 			frankenphp.WithMaxRequests(maxRequests),
 		},
 	})
 }
-
-// TestModuleMaxRequestsZeroIsUnlimited verifies that max_requests=0 (default)
-// means threads never restart.
-func TestModuleMaxRequestsZeroIsUnlimited(t *testing.T) {
-	runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, _ int) {
-		require.NotNil(t, ts)
-		client := &http.Client{Timeout: 5 * time.Second}
-
-		for i := 0; i < 50; i++ {
-			resp, err := client.Get(ts.URL + "/index.php")
-			require.NoError(t, err)
-			body, _ := io.ReadAll(resp.Body)
-			_ = resp.Body.Close()
-
-			assert.Equal(t, 200, resp.StatusCode)
-			assert.Contains(t, string(body), "I am by birth a Genevese")
-		}
-	}, &testOptions{
-		realServer: true,
-		initOpts:   []frankenphp.Option{frankenphp.WithNumThreads(2)},
-	})
-}