Skip to content
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

Attempt to kill bg process in tests #3894

Merged
merged 1 commit into from
Oct 7, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions crates/tests/src/e2e/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -873,16 +873,29 @@ impl Display for NamadaCmd {

/// A command under test running on a background thread
pub struct NamadaBgCmd {
join_handle: std::thread::JoinHandle<NamadaCmd>,
abort_send: std::sync::mpsc::Sender<()>,
// Option workaround to allow moving the handle out of this object and
// still implement Drop
join_handle: Option<std::thread::JoinHandle<Option<NamadaCmd>>>,
abort_send: std::sync::mpsc::Sender<ControlCode>,
}

impl Drop for NamadaBgCmd {
fn drop(&mut self) {
let _ = self.abort_send.send(ControlCode::EndOfText);
}
}

impl NamadaBgCmd {
/// Re-gain control of a background command (created with
/// [`NamadaCmd::background()`]) to check its output.
pub fn foreground(self) -> NamadaCmd {
self.abort_send.send(()).unwrap();
self.join_handle.join().unwrap()
pub fn foreground(mut self) -> NamadaCmd {
self.abort_send.send(ControlCode::Enquiry).unwrap();
self.join_handle
.take()
.expect("Background task should always be present")
.join()
.unwrap()
.expect("Background task has been dropped")
}
}

Expand All @@ -897,17 +910,22 @@ impl NamadaCmd {
let mut cmd = self;
loop {
match abort_recv.try_recv() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this PR, but this try_recv might be contributing substantially to the resource usage of e2e tests :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the quick fix would be to call recv_timeout instead with a sane timeout. Probably a better solution would be to switch to async channels?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think async channels wouldn't help. there is no event here triggering a new channel read, so the only reasonable thing would be what you suggested first: a poll with a timer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I've opened #3897 to track that, I'll merge this one, thank you!

Ok(())
Ok(ControlCode::EndOfText) => {
// Terminate the background task by dropping the
// corresponding NamadaCmd
return None;
}
Ok(ControlCode::Enquiry)
| Err(std::sync::mpsc::TryRecvError::Disconnected) => {
return cmd;
return Some(cmd);
}
Err(std::sync::mpsc::TryRecvError::Empty) => {}
Ok(_) | Err(std::sync::mpsc::TryRecvError::Empty) => {}
}
cmd.session.is_matched(Eof).unwrap();
}
});
NamadaBgCmd {
join_handle,
join_handle: Some(join_handle),
abort_send,
}
}
Expand Down
Loading