search

php-runtime / runtime

A home for runtimes.
MIT License
402 stars 32 forks source link

Change SIGKILL to SIGHUP #136

Closed ash-m closed 1 year ago

ash-m commented 1 year ago

Fix for:

Fatal error: Error installing signal handler for 9 in /app/vendor/react/event-loop/src/StreamSelectLoop.php on line 161

From https://www.php.net/manual/en/function.pcntl-signal.php#83981 (14 years old):

You cannot assign a signal handler for SIGKILL (kill -9).

ash-m commented 1 year ago

any idea on if this can be merged?

alexander-schranz commented 1 year ago

Looks fine from my side as this was implemented by @brettmc. Maybe he can also confirm this changes be fine.

andrew-demb commented 1 year ago

This looks wrong. sigterm is already stronger than sighup. the point of the sigkill is to send a stronger signal than the sigterm if the sigterm didn't work.

Please check https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html

Seems like handling SIGKILL is impossible from PHP

GrahamCampbell commented 1 year ago

That is intentional. The sigkill does not allow handling, and is only sent if a graceful stop does not succeed.

brettmc commented 1 year ago

I don't think changing it to SIGHUP is correct, as that's not (usually) a terminating signal (I've often seen it used as a "restart" or "reload config" signal). But I think it's probably correct that SIGKILL is not worth catching, since the process should be terminated in such a way that the application doesn't get a say in it. That's certainly true in docker/k8s, but I wasn't sure that it was always true. If SIGKILL is not compatible with StreamSelectLoop (I only tested against libev), then I don't see any harm in removing entirely.

alexander-schranz commented 1 year ago

just removing currently the SIGKILL. Thx all for the feedback and @ash-m for the PR. If somebody has other suggestion let me know and feel free to create a pull request.