diff --git a/phpthread.go b/phpthread.go index 325996195d..b0ee527481 100644 --- a/phpthread.go +++ b/phpthread.go @@ -114,36 +114,47 @@ func (thread *phpThread) forceReboot() bool { // shutdown the underlying PHP thread func (thread *phpThread) shutdown() { if !thread.state.RequestSafeStateChange(state.ShuttingDown) { - // thread is already shutting down, prefer the stable reserved state over done + // a worker that failed to boot published ShuttingDown itself and is + // still walking its C exit path: join it before TSRM is torn down + thread.waitForExit(state.Done, state.Reserved) + // prefer the stable reserved state over done _ = thread.state.CompareAndSwap(state.Done, state.Reserved) + return } close(thread.drainChan) + thread.waitForExit(state.Done) + + thread.drainChan = make(chan struct{}) + + // threads go back to the reserved state from which they can be booted again + thread.state.Set(state.Reserved) +} + +// waitForExit joins the C thread, arming force-kill past the grace period +func (thread *phpThread) waitForExit(exitStates ...state.State) { + if thread.state.WaitForStateWithTimeout(shutDownGracePeriod, exitStates...) { + return + } + // Arm force-kill after the grace period to wake any thread stuck in // a blocking syscall (sleep, blocking I/O). The wait remains // unbounded - on platforms where force-kill cannot interrupt the // syscall (macOS, Windows non-alertable Sleep) the thread will exit // when the syscall completes naturally; the operator's orchestrator // is responsible for any harder timeout. - if !thread.state.WaitForStateWithTimeout(shutDownGracePeriod, state.Done) { - globalLogger.LogAttrs( - globalCtx, - slog.LevelWarn, - "force-killing thread on shutdown timeout", - slog.String("name", thread.name()), - slog.String("state", thread.state.Name()), - slog.String("timeout", shutDownGracePeriod.String()), - ) - thread.sendKillSignal() - thread.state.WaitFor(state.Done) - } - - thread.drainChan = make(chan struct{}) - - // threads go back to the reserved state from which they can be booted again - thread.state.Set(state.Reserved) + globalLogger.LogAttrs( + globalCtx, + slog.LevelWarn, + "force-killing thread on shutdown timeout", + slog.String("name", thread.name()), + slog.String("state", thread.state.Name()), + slog.String("timeout", shutDownGracePeriod.String()), + ) + thread.sendKillSignal() + thread.state.WaitFor(exitStates...) } // setHandler changes the thread handler safely diff --git a/worker_internal_test.go b/worker_internal_test.go index c0e405b701..f48fc16fb2 100644 --- a/worker_internal_test.go +++ b/worker_internal_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/dunglas/frankenphp/internal/state" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -76,3 +77,46 @@ func TestRestartWorkersForceKillsStuckThread(t *testing.T) { assert.NotContains(t, recorder.Body.String(), "should not reach", "VM interrupt was never observed; sleep returned naturally") } + +// Init() must not return, nor the runtime tear SAPI/TSRM down, while a worker +// that failed to boot is still inside its shutdown handler +func TestInitJoinsAThreadStuckInStartupTeardown(t *testing.T) { + t.Cleanup(Shutdown) + + var failedThread *phpThread + held := make(chan struct{}) + release := make(chan struct{}) + initDone := make(chan error, 1) + + go func() { + initDone <- Init( + WithNumThreads(2), + WithWorkers("held-failing-worker", testDataPath+"/failing-worker.php", 1, + WithWorkerMaxFailures(0), + WithWorkerOnShutdown(func(threadIndex int) { + failedThread = phpThreads[threadIndex] + close(held) + <-release + }), + ), + ) + }() + + select { + case <-held: + case err := <-initDone: + t.Fatalf("Init returned before the failed worker thread started its teardown: %v", err) + } + + select { + case err := <-initDone: + t.Fatalf("Init returned while the failed worker thread was still shutting down (state: %s, error: %v)", failedThread.state.Name(), err) + case <-time.After(500 * time.Millisecond): + } + + require.True(t, failedThread.state.Is(state.ShuttingDown), "thread should still be shutting down") + close(release) + + assert.Error(t, <-initDone, "a worker failing to boot must fail Init") + assert.True(t, failedThread.state.Is(state.Reserved), "the thread must have exited and been reclaimed before Init returned, got: "+failedThread.state.Name()) +}