timeout: Implemention for windows#13363
Conversation
nikolalukovic
changed the title
|
GNU testsuite comparison: |
|
how nicely does this play with #13237 ? |
nothing in unix world was changed, just shuffled in diff files, when your PR gets merged I'll just rebase |
|
Nice then. Quite happy we're getting timeout on Windows land. Let me get this reviewed soon-ish. |
Unable to generate the performance reportThere was an internal error while processing the run's data. We're working on fixing the issue. Feel free to contact us on Discord or at support@codspeed.io if the issue persists. |
Alonely0
left a comment
Alonely0
left a comment
There was a problem hiding this comment.
This looks super nice, so lots of thanks for the work! If my other PR lands first, we should be able to enable the benchmarks on *nix as well, and I would hope the rebase/merge isn't too bad; it mostly seems that src/uu/timeout/src/platform/unix.rs would get significantly shrunk (since the logic is moven to uucore), and we'd remove the SIGNALED logic from timeout.rs altogether.
This is a lot of code, so I'll read it with fresh eyes again tomorrow or this week. Also btw, thanks for the new benches!
| ] | ||
| # "feat_os_windows" == set of utilities which can be built/run on modern windows platforms | ||
| feat_os_windows = ["feat_Tier1", "stdbuf"] | ||
| feat_os_windows = ["feat_Tier1", "stdbuf", "timeout"] |
There was a problem hiding this comment.
Not sure if we should instead move timeout to tier 1 if we get Windows support.
cc @sylvestre
| /// Overwrites any creation flags set earlier via `CommandExt::creation_flags`. | ||
| pub fn configure_process_group(cmd: &mut Command) { | ||
| use std::os::windows::process::CommandExt; | ||
| cmd.creation_flags(CREATE_NEW_PROCESS_GROUP); |
There was a problem hiding this comment.
Not thrilled about forcing an overwrite, but it seems there's no way around it...
| TimedOut, | ||
| } | ||
|
|
||
| fn cvt(result: BOOL) -> io::Result<()> { |
There was a problem hiding this comment.
I would appreciate a small doc comment.
| /// polling) shows up as a step change across the arguments. | ||
| #[cfg(windows)] | ||
| #[divan::bench(args = ["0.001", "0.005", "0.02"])] | ||
| fn timer_expiry_latency(bencher: Bencher, duration: &str) { |
There was a problem hiding this comment.
Something we should enable on unix after a certain PR lands ;)
| /// per-process signalling). Replaced on every spawn so that repeated | ||
| /// in-process invocations of `uumain` (benchmarks, fuzzing) never signal a | ||
| /// previous run's job. | ||
| static JOB: Mutex<Option<Job>> = Mutex::new(None); |
There was a problem hiding this comment.
I think this global could be removed by making post_spawn() return an Option<Job> on Windows, then pass it as an argument to platform::send_signal() as well. I know it's not super ideal to have platform-dependent signatures, but I would argue it's much less bug-prone than global state.
| /// runs; intentionally lives for the rest of the process. | ||
| static WAKE_EVENT: OnceLock<OwnedHandle> = OnceLock::new(); | ||
| /// POSIX signal number of the last console control event received (0 = none). | ||
| static LAST_CTRL_SIGNAL: AtomicI32 = AtomicI32::new(0); |
There was a problem hiding this comment.
Note that LAST_CTRL_SIGNAL is not reset when we handle it, so if you send a signal, timeout thinks it has already sent termination. I can reproduce with a small program that ignores ConsoleCtrlHandler; when ctrl+cing it at leas once, this results in non-termination when it times out.
| /// Manual-reset event signalled by the console control handler to wake | ||
| /// [`ChildExt::wait_or_timeout`]. Unset until [`enable_ctrl_forwarding`] | ||
| /// runs; intentionally lives for the rest of the process. | ||
| static WAKE_EVENT: OnceLock<OwnedHandle> = OnceLock::new(); |
There was a problem hiding this comment.
WAKE_EVENT seems to also never be reset at the end, which doesn't matter because we only wait for one program, but I think it would be appreciated by anyone who works in this codebase in the future that it is reset once we're done with it.
3 participants
Attempts to fix #6200