Skip to content

Commit bd06e11

Browse files
committed
docs: explain why the post-force-kill reboot wait must stay unbounded
Bounding this wait and giving up on the stuck thread looks like the obvious fix for #2553 (one stuck thread wedging every reboot forever), and #2573 tried exactly that. It's unsafe: php_main() calls tsrm_shutdown() on every reboot cycle, which requires every PHP thread to have already exited. Giving up on a thread whose OS thread is still alive violates that and crashes the process (confirmed by CI on #2573). Leave a comment so the same fix isn't attempted again without this context.
1 parent 3f43199 commit bd06e11

1 file changed

Lines changed: 18 additions & 0 deletions

File tree

phpmainthread.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,24 @@ func (mainThread *phpMainThread) rebootAllThreads() bool {
176176
slog.String("timeout", rebootGracePeriod.String()),
177177
)
178178
thread.sendKillSignal()
179+
// This wait is intentionally unbounded. force_kill_thread only
180+
// interrupts the Zend VM at the next opcode boundary or wakes a
181+
// real blocking syscall via EINTR (see frankenphp_force_kill_thread);
182+
// a thread parked in a blocking Go/cgo call (e.g. go_ub_write to a
183+
// stalled client) can't be reached by either and may never yield.
184+
// It's tempting to bound this wait and give up on the thread
185+
// instead of blocking the whole reboot forever, but that's unsafe:
186+
// php_main() in frankenphp.c runs this same teardown on every
187+
// reboot, not just final shutdown, and calls tsrm_shutdown() as
188+
// soon as mainThread.state becomes Rebooting. tsrm_shutdown()
189+
// requires every PHP thread to have already exited - giving up on
190+
// one while its OS thread is still alive and registered in TSRM
191+
// tears down global engine state out from under a thread that's
192+
// still using it, a use-after-free confirmed by CI when this was
193+
// tried (see the discussion on php/frankenphp#2573). The safe
194+
// mitigations are reducing how often a thread ever gets stuck like
195+
// this in the first place (see WithResponseWriteTimeout and the
196+
// opcache_reset throttle), not recovering from it after the fact.
179197
thread.state.WaitFor(state.YieldingForReboot)
180198
})
181199
}

0 commit comments

Comments
 (0)