Skip to content

Conversation

@andreacorbellini
Copy link
Contributor

Instead of checking for the process status in a busy loop, use sigtimedwait to wait for the process to exit.

sigtimedwait is 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 (via SIGSTOP) or is resumed (via SIGCONT)
  • SIGTERM: so that it can be forwarded to the child process (should this also be done for SIGINT, SIGHUP, and others? If yes, it can be added in a future PR)

This PR also does the following:

  • Removes all uses of cfg(unix) because currently timeout only works on Unix. For Windows support, I suggest moving the current logic in a unix.rs module so that we don't need to prefix each and every function with cfg(unix).
  • Replaces integer signals with the Signal type from the nix crate wherever possible.
  • Resolves all the TODO items related to exit statuses (exit codes were based on the output of GNU's timeout --help).
  • Removes the SIGTERM signal handler: now everything is caught via sigtimedwait.

Sorry for the large refactor and for addressing multiple issues in the same commit!

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout-large-parameters. tests/timeout/timeout-large-parameters is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/timeout/timeout-parameters. tests/timeout/timeout-parameters is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@andreacorbellini andreacorbellini force-pushed the timeout-sigtimedwait branch 2 times, most recently from 0bba217 to 6a98042 Compare November 3, 2025 06:19
@github-actions
Copy link

github-actions bot commented Nov 3, 2025

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout-parameters. tests/timeout/timeout-parameters is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@Alonely0
Copy link
Contributor

Alonely0 commented Nov 3, 2025

This API appears not to be available on macOS, tho afaik the one I had suggested (setitimer, iirc) does. Also, there are multiple ones on Linux, with different time resolutions (seconds/microseconds/nanoseconds iirc). We should use the one with the greatest resolution on Linux, which iirc is timer_create(2); on macOS use the other one that's available, iirc setitimer(2); and probably also add a alarm(2) as a fallback, because this is the lowest common denominator in POSIX, which takes time in seconds, so we would have to convert it to seconds and take the nearest positive integer.


loop {
let result = unsafe {
libc::sigtimedwait(
Copy link
Contributor

@Alonely0 Alonely0 Nov 5, 2025

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.

Copy link
Contributor

@Alonely0 Alonely0 Nov 5, 2025

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.

Copy link
Contributor Author

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.

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

@Alonely0
Copy link
Contributor

Alonely0 commented Nov 5, 2025

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.

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