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

fix shutdown_timeout(0) blocking #3174

Merged
merged 1 commit into from
Nov 28, 2020
Merged

fix shutdown_timeout(0) blocking #3174

merged 1 commit into from
Nov 28, 2020

Conversation

sharnoff
Copy link
Contributor

Background

After a brief discussion with @Darksonn about the precise behavior given by various methods of shutting down a Runtime (dropping it, shutdown_timeout, shutdown_background), I was experimenting with the available options to try to determine the differences between them.

The issue

During this experimentation, I discovered that calling rt.shutdown_timeout(Duration::from_nanos(0)) actually does wait until all blocking tasks finish, contrary to what would be expected. This is notably different from if we passed Duration::from_nanos(1) instead, which has the effect of shutting down immediately. As best I can tell, this is a bug.

Here's a sample program on the playground that demonstrates this behavior.

The solution

The issue seems to be that the implementation of the runtime's shutdown channel's Sender::wait is returning true (indicating that the timeout duration has not been reached) when given a duration of zero. When that happens, we take extra steps that rely on those tasks having finished.

Other things

This might be a breaking change - I wouldn't expect anyone to be directly relying on the behavior of shutdown_timeout(0), but anyone using shutdown_background might suddenly find that certain tasks are unexpectedly terminated when it wasn't happening before. This is because the current behavior seems to be identical to simply dropping the Runtime, which is more lenient than what the documentation of shutdown_background indicates.

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-runtime Module: tokio/runtime labels Nov 24, 2020
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks! Is there any way to add a test so we don't regress in the future?

@Darksonn Darksonn merged commit 0acd06b into tokio-rs:master Nov 28, 2020
@sharnoff
Copy link
Contributor Author

sharnoff commented Dec 1, 2020

@carllerche Sorry for not getting back to you - I ended up having a really busy week. I'm glad to see that there's a test now :)

@Darksonn
Copy link
Contributor

Darksonn commented Dec 1, 2020

Don't worry about it! We are happy to have your contribution.

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 C-bug Category: This is a bug. M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants