Use set_wakeup_fd to react to control-C on Windows #108
Conversation
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
As compared to the CFFI-based code that this replaces, it's (a) less race-y (the Python C-level signal handler writes to the wakeup_fd *after* setting the flag to run the Python-level signal handler, whereas our old code ran before), (b) avoids arcane CFFI stuff that might be broken in the presence of subinterpreters, (c) slightly less code. The downside of set_wakeup_fd is that writing to the wakeup socket fails with EWOULDBLOCK, then in our context the correct thing to do is to ignore that error (we just want to guarantee a wakeup, and if the send buffer is full then we are definitely going to wake up), but the hardcoded behavior in signalmodule.c is to print a spurious warning to stderr. IMO this is makes it useless to us on Unix-likes, because they get signals all the time, and we configure out wakeup socket to use a tiny send buffer to save on kernel memory. But on Windows the trade-offs are different: Windows insists on using a gargantuan send buffer (see comments in _wakeup_socketpair.py), and the only signal is control-C. It's not that big a deal if someone gets a spurious error message in an extremely rare case when responding to an explicit control-C. (Though it still would be nicer to not have this error message...) See python-triogh-42 for more details.
Codecov Report
@@ Coverage Diff @@
## master #108 +/- ##
==========================================
+ Coverage 98.24% 98.28% +0.03%
==========================================
Files 50 50
Lines 5878 5875 -3
Branches 464 464
==========================================
- Hits 5775 5774 -1
+ Misses 88 86 -2
Partials 15 15
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
As compared to the CFFI-based code that this replaces, it's (a) less
race-y (the Python C-level signal handler writes to the
wakeup_fd after setting the flag to run the Python-level signal
handler, whereas our old code ran before), (b) avoids arcane CFFI
stuff that might be broken in the presence of subinterpreters, (c)
slightly less code.
The downside of set_wakeup_fd is that writing to the wakeup socket
fails with EWOULDBLOCK, then in our context the correct thing to do is
to ignore that error (we just want to guarantee a wakeup, and if the
send buffer is full then we are definitely going to wake up), but the
hardcoded behavior in signalmodule.c is to print a spurious warning to
stderr. IMO this is makes it useless to us on Unix-likes, because they
get signals all the time, and we configure out wakeup socket to use a
tiny send buffer to save on kernel memory. But on Windows the
trade-offs are different: Windows insists on using a gargantuan send
buffer (see comments in _wakeup_socketpair.py), and the only signal is
control-C. It's not that big a deal if someone gets a spurious error
message in an extremely rare case when responding to an explicit
control-C. (Though it still would be nicer to not have this error
message...)
See gh-42 for more details.