Skip to content

Unblock a watched signal at its last removal when the script asked for it - #6

Open
EdmondDantes wants to merge 2 commits into
bukka:io_hooks_pocfrom
true-async:signal-unblock-at-removal
Open

EdmondDantes wants to merge 2 commits into
bukka:io_hooks_pocfrom
true-async:signal-unblock-at-removal

Conversation

@EdmondDantes

Copy link
Copy Markdown

This refines bdfa5fa7a12 ("Keep a watched signal blocked under exec() and pcntl"). A signal a SignalHandle is 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, through pcntl_sigprocmask() or pcntl_signal(), the unblock was dropped, so the signal stayed blocked after the last removal:

pcntl_sigprocmask(SIG_BLOCK, [SIGUSR1]);
$w = $ctx->add(new Io\Poll\SignalHandle([SIGUSR1]), [Io\Poll\Event::Signal]);
pcntl_signal(SIGUSR1, fn () => print "handler\n");
pcntl_sigprocmask(SIG_UNBLOCK, [SIGUSR1]);
$w->remove();
posix_kill(posix_getpid(), SIGUSR1);
pcntl_signal_dispatch();

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 a SIG_UNBLOCK set, or one left out of a SIG_SETMASK set, and takes the mark back for one in a SIG_BLOCK or SIG_SETMASK set.
  • pcntl_signal() marks a watched signal when zend_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 as zend_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.
  • A signal the handle blocked itself is unblocked at the last removal as before. $old is 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_UNBLOCK during 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_UNBLOCK while 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.phpt and ..._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.phpt and ..._pcntl_signal_blocked_again.phpt: an unblock followed by a block again (SIG_UNBLOCK then SIG_BLOCK; SIG_SETMASK [] then a restore of the saved mask; pcntl_signal() then SIG_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 of ext/standard/tests/poll and ext/pcntl/tests on a debug ZTS build of this branch; an ASAN ZTS build of a core that merges this branch passes the nine tests too.

Questions:

  1. The real $old cannot 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) then pcntl_sigprocmask(SIG_SETMASK, $old) hands back a $old that 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 a SIG_BLOCK during the watch of a signal the handle blocked itself is not recorded, so the last removal still unblocks it. Both need $old to show the mask the script asked for, without the handle's block. I kept $old as the real mask to stay close to your code. Would you rather keep it that way, or have $old show the script's mask? I have a version of that too and can send it.
  2. On this branch pcntl_signal_dispatch() runs the handlers with every signal blocked and restores the saved mask with a plain sigprocmask(). A SignalHandle added 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?
  3. php_io_poll_signal_reblocked() has no caller in this branch. Does an API for extensions like that fit your plans for io_poll.h, or would you rather see it in a separate PR?

…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.
@EdmondDantes
EdmondDantes requested a review from bukka as a code owner October 9, 2026 07:58
@bukka

bukka commented Oct 10, 2026

Copy link
Copy Markdown
Owner

It looks mostly good to me. The logic makes sense. About the questions:

  1. I did a bit of checking with some examples and keeping the script's mask seems a bit more logical to me so I would probably go for that
  2. This should get addressed by Execute signal handlers before restarting syscalls php/php-src#22538 so I would leave with that.
  3. I think it's better to drop it. There is no user of it and I'm not sure that it makes that much sense. If there is ever need for it, we can add it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants