fix: bound the reboot wait so an unkillable thread can't wedge the pool - #2573
Draft
dunglas wants to merge 3 commits into
Draft
fix: bound the reboot wait so an unkillable thread can't wedge the pool#2573dunglas wants to merge 3 commits into
dunglas wants to merge 3 commits into
Conversation
frankenphp_force_kill_thread() only interrupts the Zend VM at the next opcode boundary. A thread parked in a blocking write to a stalled client (e.g. an unresponsive HTTP/2 peer) has already left PHP execution and can't be reached by it. rebootAllThreads()'s post-force-kill wait was unbounded (thread.state.WaitFor(state.YieldingForReboot) with no timeout), so a single such thread would hang the whole reboot forever: scalingMu stays locked, isRebooting never clears, and every other thread's reboot blocks on it too. The same unbounded wait exists one level up in RequestSafeStateChange(), so Shutdown() then hangs the same way on that thread. Bound the wait with another rebootGracePeriod after the kill signal. If the thread still hasn't yielded, give up on it: introduce a terminal Abandoned state so the thread permanently drops out of Ready/Inactive (dispatch and shutdown then skip it cleanly instead of panicking or hanging) rather than risk reusing a slot whose OS thread might still be alive. This is very likely the actual mechanism behind the escalating, un-recovering slowdown in #2553 (see the go_read_post/go_ub_write stall discussion on that issue and #2570 (comment)): any full-pool reboot — whether triggered by opcache_reset(), the watcher, or RestartWorkers() — that catches even one thread mid-write to a stalled client would wedge permanently, independent of how often reboots are triggered or by what. Reproduced with TestRebootDoesNotHangOnUnkillableThread, using a custom http.ResponseWriter whose Write() blocks forever to deterministically simulate a stalled client: hangs indefinitely on the unpatched code, resolves within ~12s (two grace periods) with the fix, and the pool remains usable afterwards.
TestShutdownDoesNotHangWithAbandonedThread reboots into an Abandoned thread (same setup as TestRebootDoesNotHangOnUnkillableThread) and then calls Shutdown() while it's still stuck. Confirmed by temporarily reverting Abandoned's addition to RequestSafeStateChange's terminal-state bucket: the test then hangs, since RequestSafeStateChange recurses forever waiting for Ready/Inactive/Reserved on a thread that will never reach any of them.
…ive thread CI caught a segfault: TestRebootDoesNotHangOnUnkillableThread crashed the whole process on every Linux job (PHP 8.2-8.5), reproducing consistently. Not reproducible on macOS or on a from-source Linux/arm64 debug ZTS build, which points at the pthread_kill(SIGRTMIN+3) call itself, only sent on Linux/FreeBSD. A thread that ignores a full rebootGracePeriod has almost certainly already left PHP execution and is parked inside Go's own runtime (e.g. go_ub_write's netpoller wait for a stalled client) rather than a PHP-recognized C syscall. force_kill_thread's signal can't reach that at all (it's not a syscall Go's scheduler is blocked in), so sending it does nothing useful for the one scenario this code exists to handle, and delivering an async signal into an OS thread that's inside Go's runtime internals - not a syscall PHP knows about - risks corrupting shared process state instead. Replace the sendKillSignal() escalation with a second, signal-free grace period before giving up. This predates this branch (sendKillSignal was already called here); this branch's new test is just the first thing that exercised it against a permanently Go-blocked thread all the way through. thread.shutdown()'s own sendKillSignal() call (phpthread.go) has the same theoretical exposure but is unchanged here - it's pre-existing, untested against this scenario, and out of scope for this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Following up on review of #2570 and #2564 for #2553.
@henderkes's comment on #2570 (a reboot-frequency throttle) was right that throttling doesn't help a stall during
go_post_read/go_ub_write. Digging into why:frankenphp_force_kill_thread()'s own doc comment says it "interrupts the Zend VM at the next opcode boundary." A thread parked in a blocking write to a stalled client (an unresponsive HTTP/2 peer, for example) has already left PHP execution — there's no opcode boundary for the interrupt to land on, so force-kill can't reach it. On some platforms it can't reach the underlying syscall either: macOS never unblocks a blocking syscall this way at all, Windows only wakes alertable I/O.rebootAllThreads()'s fallback after a failed force-kill was:If force-kill doesn't actually unstick the thread, this blocks forever. Since it runs under
scalingMu.Lock()held for the whole reboot, one thread stuck this way wedges the reboot of every other thread permanently —scalingMunever unlocks,isRebootingnever clears. The same unbounded wait exists one level up inRequestSafeStateChange(), soShutdown()then hangs on that same thread too.This plausibly explains the actual mechanism behind #2553's escalating, non-recovering slowdown, independent of how often or why a reboot is triggered (opcache, the watcher,
RestartWorkers()) — and independent of opcache being enabled at all, which matches weinbrenner disabling opcache entirely on that issue without the problem going away.#2574 now bounds the specific
go_ub_writestall with a write deadline, which should make that particular trigger rare. This PR isn't about that one trigger, though — it's a generic backstop for anything force-kill can't reach: a hung stream wrapper,flock(), a slow DNS lookup, or any other extension call blocking outside the Zend VM's opcode loop, on any platform including the ones where force-kill has no syscall-level reach at all.Fix
rebootGracePeriod.Abandonedstate. The thread permanently drops out ofReady/Inactive(its OS thread may still be alive, so its slot can't safely be reused), and dispatch/shutdown code skips it cleanly instead of panicking or hanging on it. The rest of the pool reboots and keeps serving normally; only that one thread's capacity is permanently lost.Abandonedis also added toRequestSafeStateChange()'s terminal-state bucket, which independently fixesShutdown()hanging forever on the same thread via that function's unboundedWaitFor(Ready, Inactive, Reserved)fallback.Test
TestRebootDoesNotHangOnUnkillableThreaduses a customhttp.ResponseWriterwhoseWrite()blocks forever, deterministically simulating a stalled client (no network timing flakiness needed — this is the same code path a real stalled write would hit). On the unpatched code this hangs indefinitely; with the fix it resolves in ~12s (two grace periods: one waiting for a graceful yield, one after the kill signal), and a fresh request afterwards confirms the pool is still usable.Full
go test -race ./...and thecaddypackage suite both pass.