Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restartSysCalls is not defined #1251

Closed
gmponos opened this issue Dec 10, 2018 · 4 comments
Closed

restartSysCalls is not defined #1251

gmponos opened this issue Dec 10, 2018 · 4 comments
Labels
Milestone

Comments

@gmponos
Copy link
Contributor

gmponos commented Dec 10, 2018

On this line https://github.com/Seldaek/monolog/blob/master/src/Monolog/SignalHandler.php#L96 the restartSyscalls is never defined.. I could not understand what the intention was in order to fix it 😞

@Seldaek
Copy link
Owner

Seldaek commented Dec 11, 2018

Pretty sure it's a typo and should be $this->signalRestartSyscalls as that one exists and is unused.

@Seldaek
Copy link
Owner

Seldaek commented Dec 11, 2018

Ping @RGustBardon though - also this should be applied on 1.x AFAIK

@Seldaek Seldaek added this to the 1.x milestone Dec 11, 2018
@Seldaek Seldaek added the Bug label Dec 11, 2018
@RGustBardon
Copy link
Contributor

@gmponos: Thank you for reporting the bug! As @Seldaek suggested, it should be $this->signalRestartHandlers.

I could not understand what the intention was in order to fix it

Thank you for attempting to come up with a fix! The idea is as follows:

When registering a signal handler, the user can require calling the signal handler previously registered for this signal after the logging of the signal is done. This is what the $callPrevious parameter of the registerSignalHandler method is responsible for. Previous signal handlers are stored in the previousSignalHandler property.

Another parameter of the registerSignalHandler method ($restartSyscalls) controls whether system calls should be restarted upon a signal (cf. usleep in SignalHandlerTest). These preferences are stored in the signalRestartSyscalls property.

Before PHP 7.1, it was not possible to get the current signal handler. Since PHP 7.1, pcntl_signal_get_handler returns either SIG_DFL (the default system handler for a given signal), SIG_IGN (ignore the signal), or a function (previously registered using pcntl_signal).

If the user wishes to call the previous signal handler after the logging takes place and pcntl_signal_get_handler returns a function, this can be done in a straightforward matter: a call to the previous handler will suffice. This is what happens at the very end of handleSignal. However, in cases the previous signal handler is unknown, handleSignal attempts to call the default signal handler (SIG_DFL) once it is done logging.

Right before making such an attempt, the current signal handler is still the one used for logging. For this reason, the signal handler is temporarily substituted with the default one (the first call to the pcntl_signal function at the end of handleSignal), the process sends the signal to itself (posix_kill), it dispatches the newly installed signal handler (pcntl_signal_dispatch), and finally restores the Monolog signal handler (the second call to the pcntl_signal function).

The current units tests have not been able to identify the problem as they never call registerSignalHandler with both $callPrevious and $restartSyscalls set to true.

@Seldaek: Thank you for bringing the bug to my attention! I have created two pull requests with the same fix (#1252 for the master branch and #1253 for the 1.x branch).

Seldaek added a commit that referenced this issue Dec 11, 2018
Seldaek added a commit that referenced this issue Dec 11, 2018
…ster

Fix the property for restarting syscalls (#1251)
@Seldaek
Copy link
Owner

Seldaek commented Dec 11, 2018

Great, thanks for the fixes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants