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

rt: add Handle::block_on #3569

Merged
merged 19 commits into from
Mar 20, 2021
Merged

Conversation

davidpdrsn
Copy link
Member

This is a continuation of #3549 but with tests for the behavior described here and part of the implementation.

Still missing network operations failing if runtime associated with the handle has been shutdown. Changes to make timers and filesystem operations fail are at least passing the tests.

This is my first contribution to Tokio but let me know if I'm on the right track here 😊

Refs: #3097
Fixes #2965, #3096
cc @stuhood, @Keruspe

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

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Some comments here.

tokio/src/fs/mod.rs Outdated Show resolved Hide resolved
tokio/src/runtime/handle.rs Outdated Show resolved Hide resolved
tokio/src/time/driver/entry.rs Outdated Show resolved Hide resolved
tokio/src/runtime/tests/handle.rs Outdated Show resolved Hide resolved
tokio/src/runtime/tests/handle.rs Outdated Show resolved Hide resolved
@jonhoo jonhoo mentioned this pull request Mar 1, 2021
@carllerche
Copy link
Member

Generally I dont think attempting an op on a shutdown runtime should panic. Instead Err is better. That said... sleep is an interesting case because it does not return Result.

@davidpdrsn
Copy link
Member Author

Summary from discussions on Discord:

  • Handle::block_on should simply return the output of the future.
  • Things that aren't runtime dependent shouldn't care about the runtime being shutdown. They should continue to run.
    • This includes tokio::signal::ctrl_c and channels.
  • Functions such as TcpStream::connect which return a result today and require the runtime to work should return an error if the runtime gets shutdown. This includes stuff in tokio::net.
    • Currently they will fail if put onto one runtime, moved to a second, and the first is shutdown. We should reuse the same error for this case.
  • Futures created by functions from tokio::fs have two possible cases
    • If the task has started before the runtime is shutdown it should continue (cannot cancel in-flight).
    • Any task that has not yet started should error when given to Handle::block_on as the JoinHandle should return an error when awaited.
    • Same goes for Stdin, Stderr, Stdout as they also run on a blocking threadpool.
  • Timer futures should panic as they don't return a Result today and are dependent on the runtime.
    • There is some uncertainty about whether this is the current behavior so we should verify this with tests.

@carllerche
Copy link
Member

@davidpdrsn what's the status of this? I think it was blocked on shutting down the timer properly. Is that issue tracked somewhere?

@davidpdrsn
Copy link
Member Author

@carllerche I haven't had more time to work in it. Will spend some time on it this week.

@davidpdrsn
Copy link
Member Author

I've pushed a bunch more changes. I think all the requirements should have tests and be working.

Still missing docs but I'll work on that if people think the implementation looks sound.

let err = Handle::current().block_on(listener.accept()).unwrap_err();

assert_eq!(err.kind(), std::io::ErrorKind::Other);
assert_eq!(err.get_ref().unwrap().to_string(), "reactor gone");
Copy link
Member Author

Choose a reason for hiding this comment

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

This error message was already there. I think it makes sense to change it to A Tokio 1.x context was found, but it is being shutdown. for consistency. Wondering if that would be breaking change though.

@davidpdrsn davidpdrsn marked this pull request as ready for review March 13, 2021 23:14
@davidpdrsn davidpdrsn force-pushed the rt-handle-block-on branch 2 times, most recently from 99823ef to 048bc21 Compare March 13, 2021 23:33
tokio/src/runtime/handle.rs Outdated Show resolved Hide resolved
@carllerche
Copy link
Member

We need to slow down here. Anything that touches I/O shutdown needs to factor in #3481 or it will be racy. I will try to provide a deeper review shortly.

@carllerche
Copy link
Member

Thanks for the detailed tests. I am going to spend more time with this PR and will comment asap.

@carllerche
Copy link
Member

Sigh, I'm sorry, it seems like this PR is exposing more bugs in shutdown 😬

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 for doing this. I left some notes inline.

This is definitely a bit of a tangled problem that you are tackling. The issue is that there are known bugs with the shutdown process that you are hitting when trying to write tests. I love that you took the initiative to try to fix I/O shutdown as part of this PR. Unfortunately, the proposed solution will be racy when concurrently shutting down and registering a new resource. Issue #3481 includes a proposed solution that should avoid races.

Fixing I/O shutdown is out of scope of this PR, but you also wrote great tests that won't pass until the shutdown bugs are fixed. I see two paths forward.

a) We pause this PR until #3481 is fixed, then rebase and land this one.
b) We mark tests that cannot pass until #3481 is fixed with #[ignore] and reference the issue. Then we revert the is_shutdown: AtomicBool part of this PR and land it w/ known issues.

If I had to pick one, I would opt for b as we do need Handle::block_on and the shutdown bugs are unrelated. Thoughts?

tokio/src/io/driver/mod.rs Outdated Show resolved Hide resolved
tokio/src/io/driver/registration.rs Outdated Show resolved Hide resolved
tokio/src/io/driver/registration.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Contributor

jonhoo commented Mar 15, 2021

I think I agree that b is the right path forward — we don't expect this to introduce additional broken behavior, and we believe (I think) that we'd still want the same public API once we fix #3481.

davidpdrsn added a commit to EmbarkStudios/tokio that referenced this pull request Mar 16, 2021
@jeromegn
Copy link

jeromegn commented Mar 18, 2021

Maybe interesting data point: we've been running this in production on all our servers for 2 days.

Co-authored-by: Carl Lerche <me@carllerche.com>
Co-authored-by: Alice Ryhl <alice@ryhl.io>
@Darksonn Darksonn dismissed carllerche’s stale review March 20, 2021 09:11

Carl has already approved this as per discussion in Discord.

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.

Replacement for Handle::current
6 participants