Review by AlliBalliBaba

Author: nicolas-grekas

frankenphp.go

@@ -318,7 +318,9 @@ func Init(options ...Option) error {
 	}
 
 	regularRequestChan = make(chan contextHolder)
+	regularThreadMu.Lock()
 	regularThreads = make([]*phpThread, 0, opt.numThreads-workerThreadCount)
+	regularThreadMu.Unlock()
 	for i := 0; i < opt.numThreads-workerThreadCount; i++ {
 		convertToRegularThread(getInactivePHPThread())
 	}
@@ -371,13 +373,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 +791,5 @@ func resetGlobals() {
 	watcherIsEnabled = false
 	maxIdleTime = defaultMaxIdleTime
 	maxRequestsPerThread = 0
-	shutdownInProgress.Store(false)
 	globalMu.Unlock()
 }

internal/state/state.go

@@ -30,6 +30,9 @@ const (
 	TransitionRequested
 	TransitionInProgress
 	TransitionComplete
+
+	// thread is exiting the C loop for a full ZTS restart (max_requests)
+	Rebooting
 )
 
 func (s State) String() string {
@@ -58,6 +61,8 @@ func (s State) String() string {
 		return "transition in progress"
 	case TransitionComplete:
 		return "transition complete"
+	case Rebooting:
+		return "rebooting"
 	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)},
-	})
-}

maxrequests_test.go

@@ -64,6 +64,11 @@ func (thread *phpThread) boot() {
 
 // shutdown the underlying PHP thread
 func (thread *phpThread) shutdown() {
+	// if rebooting, wait for the reboot goroutine to finish (it will bail if server is shutting down)
+	if thread.state.Is(state.Rebooting) {
+		thread.state.WaitFor(state.Ready, state.Inactive, state.Done, state.Reserved)
+	}
+
 	if !thread.state.RequestSafeStateChange(state.ShuttingDown) {
 		// already shutting down or done, wait for the C thread to finish
 		thread.state.WaitFor(state.Done, state.Reserved)