rt: add Handle::block_on #3569

davidpdrsn · 2021-03-01T16:36:41Z

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

Some comments here.

tokio/src/fs/mod.rs

tokio/src/runtime/handle.rs

tokio/src/time/driver/entry.rs

tokio/src/runtime/tests/handle.rs

tokio/src/runtime/blocking/pool.rs

tokio/src/time/driver/entry.rs

carllerche · 2021-03-01T21:11:41Z

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 · 2021-03-02T21:48:14Z

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 · 2021-03-09T17:49:08Z

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

davidpdrsn · 2021-03-09T18:23:39Z

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

davidpdrsn · 2021-03-13T23:03:54Z

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.

davidpdrsn · 2021-03-13T23:10:08Z

tokio/tests/rt_handle_block_on.rs

+        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");


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.

tokio/src/io/driver/registration.rs

tokio/src/runtime/handle.rs

tokio/src/time/driver/entry.rs

carllerche · 2021-03-15T21:59:24Z

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 · 2021-03-15T22:54:03Z

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

carllerche · 2021-03-15T23:21:55Z

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

carllerche

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

tokio/src/io/driver/registration.rs

jonhoo · 2021-03-15T23:57:19Z

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.

See tokio-rs#3569 (review) for more details

jeromegn · 2021-03-18T20:44:59Z

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

tokio/src/runtime/handle.rs

Co-authored-by: Carl Lerche <me@carllerche.com>

tokio/src/runtime/handle.rs

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Carl has already approved this as per discussion in Discord.

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

Darksonn reviewed Mar 1, 2021

View reviewed changes

jonhoo mentioned this pull request Mar 1, 2021

rt: add Handle::block_on #3549

Closed

LucioFranco reviewed Mar 1, 2021

View reviewed changes

tokio/src/runtime/blocking/pool.rs Outdated Show resolved Hide resolved

LucioFranco reviewed Mar 1, 2021

View reviewed changes

tokio/src/time/driver/entry.rs Outdated Show resolved Hide resolved

davidpdrsn force-pushed the rt-handle-block-on branch from 3a8f1e4 to d85ea74 Compare March 13, 2021 23:01

davidpdrsn requested review from LucioFranco, Darksonn and carllerche March 13, 2021 23:02

davidpdrsn commented Mar 13, 2021

View reviewed changes

davidpdrsn marked this pull request as ready for review March 13, 2021 23:14

davidpdrsn force-pushed the rt-handle-block-on branch 2 times, most recently from 99823ef to 048bc21 Compare March 13, 2021 23:33

LucioFranco reviewed Mar 15, 2021

View reviewed changes

tokio/src/io/driver/registration.rs Outdated Show resolved Hide resolved

jonhoo reviewed Mar 15, 2021

View reviewed changes

tokio/src/io/driver/registration.rs Outdated Show resolved Hide resolved

jonhoo reviewed Mar 15, 2021

View reviewed changes

tokio/src/runtime/handle.rs Outdated Show resolved Hide resolved

jonhoo reviewed Mar 15, 2021

View reviewed changes

tokio/src/time/driver/entry.rs Show resolved Hide resolved

jonhoo approved these changes Mar 15, 2021

View reviewed changes

carllerche previously requested changes Mar 15, 2021

View reviewed changes

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

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

Ignore io+shutdown tests for now due to known bugs

df097d9

See tokio-rs#3569 (review) for more details

davidpdrsn added 14 commits March 16, 2021 21:46

Fix for some features not being enabled

b55798a

Don't test unix things on windows

c157cf1

Remove unused import

bba8cf0

Fix typo

d6a99aa

Only upgrade Arc twice

cc592b3

Ignore io+shutdown tests for now due to known bugs

f87eac2

See tokio-rs#3569 (review) for more details

Improve error message when runtime is shutting down

6f6dca2

Initial docs for Handle::block_on

ea59cc5

fix grammar

7f07403

Tweak example

9da232b

Mention drop as a way of shutting down the runtime

f5fbffa

Adjust docs

1c3f43d

Remove _ prefix

6d4fa2c

Fix docs

4a5bdfa

davidpdrsn force-pushed the rt-handle-block-on branch from db1acca to 4a5bdfa Compare March 16, 2021 20:47

tdyas mentioned this pull request Mar 17, 2021

upgrade to Tokio v1.x ecosystem pantsbuild/pants#11475

Merged

carllerche reviewed Mar 19, 2021

View reviewed changes

tokio/src/runtime/handle.rs Outdated Show resolved Hide resolved

Update tokio/src/runtime/handle.rs

2372a40

Co-authored-by: Carl Lerche <me@carllerche.com>

Darksonn reviewed Mar 20, 2021

View reviewed changes

tokio/src/runtime/handle.rs Outdated Show resolved Hide resolved

Update tokio/src/runtime/handle.rs

c83fa53

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Make Handle::block_on examples run

2e66473

Darksonn approved these changes Mar 20, 2021

View reviewed changes

davidpdrsn merged commit c39d986 into tokio-rs:master Mar 20, 2021

davidpdrsn deleted the rt-handle-block-on branch March 20, 2021 09:52

This was referenced Mar 20, 2021

chore: prepare Tokio v1.4.0 #3621

Merged

Add block_on to Handle #3096

Closed

softdevca mentioned this pull request Mar 22, 2021

Bump tokio from 1.3.0 to 1.4.0 softdevca/mootranscode#5

Merged

jszwedko mentioned this pull request Apr 2, 2021

chore(deps): Upgrade Tokio to 1.4 vectordotdev/vector#6993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rt: add Handle::block_on #3569

rt: add Handle::block_on #3569

davidpdrsn commented Mar 1, 2021

Darksonn left a comment

carllerche commented Mar 1, 2021

davidpdrsn commented Mar 2, 2021

carllerche commented Mar 9, 2021

davidpdrsn commented Mar 9, 2021

davidpdrsn commented Mar 13, 2021

davidpdrsn Mar 13, 2021

carllerche commented Mar 15, 2021

carllerche commented Mar 15, 2021

carllerche commented Mar 15, 2021

carllerche left a comment

jonhoo commented Mar 15, 2021

jeromegn commented Mar 18, 2021 •

edited

Loading

rt: add Handle::block_on #3569

rt: add Handle::block_on #3569

Conversation

davidpdrsn commented Mar 1, 2021

Darksonn left a comment

Choose a reason for hiding this comment

carllerche commented Mar 1, 2021

davidpdrsn commented Mar 2, 2021

carllerche commented Mar 9, 2021

davidpdrsn commented Mar 9, 2021

davidpdrsn commented Mar 13, 2021

davidpdrsn Mar 13, 2021

Choose a reason for hiding this comment

carllerche commented Mar 15, 2021

carllerche commented Mar 15, 2021

carllerche commented Mar 15, 2021

carllerche left a comment

Choose a reason for hiding this comment

jonhoo commented Mar 15, 2021

jeromegn commented Mar 18, 2021 • edited Loading

jeromegn commented Mar 18, 2021 •

edited

Loading