ext/pcntl: do not drop queued signals when an exception is pending

[nicolas-grekas]

pcntl_signal_dispatch() detaches the whole queue from PCNTL_G(head) before it starts calling handlers, and recycles every entry it walks over. Two paths let queued signals disappear.

1. Dispatching while an exception is pending

ZEND_VM_FCALL_INTERRUPT_CHECK() runs right after an internal function returns, before the pending exception is handled. So when an internal function throws, pcntl_interrupt_function() reaches the dispatcher with EG(exception) set. There, zend_call_function() returns without calling anything, the if (EG(exception)) break; added by 296fad1 fires on the very first entry, and the /* drain the remaining */ loop recycles the entire queue. No handler ever runs, and nothing is left for a later pcntl_signal_dispatch() either.

Any signal that arrives while such a function is running is therefore lost, not delayed. The fix sets the exception aside for the duration of the dispatch, the way zend_objects_destroy_object() does around destructors called during unwinding. zend_exception_save() is not usable for that: it goes through the single EG(prev_exception) slot, so a dispatch happening inside an autoloader called with an exception set aside would hand that exception back too early.

2. Signals queued behind a throwing handler

If a handler throws, the signals queued behind it were recycled as well. They now go back to the queue, and with asynchronous signals the interrupt is re-armed, so the engine delivers them on its own once the exception is handled rather than waiting for another signal to come in. The existing behaviour of not calling further handlers while the exception propagates is unchanged, and pcntl_signal_dispatch_exception.phpt still passes as is.

How I ran into it

Any long blocking internal call that throws on timeout hits case 1. pecl/amqp is one: AMQPQueue::consume() throws AMQPQueueException("Consumer timeout exceed") when the connection read timeout expires. A Symfony Messenger worker built on it (symfony/symfony#65920) misses every SIGTERM, whatever the timeout is, because the worker spends essentially all of its wall time inside that call:

read_timeout=0.20s, SIGTERM at 1.70s: 31 rounds, never stopped
read_timeout=0.05s, SIGTERM at 2.13s: 121 rounds, never stopped
read_timeout=0.01s, SIGTERM at 2.57s: 653 rounds, never stopped

653 iterations of a 10 ms call and the signal is never delivered, not even by an explicit pcntl_signal_dispatch() afterwards. SigBlk, SigIgn and SigCgt read from inside the extension's callback confirm the signal is neither blocked nor ignored and that the handler is installed, and the same signal during sleep(), stream_select(), stream_socket_accept() or socket_read() is delivered normally. What makes consume() different is only that it throws: when a message arrives and it returns normally instead, the signal is delivered.

With this patch, same build, same test, no userland workaround:

read_timeout=0.20s, SIGTERM at 1.70s: stopped, latency 0.094s
read_timeout=0.20s, SIGTERM at 2.31s: stopped, latency 0.083s
read_timeout=0.20s, SIGTERM at 3.05s: stopped, latency 0.140s

I verified this against pecl/amqp 2.2.0 built on this branch, with and without the patch. Case 2 is covered by the two added .phpts, one with an explicit second dispatch and one where the engine has to come back on its own; case 1 needs an internal function that blocks long enough for a signal to arrive and then throws, which I could not build out of core functions alone, since re-entering the VM for any userland callback dispatches the signal first.

ext/pcntl, Zend/tests and ext/standard/tests are green (8180 passed, 0 failed).

[devnexen]

Thanks I ll have a look some time in the following days, I would say tough after a quick look there are incoming changes needed.

[TimWolla]

Is #22538 related?

[nicolas-grekas]

@TimWolla Related in that #22538 rewrites the same function, but it neither causes nor fixes this, and it does not help the case that made me look. I built it (master, 8.6.0-dev) with pecl/amqp 2.2.0 on top (plus php-amqp#638 to get that compiling) and ran the same worker loop:

read_timeout=0.20s, SIGTERM at 1.70s: 31 rounds, never stopped
read_timeout=0.20s, SIGTERM at 2.31s: 31 rounds, never stopped
control on that build: stream_socket_accept / stream_select / sleep all interrupted at 0.50s

Two reasons. #22538 runs handlers on EINTR inside php_sockop_*, but librabbitmq owns that socket and retries EINTR itself, so PHP never sees it and the only way out of consume() is still the read timeout, which throws. And the rewritten dispatcher keeps both losses: call_user_function() is still called with EG(exception) possibly set (so it is a no-op), if (EG(exception)) { interrupt = true; break; } then fires on the first entry, and /* drain the remaining in case of exception thrown */ if (EG(exception)) { while (pcntl_signal_dequeue(&sig)) {} } empties the ring buffer. The second loss arguably matters more there, since #22538 makes "a handler throws" a supported way to interrupt a syscall: whatever is queued behind that handler is then silently gone.

So whichever lands first, the other needs the same two guards, setting the exception aside while handlers run and keeping what a throwing handler leaves behind. I can port them onto #22538 once it settles.

@devnexen Thanks. Meanwhile I found a problem in my own change and reworked it in 17ff74f: zend_exception_save() goes through the single EG(prev_exception) slot, so a dispatch happening inside an autoloader called with an exception set aside would have handed that exception back too early. It now uses a local, the way zend_objects_destroy_object() does. The other change is that when a throwing handler leaves signals behind, the interrupt is re-armed so the engine delivers them on its own once the exception is handled, rather than waiting for another signal; that turned out to be deterministic to test, hence pcntl_signal_dispatch_exception_3.phpt. If what you saw is something else, a pointer would help.