-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
rt: fix blocking pool shutdown logic #1978
Conversation
The blocking task queue was not explicitly drained as part of the blocking pool shutdown logic. It was originally assumed that the contents of the queue would be dropped when the blocking pool structure is dropped. However, tasks must be explictly shutdown, so we must drain the queue can call `shutdown` on each task. Fixes #1970, #1946
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 looks good to me, modulo a couple of minor non-blocking nits.
tokio/src/runtime/blocking/pool.rs
Outdated
impl Drop for Inner { | ||
fn drop(&mut self) { | ||
let shared = self.shared.lock().unwrap(); | ||
assert!(shared.queue.is_empty()); |
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 seems like a good way to get weird SIGILLs if there's a bug in the logic for draining the queue...can we just drain the queue here instead, or is there a reason we can't do that?
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.
Oh yeah, this wasn't meant to make it into the final... I was debugging still.
drop(shared); | ||
task.shutdown(); | ||
|
||
shared = self.shared.lock().unwrap(); |
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.
the way this code drops and then reacquires the lock to be used in the next iteration of the loop is a little weird to me, but I can't really think of a better way to express it, given that the lock is already held when the loop starts...
It was there for debugging
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 seems good to me, although I'm not too familiar with this code.
The blocking task queue was not explicitly drained as part of the
blocking pool shutdown logic. It was originally assumed that the
contents of the queue would be dropped when the blocking pool structure
is dropped. However, tasks must be explictly shutdown, so we must drain
the queue can call
shutdown
on each task.Fixes #1970, #1946