-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
timeout: replace the busy loop with sigtimedwait (#9099)
#9123
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
base: main
Are you sure you want to change the base?
timeout: replace the busy loop with sigtimedwait (#9099)
#9123
Conversation
|
GNU testsuite comparison: |
0bba217 to
6a98042
Compare
|
GNU testsuite comparison: |
6a98042 to
7fad139
Compare
|
GNU testsuite comparison: |
|
This API appears not to be available on macOS, tho afaik the one I had suggested ( |
|
|
||
| loop { | ||
| let result = unsafe { | ||
| libc::sigtimedwait( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We honestly should be using the nix crate where possible, I'll replace this with nix's API and we should automatically gain macOS support; if not, I'll make sure to handle platform-specific cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best way is probably going to be to use self-pipe tricks with signal handlers and pass the timeout to select()... Already on it.
EDIT: It's probably even better to just use pselect with the needed signal mask, LOL. It's POSIX 2001, but everybody has implemented it because it's crucial for efficient servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We honestly should be using the
nixcrate where possible, I'll replace this withnix's API and we should automatically gain macOS support; if not, I'll make sure to handle platform-specific cases.
Agreed, but nix does not support sigtimedwait as of today.
The best way is probably going to be to use self-pipe tricks with signal handlers and pass the timeout to
select()... Already on it.
For MacOS: maybe. I do think however that sigtimedwait is the best approach on modern systems and we should stick to that when supported. Although I can understand the burden of having multiple implementations that do the same thing, so if that's a concern I guess I'm fine with dropping sigtimedwait
|
Got it fixed with select and self-pipes, I'll submit a PR after I got it tested properly. EDIT: replaced it with pselect due to the greater code complexity and slightly higher latency of self-pipes and select. |
Instead of checking for the process status in a busy loop, use
sigtimedwaitto wait for the process to exit.sigtimedwaitis a POSIX method to suspend the current process until a specified set of signals is received. The signals we're interested in are:SIGCHLD: this is sent whenever the child process quits, or is suspended (viaSIGSTOP) or is resumed (viaSIGCONT)SIGTERM: so that it can be forwarded to the child process (should this also be done forSIGINT,SIGHUP, and others? If yes, it can be added in a future PR)This PR also does the following:
cfg(unix)because currentlytimeoutonly works on Unix. For Windows support, I suggest moving the current logic in aunix.rsmodule so that we don't need to prefix each and every function withcfg(unix).Signaltype from thenixcrate wherever possible.timeout --help).sigtimedwait.Sorry for the large refactor and for addressing multiple issues in the same commit!