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

Maybe use signal.set_wakeup_fd on unix-likes #109

Closed
njsmith opened this issue Mar 22, 2017 · 1 comment · Fixed by #1267
Closed

Maybe use signal.set_wakeup_fd on unix-likes #109

njsmith opened this issue Mar 22, 2017 · 1 comment · Fixed by #1267

Comments

@njsmith
Copy link
Member

njsmith commented Mar 22, 2017

As of #108, we now use set_wakeup_fd to ensure that when a signal that we have a handler for is delivered, it causes our IO loop to actually wake up and run the handler ... on Windows.

On Unix, this is mostly unnecessary, but there are some edge cases where it matters:

  • When entering epoll/kqueue, there's a race condition when a signal arrives in between the last time the interpreter checks for signals and when it calls epoll_wait. This is partially a bug in CPython – on Linux it could be eliminated by masking the signal, checking for the signal, and then doing epoll_pwait. After first fixing the bug below so that threads don't break signal masking. Making this work with kqueue is more complicated, but also possible in principle (mask the signal, check for the signal, and then add a filter listening for that signal to the kqueue – but this is annoying since the user might also want to be listening for the signal, and we don't want to interfere with that).

  • If the kernel decides to deliver a signal to a non-main thread, then CPython doesn't run the handler until the main thread re-enters Python. I think this is a bug in CPython (bpo-21895), but we might still want to work around it. The only time this is known to happen in practice is that when a child thread spawns a subprocess, then on Linux the SIGCHLD gets delivered to the child thread.

Neither of these are super-urgent (one's a rare race condition, and the other only seems to happen with SIGCHLD, which we don't plan to use), but it might be nice to fix them anyway.

Using set_wakeup_fd would fix them... except that it generates spurious warning messages when the wakeup fd buffer is full, and there's no way to stop it :-(. I've added this to the list of CPython issues in #103, and filed it upstream as bpo-30050.

We could handle this by sizing our wakeup fd's buffer to be "large enough" that the errors happen "rarely enough" to be acceptable, but I'd prefer not to be wasting a bunch of non-swappable memory on an imperfect workaround for an unhelpful feature :-/. (Currently we size our wakeup fd buffer to be as small as possible to save memory, which ends up meaning 6 bytes on Linux and 1 byte on MacOS. So we'd definitely get these spurious warnings if we just started using set_wakeup_fd with no other changes.)

Given how small the impact of this is, I'm going to wait for now and see how upstream handles it before deciding on a final resolution.

@njsmith
Copy link
Member Author

njsmith commented Jan 18, 2018

python/cpython#4792 has been merged, so I think we should:

  • always use set_wakeup_fd
  • use warn_on_full_buffer=False where available
  • use large buffers everywhere else

We should also hassle PyPy about implementing warn_on_full_buffer=False (or just submit a patch)

njsmith added a commit to njsmith/trio that referenced this issue Oct 24, 2019
Also switched signal_raise to always run the C-level signal handler in
the current thread, which gives us more control over signal delivery
and lets us test the new behavior.

Fixes python-triogh-109
njsmith added a commit to njsmith/trio that referenced this issue Oct 24, 2019
Also switched signal_raise to always run the C-level signal handler in
the current thread, which gives us more control over signal delivery
and lets us test the new behavior.

Fixes python-triogh-109
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 a pull request may close this issue.

1 participant