Allow SA_RESTART for SIGALRM

[nikic]

If no explicit restart_syscalls is passed, default to restart_syscalls=0 for SIGALRM only, to reduce BC impact.

Fix for bug #77335, followup to #3717.

[nikic]

@mathieuk @ralphschindler Any comments on this one? Otherwise I plan to land it on the 7.3 branch.

[ralphschindler]

Handling signals is not my strong suit, but I will do my best to understand the path forward and see what changes might need to be made in the userland software that expresses the problem we've been seeing.

@yitam assuming this hits 7.3, are there changes necessary for the MS driver or ODBC/PDO drivers necessary on the Microsoft side?

Specifically, this problem expresses itself when Laravel Horizon Jobs (this is where some of the pcntl signals/handlers are registered) are attempting to use the MS Sql Server driver for PDO (which uses an ODBC layer) to process some long running jobs.

If there are no changes in the driver layer, do changes need to happen in laravels job handling layer?
https://github.com/laravel/framework/blob/e7a14f2cc85d35d4abcb5b0acc877569ceebbd7c/src/Illuminate/Queue/Worker.php#L137-L149
and
https://github.com/laravel/framework/blob/e7a14f2cc85d35d4abcb5b0acc877569ceebbd7c/src/Illuminate/Queue/Worker.php#L514-L529

Again, I apologize for my ignorance with regards to pcntl and OS signaling, I can try out any combination of PR's and/or patches to test out if a solution works.

[mathieuk]

@ralphschindler to prevent that specific issue from happening, with this patch, all those pcntl_signal calls need a third argument $restart_syscalls=true. YMMV however as the signal handlers might not be (able to be) called depending on the kind of syscall the system was in. Refer to the previous PR and the MSSQL driver bug for more info.

@nikic I'm still on the fence on this. The change looks good, but this may enable people to write code that ends up in a stuck state. It does little to alleviate the original problem. For instance, php-lock/lock#6 may experience problems if used with SA_RESTART. If anything that should be noted in the documentation.

Maybe for a later path, we could think about extending zend_throw_exception_internal to call zend_interrupt_function when EG(vm_interrupt) is set? It seems kinda weird perhaps, but seems to me that might handle a lot of cases where a library used by an extension doesn't implement EINTR handling (but does use exceptions). In my case that would help the original problem.

[nikic]

@ralphschindler to prevent that specific issue from happening, with this patch, all those pcntl_signal calls need a third argument $restart_syscalls=true. YMMV however as the signal handlers might not be (able to be) called depending on the kind of syscall the system was in. Refer to the previous PR and the MSSQL driver bug for more info.

To be more precise, the pcntl_signal call for SIGARLM needs $restart_syscalls=true, for the others it's on by default.

@nikic I'm still on the fence on this. The change looks good, but this may enable people to write code that ends up in a stuck state. It does little to alleviate the original problem. For instance, php-lock/lock#6 may experience problems if used with SA_RESTART. If anything that should be noted in the documentation.

Yeah, me too... With this patch, people will still have to opt-in to restarting syscalls, but the cases which are currently causing issues are probably also the ones who wouldn't want to restart syscalls...

Maybe for a later path, we could think about extending zend_throw_exception_internal to call zend_interrupt_function when EG(vm_interrupt) is set? It seems kinda weird perhaps, but seems to me that might handle a lot of cases where a library used by an extension doesn't implement EINTR handling (but does use exceptions). In my case that would help the original problem.

I don't really understand this suggestion. How would this help things?

[mathieuk]

I don't really understand this suggestion. How would this help things?

I see how that is a bit of a confusing suggestion.

It seems to me that this would provide the developer with the expected (or to me: preferred) developer experience: your signal handler is called when SIGALRM is raised , even (or especially) when execution got interrupted during I/O to deliver a signal in a library that doesn't handle EINTR and (potentially) throws an exception based on that (like the PDO+MSSQL driver, and probably others).

To sketch out my use-case: I'm working with Laravel worker daemons. These workers perform long running jobs that involves various heavy queries. It might get interrupted by SIGALRM (and I want that) but I have to do some housekeeping after it gets interrupted. Right now I have to catch the right exceptions the MSSQL driver throws and act accordingly. It works but it's a bit hacky and fragile.

What I'd like to have is actually have my signal handler called so that I can really know I hit the timeout and act accordingly.

Now, with SA_RESTART I may or may not be able to do that because it'd hop right back into the syscall that got interrupted before PHP can do anything and it may hang for an indeterminate amount of time. The MSSQL driver would be in a read() call, the signal handler gets called, it'd register the userland handler to be called and hand control back which would restart the syscall.

But, if I'd leave out SA_RESTART, that driver would error out. Again, right now that causes an (connection is broken) exception the instant control is handed back. With my suggestion, the exception handling code would see if there was an interrupt scheduled and handle that first and my signal handler (read: the zend_interrupt_function) gets called before that exception gets actually thrown. That allows me to handle the SIGALRM as I expected it to work.

It's a bit specific but it seems to me this would make pcntl+SIGALRM work as expected for these use-cases.

[krakjoe]

@nikic please can you wrap up this one and the related #3717 ...

I'm not really able to determine the consensus here ... if it still looks good to you 9 months later then ship it, probably ...

[nikic]

Merged as e98e1f9 into 7.4. This does not really fix the problem people were having originally, but it's still a change we should do, because PHP has no business overriding what the user explicitly specified in the call...