Repository navigation
Unblock a watched signal at its last removal when the script asked for it - #6
Open
EdmondDantes wants to merge 2 commits into
Open
EdmondDantes wants to merge 2 commits into
EdmondDantes wants to merge 2 commits into
Conversation
…r it pcntl_sigprocmask() and pcntl_signal() keep a watched signal blocked, but dropped the script's unblock of a signal it had blocked before the watch: after the last removal the signal stayed blocked. The unblock is now recorded and done at the last removal, and a later block by the script takes it back. php_io_poll_signal_reblocked() records an unblock by code outside pcntl, which no request of the script takes back.
Each case of poll_signal_handle_script_unblock.phpt is now its own test, so a failure names the case and no case runs on the mask another one left.
Owner
|
It looks mostly good to me. The logic makes sense. About the questions:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This refines
bdfa5fa7a12("Keep a watched signal blocked under exec() and pcntl"). A signal aSignalHandleis watched for stays blocked, and that part is unchanged. But when the script had the signal blocked before the watch and unblocked it during the watch, throughpcntl_sigprocmask()orpcntl_signal(), the unblock was dropped, so the signal stayed blocked after the last removal:Expected:
handler. Actual: nothing; SIGUSR1 stays blocked and pending.The fix records the unblock and leaves it for the last removal:
php_io_poll_signal_unblock_at_removal()(ext/standard/io_poll.c) marks a watched signal the script had blocked before the watch to be unblocked at the last removal.php_io_poll_signal_keep_blocked_at_removal()takes that mark back when the script blocks the signal again during the watch, so a script that unblocks and then blocks again ends with the signal blocked, as before this change.pcntl_sigprocmask()marks a watched signal in aSIG_UNBLOCKset, or one left out of aSIG_SETMASKset, and takes the mark back for one in aSIG_BLOCKorSIG_SETMASKset.pcntl_signal()marks a watched signal whenzend_sigaction()unblocked it before pcntl blocked it again.php_io_poll_signal_reblocked()is for code outside pcntl that blocks a watched signal again after something else unblocked it, such aszend_sigaction()called by another extension. The last removal unblocks such a signal, and a later block by the script does not take that back. TrueAsync's extension calls it before each poll of its reactor, in place of its own record of such signals, which unblocked a signal at the end of its watch even while another handle still watched it. Nothing in this branch calls it, and no test here covers it; the extension's tests do.$oldis still the real mask, and the existing tests are unchanged.Tests, one case each, in
ext/standard/tests/poll/:poll_signal_handle_script_unblock_sig_unblock.phpt:SIG_UNBLOCKduring the watch.poll_signal_handle_script_unblock_setmask_without_it.phpt:SIG_SETMASK []during the watch.poll_signal_handle_script_unblock_two_handles.phpt:SIG_UNBLOCKwhile two handles watch the signal; it stays blocked until the second removal.poll_signal_handle_script_unblock_pcntl_signal.phpt:pcntl_signal()during the watch of a blocked signal.poll_signal_handle_script_unblock_saved_mask_restored.phptand..._saved_mask_restored_unblocked_before.phpt: a mask saved and restored during the watch, with the signal blocked before the watch and without.poll_signal_handle_script_unblock_blocked_again.phpt,..._cleared_mask_restored.phptand..._pcntl_signal_blocked_again.phpt: an unblock followed by a block again (SIG_UNBLOCKthenSIG_BLOCK;SIG_SETMASK []then a restore of the saved mask;pcntl_signal()thenSIG_BLOCK).The two
pcntl_signal()tests skip without Zend signals, where installing a handler does not unblock.Without the fix the first four fail: the signal stays blocked, and in the
pcntl_signal()test the handler never runs. The other five pass without the fix too; they check that the fix keeps those cases as they were. With the fix all nine pass, and so do the rest ofext/standard/tests/pollandext/pcntl/testson a debug ZTS build of this branch; an ASAN ZTS build of a core that merges this branch passes the nine tests too.Questions:
$oldcannot tell a save-and-restore from a request to keep the signal blocked. After an unblock during the watch,pcntl_sigprocmask(SIG_BLOCK, [SIGUSR2], $old)thenpcntl_sigprocmask(SIG_SETMASK, $old)hands back a$oldthat contains the watched signal, because the handle blocked it. The restore takes the unblock back and the signal stays blocked, as before this change. And aSIG_BLOCKduring the watch of a signal the handle blocked itself is not recorded, so the last removal still unblocks it. Both need$oldto show the mask the script asked for, without the handle's block. I kept$oldas the real mask to stay close to your code. Would you rather keep it that way, or have$oldshow the script's mask? I have a version of that too and can send it.pcntl_signal_dispatch()runs the handlers with every signal blocked and restores the saved mask with a plainsigprocmask(). ASignalHandleadded inside a handler finds its signal blocked, so the restore unblocks it while it is still watched. My pcntl_signal_dispatch() undoing the signal mask changes of its handlers php/php-src#24187 runs the handlers under the mask from before the dispatch, which removes this case. Is it fine to leave this to pcntl_signal_dispatch() undoing the signal mask changes of its handlers php/php-src#24187, or do you want the dispatch to go through the same record here?php_io_poll_signal_reblocked()has no caller in this branch. Does an API for extensions like that fit your plans forio_poll.h, or would you rather see it in a separate PR?