-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
fix(cli): remove possible deadlock in test channel #22662
Conversation
let Some(stream) = &mut self.read_opt else { | ||
return; |
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.
This could lead to starvation as we just quietly returned "ready" and tokio could select us again.
cli/tools/test/channel.rs
Outdated
@@ -253,7 +265,7 @@ impl TestEventSenderFactory { | |||
loop { | |||
tokio::select! { | |||
_ = test_stdout.pipe(), if test_stdout.is_alive() => {}, | |||
_ = test_stderr.pipe(), if test_stdout.is_alive() => {}, | |||
_ = test_stderr.pipe(), if test_stderr.is_alive() => {}, |
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.
This typo here meant that we were polling test_stderr in a hot loop if it finished, never getting to the sync receiver
@@ -377,7 +389,9 @@ impl TestEventSender { | |||
let mutex = parking_lot::RawMutex::INIT; | |||
mutex.lock(); | |||
self.sync_sender.send(SendMutex(&mutex as _))?; | |||
mutex.lock(); | |||
if !mutex.try_lock_for(Duration::from_secs(30)) { |
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.
Just in case
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.
Really clever solution, LGTM!
The stderr stream could possibly starve the other bits of the output-redirecting event loop. Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>
The stderr stream could possibly starve the other bits of the output-redirecting event loop.