timeout: replace the busy loop with sigtimedwait (#9099)#9123
timeout: replace the busy loop with sigtimedwait (#9099)#9123andreacorbellini wants to merge 1 commit intouutils:mainfrom
sigtimedwait (#9099)#9123Conversation
|
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.
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.
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.
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. |
|
the ci didn't pass, this is why you didn't get review, sorry! |
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!