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

Catch signals on Linux with signalfd (follow-up to #98 and #132) #142

Closed
wants to merge 1 commit into from

Conversation

edgar-bonet
Copy link
Collaborator

@edgar-bonet edgar-bonet commented Dec 1, 2023

As noticed by @hartwork, pselect is not a reliable way of catching signals on Linux: in case of high input pressure, signal delivery is starved. Pull request #132 partially fixed the issue by issuing two consecutive pselect calls, but this left a race window between these two calls.

This pull request fixes the issue without race conditions by using signalfd(), which makes signal delivery reliable. The call is only issued on Linux, as both the pselect() issue and signalfd() are Linux-specific.

This is one of two possible alternatives, the other one being the self-pipe trick (#143).

On Linux, pselect() does not reliably catch signals when there is high
pressure on the watched file descriptors. This was mostly fixed in
commit e94cecd by calling pselect()
twice. This solution, however, leaves a race window between the two
calls.

Fix the pselect() issue without races by using signalfd(). This makes
signal delivery reliable. The call is only issued on Linux, as both the
pselect() issue and signalfd() are Linux-specific.
@hartwork hartwork changed the title Catch signals on Linux with signalfd Catch signals on Linux with signalfd (follow-up to #98 and #132) Dec 1, 2023
Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edgar-bonet on high level, I prefer #143 over this because it:

  • has a lower chance of breaking support for BSD and/or Linux and/or other Unix through future changes because everyone is running the same code
  • the control flow is simpler or rather there is only one flow rather than two in some sense

(On a low/detail level, two of my comments on #143 also apply here, but we can save those comments and changes if the plan is to get #143 ready to merge.)

Do you happen to have a preference for #142 here over the self-pipe trick, is there some benefit that I may be missing?

@hartwork
Copy link
Collaborator

hartwork commented Dec 1, 2023

@edgar-bonet PS: nice demo, by the way 👍

@edgar-bonet
Copy link
Collaborator Author

@hartwork: This approach is indeed problematic from the point of view of testing. One trick I used is to #undef __linux__ at the top, which lets me exercise the BSD code paths on Linux: as expected, it works, except for signals being missed under high stdin pressure.

I do also prefer the self-pipe approach: less code, and conceptually simpler as all events (signals, keystrokes and data) are caught in the exact same way: as “file descriptor ready” events. Closing this PR. I will work on #143, later, on the basis of your review.

@edgar-bonet edgar-bonet closed this Dec 2, 2023
@edgar-bonet edgar-bonet deleted the signalfd branch December 2, 2023 21:11
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