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: avoid early task shutdown #3870
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,9 +124,14 @@ impl<T> Local<T> { | |
// There is capacity for the task | ||
break tail; | ||
} else if steal != real { | ||
// Concurrently stealing, this will free up capacity, so | ||
// only push the new task onto the inject queue | ||
inject.push(task); | ||
// Concurrently stealing, this will free up capacity, so only | ||
// push the new task onto the inject queue | ||
// | ||
// If the task failes to be pushed on the injection queue, there | ||
// is nothing to be done at this point as the task cannot be a | ||
// newly spawned task. Shutting down this task is handled by the | ||
// worker shutdown process. | ||
let _ = inject.push(task); | ||
return; | ||
} else { | ||
// Push the current task and half of the queue into the | ||
|
@@ -507,19 +512,19 @@ impl<T: 'static> Inject<T> { | |
} | ||
|
||
/// Pushes a value into the queue. | ||
pub(super) fn push(&self, task: task::Notified<T>) | ||
/// | ||
/// Returns `Err(task)` if pushing fails due to the queue being shutdown. | ||
/// The caller is expected to call `shutdown()` on the task **if and only | ||
/// if** it is a newly spawned task. | ||
pub(super) fn push(&self, task: task::Notified<T>) -> Result<(), task::Notified<T>> | ||
where | ||
T: crate::runtime::task::Schedule, | ||
{ | ||
// Acquire queue lock | ||
let mut p = self.pointers.lock(); | ||
|
||
if p.is_closed { | ||
// Drop the mutex to avoid a potential deadlock when | ||
// re-entering. | ||
drop(p); | ||
task.shutdown(); | ||
return; | ||
return Err(task); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fix is to push the |
||
} | ||
|
||
// safety: only mutated with the lock held | ||
|
@@ -538,6 +543,7 @@ impl<T: 'static> Inject<T> { | |
p.tail = Some(task); | ||
|
||
self.len.store(len + 1, Release); | ||
Ok(()) | ||
} | ||
|
||
pub(super) fn push_batch( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,8 +12,8 @@ use std::future::Future; | |
use std::pin::Pin; | ||
use std::sync::atomic::AtomicUsize; | ||
use std::sync::atomic::Ordering::Relaxed; | ||
use std::sync::{mpsc, Arc}; | ||
use std::task::{Context, Poll}; | ||
use std::sync::{mpsc, Arc, Mutex}; | ||
use std::task::{Context, Poll, Waker}; | ||
|
||
#[test] | ||
fn single_thread() { | ||
|
@@ -405,6 +405,74 @@ async fn hang_on_shutdown() { | |
tokio::time::sleep(std::time::Duration::from_secs(1)).await; | ||
} | ||
|
||
/// Demonstrates tokio-rs/tokio#3869 | ||
#[test] | ||
fn wake_during_shutdown() { | ||
struct Shared { | ||
waker: Option<Waker>, | ||
} | ||
|
||
struct MyFuture { | ||
shared: Arc<Mutex<Shared>>, | ||
put_waker: bool, | ||
} | ||
|
||
impl MyFuture { | ||
fn new() -> (Self, Self) { | ||
let shared = Arc::new(Mutex::new(Shared { waker: None })); | ||
let f1 = MyFuture { | ||
shared: shared.clone(), | ||
put_waker: true, | ||
}; | ||
let f2 = MyFuture { | ||
shared, | ||
put_waker: false, | ||
}; | ||
(f1, f2) | ||
} | ||
} | ||
|
||
impl Future for MyFuture { | ||
type Output = (); | ||
|
||
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<()> { | ||
let me = Pin::into_inner(self); | ||
let mut lock = me.shared.lock().unwrap(); | ||
println!("poll {}", me.put_waker); | ||
if me.put_waker { | ||
println!("putting"); | ||
lock.waker = Some(cx.waker().clone()); | ||
} | ||
Poll::Pending | ||
} | ||
} | ||
|
||
impl Drop for MyFuture { | ||
fn drop(&mut self) { | ||
println!("drop {} start", self.put_waker); | ||
let mut lock = self.shared.lock().unwrap(); | ||
if !self.put_waker { | ||
lock.waker.take().unwrap().wake(); | ||
} | ||
drop(lock); | ||
println!("drop {} stop", self.put_waker); | ||
} | ||
Comment on lines
+451
to
+459
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To reproduce this bug, it requires the task with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's any way to get any guarantees about the drop order. |
||
} | ||
|
||
let rt = tokio::runtime::Builder::new_multi_thread() | ||
.worker_threads(1) | ||
.enable_all() | ||
.build() | ||
.unwrap(); | ||
|
||
let (f1, f2) = MyFuture::new(); | ||
|
||
rt.spawn(f1); | ||
rt.spawn(f2); | ||
|
||
rt.block_on(async { tokio::time::sleep(tokio::time::Duration::from_millis(20)).await }); | ||
} | ||
|
||
fn rt() -> Runtime { | ||
Runtime::new().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.
IMO, any unhandled
Result
values should result in a compilation failure.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 is not directly related, so if needed, I can split it off to another PR.