rt: avoid dropping a task in calls to wake() #1972

carllerche · 2019-12-16T23:16:04Z

Calls to tasks should not be nested. Currently, while a task is being
executed and the runtime is shutting down, a call to wake() can result
in the wake target to be dropped. This, in turn, results in the drop
handler being called.

If the user holds a ref cell borrow, a mutex guard, or any such value,
dropping the task inline can result in a deadlock.

The fix is to permit tasks to be scheduled during the shutdown process
and dropping the tasks once they are popped from the queue.

Fixes #1929, #1886

This only fixes single threaded schedulers. I have net yet reproduced the bug in
the threaded scheduler but will continue to explore.

Calls to tasks should not be nested. Currently, while a task is being executed and the runtime is shutting down, a call to wake() can result in the wake target to be dropped. This, in turn, results in the drop handler being called. If the user holds a ref cell borrow, a mutex guard, or any such value, dropping the task inline can result in a deadlock. The fix is to permit tasks to be scheduled during the shutdown process and dropping the tasks once they are popped from the queue. Fixes #1929, #1886 This only fixes single threaded schedulers.

hawkw

The fix makes sense to me, thanks for figuring this out! I left a few nits that aren't really blockers.

hawkw · 2019-12-16T23:19:46Z

tokio/src/runtime/basic_scheduler.rs

+                    break;
+                }
+
+                self.local.park.park().ok().expect("park failed");


Nit/TIOLI: this is safe to call, right? Maybe it should be moved out of the unsafe block?

tokio/src/task/queue.rs

tokio/tests/task_local_set.rs

hawkw · 2019-12-16T23:24:03Z

tokio/src/runtime/basic_scheduler.rs

@@ -161,7 +161,7 @@ impl Spawner {
        F::Output: Send + 'static,
    {
        let (task, handle) = task::joinable(future);
-        self.scheduler.schedule(task);
+        self.scheduler.schedule(task, true);


The meaning of the bool parameter here is a little unclear at the callsite, I'd prefer it if it was clearer what the "false" means. But, I don't think the other options I can think of (having separate schedule and schedule_spawn fns, or using an enum) are really worth it...

hawkw · 2019-12-17T01:16:20Z

tokio/tests/task_local_set.rs

+use tokio::task::{self, LocalSet};
+
+#[test]
+fn acquire_mutex_in_drop() {


The failure mode for this is a deadlock, right? Should we maybe do something similar to this test and add a timeout?

tokio/tokio/src/task/local.rs

Lines 732 to 741 in efb4b67

// Since the failure mode of this test is an infinite loop, rather than

// something we can easily make assertions about, we'll run it in a

// thread. When the test thread finishes, it will send a message on a

// channel to this thread. We'll wait for that message with a fairly

// generous timeout, and if we don't recieve it, we assume the test

// thread has hung.

//

// Note that it should definitely complete in under a minute, but just

// in case CI is slow, we'll give it a long timeout.

match done_rx.recv_timeout(Duration::from_secs(60)) {

I rather it just deadlock tbh... timeouts don't seem great.

carllerche · 2019-12-17T05:29:48Z

It seems like CI isn't reporting back :(

Co-Authored-By: Eliza Weisman <eliza@buoyant.io>

carllerche requested review from jonhoo and hawkw December 16, 2019 23:16

hawkw approved these changes Dec 16, 2019

View reviewed changes

carllerche added 2 commits December 16, 2019 16:43

fmt

0ff8094

fix warning

93b3779

hawkw reviewed Dec 17, 2019

View reviewed changes

carllerche added 2 commits December 16, 2019 18:39

CI bump

cfd8a12

CI bump

6757bb3

CI bump

3ed3064

This was referenced Dec 17, 2019

Dropping an mpsc::Receiver deadlocks #1886

Closed

BorrowMutError in tokio-0.2.4 #1929

Closed

Update tokio/src/task/queue.rs

db3bc8f

Co-Authored-By: Eliza Weisman <eliza@buoyant.io>

carllerche merged commit 41d15ea into master Dec 18, 2019

carllerche deleted the fix-drop-in-wake branch January 10, 2020 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rt: avoid dropping a task in calls to wake() #1972

rt: avoid dropping a task in calls to wake() #1972

carllerche commented Dec 16, 2019

hawkw left a comment

hawkw Dec 16, 2019

hawkw Dec 16, 2019

hawkw Dec 17, 2019

carllerche Dec 17, 2019

carllerche commented Dec 17, 2019

	// Since the failure mode of this test is an infinite loop, rather than
	// something we can easily make assertions about, we'll run it in a
	// thread. When the test thread finishes, it will send a message on a
	// channel to this thread. We'll wait for that message with a fairly
	// generous timeout, and if we don't recieve it, we assume the test
	// thread has hung.
	//
	// Note that it should definitely complete in under a minute, but just
	// in case CI is slow, we'll give it a long timeout.
	match done_rx.recv_timeout(Duration::from_secs(60)) {

rt: avoid dropping a task in calls to wake() #1972

rt: avoid dropping a task in calls to wake() #1972

Conversation

carllerche commented Dec 16, 2019

hawkw left a comment

Choose a reason for hiding this comment

hawkw Dec 16, 2019

Choose a reason for hiding this comment

hawkw Dec 16, 2019

Choose a reason for hiding this comment

hawkw Dec 17, 2019

Choose a reason for hiding this comment

carllerche Dec 17, 2019

Choose a reason for hiding this comment

carllerche commented Dec 17, 2019