-
Notifications
You must be signed in to change notification settings - Fork 1
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
Cleanup when terminated by signal #11
Conversation
76d5a8d
to
804b203
Compare
c61e441
to
59333e5
Compare
cbfca5a
to
d182b25
Compare
d182b25
to
bbcdf62
Compare
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.
lgtm
@@ -258,4 +320,39 @@ mod tests { | |||
// ...and end the event loop. | |||
timeout(ONE_SEC, join_handle).await.unwrap().unwrap(); | |||
} | |||
|
|||
#[cfg(target_family = "unix")] |
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.
can we test windows?
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.
Good question! The hassle wasn't worth it to tokio, who just tests that their own plumbing is working and trusts that Win32 is going to do the right thing: https://github.com/tokio-rs/tokio/blob/d51f16855bce90c3c73ae199c7d6bdb340297e99/tokio/src/signal/windows/sys.rs#L159-L164 (in our case, testing the code paths on Unix is a similar test of our own plumbing)
However, if we're using tokio's signal-handling but not testing Windows, and tokio's implementing the signal-handling but not testing Windows, then who's flying the plane? So to answer the question of "can we test windows," I do believe such a thing is within the realm of possibility: https://learn.microsoft.com/en-us/windows/console/generateconsolectrlevent exists, but unless we somehow put the unit test into its own process group, this would send the event to every process sharing the console -- including the test runner itself, despite the test case itself passing: https://github.com/oxidecomputer/thouart/actions/runs/8369557404/job/22915498686#step:6:71
cargo-test
doesn't seem to set a process group for its invocation of the test binary. But nextest
has this TODO that should be pretty straightforward to fill in, so I think I'll pursue that route: nextest-rs/nextest@main...lifning:nextest:win32-process-group (as luck would have it, we happen to know the maintainer)
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.
"Let's fire up a windows VM and see if it works lol lmao" is fine by me. We've probably sufficiently stared into this abyss.
Rel. oxidecomputer/oxide.rs#536
Footnotes
which sure are a pain in the neck to send to an arbitrary PID compared to Unix! ↩