Execute signal handlers before restarting syscalls

[arnaud-lb]

This PoCs the ideas discussed in #5521:

First and foremost, the whole concept of "restart syscalls" IMHO doesn't really work in PHP right now. The idea behind SA_RESTART is that the signal handler gets executed and then the syscall is restarted. However, because in PHP (at least as far as pcntl signals are concerned) the signal handler only sets a flag and the actual PHP-level callback is only executed on VM reentry, this means that "restart syscalls" really means "delay syscalls until the end of I/O operations", which is pretty useless. To use something like SIGALARM this effectively means that you need to disable syscall restarting, which also means that you need to handle this all over the place, which most code is not prepared to do.

The correct approach to handle that is (as the PEP outlines) disable SA_RESTART and manually implement restart on EINTR, but with an explicit call to the interrupt handler. This implements how C-level SA_RESTART behaves in PHP.

Syscalls are interrupted when:

• Any signal handler returns SignalResult::Interrupt
• Any signal handler returns SignalResult::Default or no value, and was registered with $restart_syscall=false
• Any signal handler throws an exception
• EG(timed_out) is set

Otherwise, syscalls are restarted.

Implemented only in php_sockop_* for now, but the goal would be to implement this for all syscalls eventually, and at least for existing EINTR loops. (Edit: Since we are clearing SA_RESTART now, syscalls that would be restarted by the OS before are not restarted anymore. So we may have to implement restarting for all syscalls immediately.)

API changes:

New enum:

namespace Pcntl;

enum SignalReturn {
    case Default;   /* Use pcntl_signal()'s $restart_syscall */
    case Interrupt; /* Interrupt the syscall */
};

pcntl_signal() changes: The $handler function must now return a SignalReturn, NULL, or no value. Before, the return value was ignored.

Some considerations:

• Some signal handlers may already be pending before performing a syscall, so we have to check the queue before that
• Operations with a timeout should use a deadline, as restarting will extend the total time
• Syscalls executed by a signal handler may need to execute other signal handlers, if new signals are delivered

Exceptions vs return value:

Returning a value is more PHP-ish than throwing an exception, but exceptions have some benefits in this context:

• Returning SignalResult::Interrupt from a handler does nothing outside of syscalls
• Exceptions will propagate to parent signal handlers by default (see above)

Reentrancy:

Running signal handlers on EINTR increases the risk of reentrancy-related bugs.

For streams this is solved by preventing usage of the interrupted stream during signal handler execution (see PHP_STREAM_FLAG_IN_USE).

When implementing EINTR support on non-stream-related system calls, the surrounding code will need to be made reentrant or to implement a similar mechanism.

Nested dispatch:

Unbounded nested dispatch can cause stack growth during in the event of a signal storm.

Possible roadmap:

• Merge this in master after 8.6 is branched
• Implement EINTR handling for remaining all syscalls incrementally

BC breaks:

• Returning anything but NULL or SignalResult from a signal handler will throw a TypeError

Demo:

$pair = @stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, 0);
[$read, $write] = $pair;

pcntl_signal(SIGINT, function () {
    echo "received SIGINT (Ctrl-C)\n";
    return Pcntl\SignalReturn::Interrupt;
});

var_dump(fread($read, 1));

echo "Done\n";

If a SIGINT (Ctrl-C) is delivered during the fread() call, the signal handler is executed immediately to decide whether the syscall should be restarted or not. In this case the handler decides to not restart, so the fread() call returns.

Previously, the signal would be queued and fread() would hang indefinitely.

TODO:

• Use a deadline
  • Use a monotonic clock
• Allow nested signal delivery
• Restart handling in main/streams/xp_socket.c
• Restart handling for remaining syscalls

[bwoebi]

This is a pretty reasonable proposal and I will support this. Thank you for picking this up.