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

Test for shutdown of runtime in Handle::spawn #3708

Closed
wants to merge 3 commits into from

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Apr 16, 2021

Tries to fix #3548

We use the shutdown flag of the blocking spawner's Shared field to allow a Handle to infer that the runtime was shutdown. If the flag is set, Handle::spawn and Handle::spawn_blocking will return a newly introduced JoinError::DroppedRuntime.

A potential downside is that JoinError was previously solely related to errors caused by a task itself. I couldn't find a cleaner alternative than to introduce a new kind of JoinError since returning a Result from the spawn methods breaks all kinds of things in the API.

Also I'm not 100% sure that the shutdown of the blocking spawner is always caused by the runtime being dropped.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Apr 16, 2021
@Darksonn
Copy link
Contributor

We usually use the existing cancelled variant of the join error when the runtime shuts down.

@b-naber
Copy link
Contributor Author

b-naber commented Apr 16, 2021

@Darksonn thanks, I'll change that.

Comment on lines 147 to 153
if self.is_shutdown() {
return JoinHandle::new_dropped_runtime();
}

#[cfg(all(tokio_unstable, feature = "tracing"))]
let future = crate::util::trace::task(future, "task");
self.spawner.spawn(future)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the runtime is shut down right after the self.is_shutdown() call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. We should probably test for the shutdown after the self.spawner.spawn call.

…wns between the is_shutdown call and the spawn call
@carllerche
Copy link
Member

Adding a check in spawn like that is going to be vulnerable to races. Instead, the "shutdown" check needs to happen atomically when pushing into the queue.

It looks like the multi-threaded scheduler already does the right thing: https://github.com/tokio-rs/tokio/blob/master/tokio/src/runtime/queue.rs#L511-L517

For the current-thread scheduler, my guess is similar logic should go here: https://github.com/tokio-rs/tokio/blob/master/tokio/src/runtime/basic_scheduler.rs#L448

@b-naber
Copy link
Contributor Author

b-naber commented Apr 21, 2021

@carllerche The multi-thread scheduler also hangs in this example.

Afaict this hangs because the poll method of JoinHandle sets a join waker in can_read_output, but remains pending, presumably because the runtime was dropped and was responsible for the waking.

I don't understand why the approach using the shutdown flag is vulnerable to race conditions. If you don't mind can you maybe give an example of how a race can occur here? Also why couldn't we solve this by making the shutdown flag atomic?

@Darksonn
Copy link
Contributor

The issue was fixed by #3752.

@Darksonn Darksonn closed this May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rt: Handle::spawn should return an error that the runtime has gone away
3 participants