rt: move block_on to handle #3097

Keruspe · 2020-11-05T13:16:50Z

One difference between Runtime::block_on and Handle::block_on: the latter doesn't use the Inner from BasicScheduler in case of CurrentThreadconfiguration. Given the main use ofHandle::block_on`` is to be able to call block_on on the Runtime when using `#[tokio::main]`, it shouldn't matter though, given the `Inner` is already being used to `blobk_on` the `main`, thus is `None`.

One potential behavioural change in Runtime::block_on: the use of BasicScheduler::inner is not longer in the loop, so we'll only attempt at using it on the first pass. I cannot think of an easy workaround for this without bringing the scheduler inside of the Handle, making it Handle<P: Park> which would make it a pain to carry I think.

Signed-off-by: Marc-Antoine Perennou <[email protected]>

Keruspe · 2020-11-06T09:23:26Z

(force-push was just a missing comma in the last commit because of a fail amend, 1s after initial push)

I'm not sure everything is handled as it should, but at least the public API should be ok and the behaviour too

LucioFranco · 2020-12-14T17:10:32Z

Since, this patch isn't trivial and is a larger change than we'd like. We are not going to include this in 1.0. As for other solutions, one should be using Arc<Runtime and Weak<Runtime> to capture the same behavior as this. While this may not be ideal, I do not think hastily including this for 1.0 is a good idea until we have worked through the possible permutations.

stuhood · 2020-12-14T17:27:50Z

Since, this patch isn't trivial and is a larger change than we'd like. We are not going to include this in 1.0. As for other solutions, one should be using Arc<Runtime and Weak<Runtime> to capture the same behavior as this.

How would you get a reference to the runtime when using the tokio::test or tokio::main macros?

LucioFranco · 2020-12-14T19:00:38Z

How would you get a reference to the runtime when using the tokio::test or tokio::main macros?

In this, case you will want to just create the runtime manually and use block_on. Its a bit more verbose but isn't that much of a change. You just won't be able to use tokio::test or tokio::main but those are simple macros that only remove 5loc really.

sunjay · 2020-12-14T20:27:25Z

I am fine with letting this iterate for a little while until people are satisfied with the implementation, but I do want to add that as a library attempting to use Tokio entirely behind the scenes, just using Arc<Runtime> is not really an option for me. I'm probably going to need to wait until handle.block_on is released before I can finish my migration.

I definitely agree that in most cases threading the runtime through the app is probably what you want, but in general I think of the runtime as a somewhat global/implicit resource. Being able to get a handle to that in any location is extremely useful. :)

LucioFranco · 2020-12-14T21:04:21Z

@sunjay can you explain more why Arc<Runtime> does not work for you?

stuhood · 2020-12-14T22:13:06Z

How would you get a reference to the runtime when using the tokio::test or tokio::main macros?

In this, case you will want to just create the runtime manually and use block_on. Its a bit more verbose but isn't that much of a change. You just won't be able to use tokio::test or tokio::main but those are simple macros that only remove 5loc really.

If you have hundreds of tests, not being able to use tokio::test is a non-trivial issue. Would probably need to write a version of the macro that gave the test a reference to the runtime.

As explained on #3076 (comment) , not having a uniform way to get access to the current Runtime blocks our upgrade to 0.3... and based on the number of issues that reference either this PR or #2965, we're not alone.

sunjay · 2020-12-14T22:18:52Z

@sunjay can you explain more why Arc<Runtime> does not work for you?

@LucioFranco Sure. I am using tokio behind the scenes in the turtle crate. (PR for updating to tokio 0.3.) The crate is targeting beginners to Rust, so we try to avoid additional complexity whenever we can. In the default "synchronous" mode of the crate, tokio is hidden entirely. The runtime is created implicitly the first time an asynchronous call is made.

When users use the AsyncTurtle API (WIP, currently private), they control everything. We expect that people will use #[tokio::main] or otherwise create their Tokio runtime in whatever way suits their application. We don't want them to have to pass a runtime to the turtle crate somehow because then they can't use #[tokio::main] (as you pointed out). This is a totally unnecessary burden and I would not want to add that complexity to the crate.

We could potentially rely on the RUNTIME global static, but that would both break encapsulation and end up spawning a second runtime in cases where the user has already created their own.

So that's why Arc<Runtime> doesn't work for me here. We want tokio to largely remain an implementation detail and we want the user to be able to create their runtime however they want without passing it to the crate.

As for where we'd need to use Arc<Runtime>: Thanks to #3262 being merged we can remove most of our uses of block_on by either calling blocking_send or blocking_recv. The only remaining uses I've found so far are in the code where we wait for a process to end in a Drop implementation.

I don't actually think there is any way to get around this one without block_on, so we are stuck until that is added somehow.

LucioFranco · 2020-12-16T15:37:59Z

@stuhood while yes its unfortunate, for us to get 1.0 out we have needed to reduce surface area, and this is one of those places unfortunately. Its possible to fork tokio-macros or create your own macro. While its a bit annoying it is a way to unblock yourself.

@sunjay personally, I think its fair to ask users to run it within a future executing on the tokio runtime. That way you reduce the surface area. I think Handle/Handle::current is a smell for a design. The tokio futures should panic and provide a clear message as to why (something I think we can do even better).

qm3ster · 2020-12-20T02:33:00Z

Are we getting Runtime::current() instead, then?

Darksonn · 2020-12-20T09:08:57Z

@qm3ster No, just renaming the Handle type to Runtime doesn't solve this issue. See #2965 for discussion on that. We will eventually just add Handle::block_on when we figure out a reasonable behavior if called during shutdown of the runtime.

JeremyRubin · 2021-01-02T20:34:42Z

Just chiming in -- I may be missing something -- but I think I need Handle::try_current + Handle::block_on in an application I'm working on where I'm implementing an externally defined non-async trait on a struct which has internal async types (e.g., TcpStream).

carllerche · 2021-02-23T18:27:25Z

Thanks for doing this. I think this change is fine. It looks like it is mostly shuffling things around. I don't see a problem w/ how this landed.

Thoughts @tokio-rs/maintainers? I believe this is blocking some upgrades to 1.0.

carllerche · 2021-02-23T18:39:46Z

I resolved the merge conflicts.

Darksonn · 2021-02-23T20:33:07Z

How does this handle the case where you shut down the runtime while a handle is blocking on something? I'm fine with whatever, but it is nice to know the answer.

carllerche · 2021-02-23T20:34:48Z

How does this handle the case where you shut down the runtime while a handle is blocking on something?

A very good point. What do we want to see here?

carllerche · 2021-02-23T20:37:18Z

My guess is, we want the shutdown logic to reclaim the runtime and terminate it.

LucioFranco

Before I review this anymore, I am not sure this behavior is correct and there are no tests? This type of behavior is tricky and we should have tests checking all the cases.

LucioFranco · 2021-02-23T20:32:04Z

tokio/src/runtime/handle.rs

+    /// Run a future to completion on the Tokio runtime. This is the
+    /// runtime's entry point.


This is incorrect, this is not the entry point but just an additional block_on?

LucioFranco · 2021-02-23T20:32:57Z

tokio/src/runtime/handle.rs

+    ///
+    /// # Multi thread scheduler
+    ///
+    /// When the multi thread scheduler is used this will allow futures


I would mention it will also run spawned tasks on the runtime worker threads.

LucioFranco · 2021-02-23T20:35:02Z

tokio/src/runtime/handle.rs

+    ///
+    /// # Current thread scheduler
+    ///
+    /// When the current thread scheduler is enabled `block_on`


While this is the behavior of Runtime::block_on I don't think this should be the behavior of Handle::block_on? @carllerche What do you think? I don't think the handle's block on should be able to steal the runtime. In that case it is the same as Runtime except it removes the ownership of the runtime type.

Well, my initial thinking was that the handle should follow the Runtime's behavior, but it does complicate the shutdown logic as well as seems a bit weird given the "current_thread" name... Perhaps, block_on in this case should just block the thread and enter the runtime context...

LucioFranco · 2021-02-23T20:46:50Z

Also, the behavior should follow this (which we don't even have yet for spawn) #3548 where we return an error when the runtime goes away. A handle should not hold ownership of any of the scheduler components (like the task queue).

LucioFranco · 2021-02-23T20:47:45Z

How does this handle the case where you shut down the runtime while a handle is blocking on something? I'm fine with whatever, but it is nice to know the answer.

@Darksonn we should return an error, somehow we'd need to wake up. Its complicated but in the case where the runtime goes away while you're blocking on. It will just hang and this is bad UX/hard to debug.

carllerche · 2021-02-23T20:56:45Z

I opened a draft PR w/ an alternate strategy: #3549. I'm interested in feedback on the pros / cons.

carllerche · 2021-03-16T19:58:03Z

Thanks for the initial work & getting the ball going on this one. Closing in favor of #3569, which should be close.

Add `runtime::Handle::block_on`. The function enters the runtime context and blocks the current thread while the future executes. Refs: #3097 Fixes #2965, #3096

Keruspe added 6 commits November 5, 2020 14:11

rt: move block_on to handle

d43812d

Signed-off-by: Marc-Antoine Perennou <[email protected]>

rt: fix formatting

7bb036e

Signed-off-by: Marc-Antoine Perennou <[email protected]>

rt: avoid irrefutable if let

43094fb

Signed-off-by: Marc-Antoine Perennou <[email protected]>

rt: naive attempt at restoring previous Runtime::block_on behaviour

e4727ff

Signed-off-by: Marc-Antoine Perennou <[email protected]>

rt: formatting fix

14ee0c2

Signed-off-by: Marc-Antoine Perennou <[email protected]>

rt: another formatting fix

54fc4fb

Signed-off-by: Marc-Antoine Perennou <[email protected]>

Keruspe force-pushed the handle branch from 6b12d55 to 54fc4fb Compare November 6, 2020 09:01

emgre mentioned this pull request Nov 6, 2020

migrate to tokio 0.3 stepfunc/rodbus#41

Closed

taiki-e added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Nov 7, 2020

sunjay mentioned this pull request Dec 14, 2020

Upgrade to tokio 1.0 sunjay/turtle#215

Open

5 tasks

stuhood mentioned this pull request Dec 14, 2020

Replacement for Handle::current #2965

Closed

MikailBag mentioned this pull request Dec 25, 2020

[Experiment] Semver trick #3322

Closed

noel2004 mentioned this pull request Jan 12, 2021

tokio v1! fluidex/dingir-exchange#39

Merged

Merge remote-tracking branch 'origin/master' into handle

46e0cdf

LucioFranco requested changes Feb 23, 2021

View reviewed changes

carllerche mentioned this pull request Feb 23, 2021

rt: add Handle::block_on #3549

Closed

davidpdrsn mentioned this pull request Mar 1, 2021

rt: add Handle::block_on #3569

Merged

carllerche closed this Mar 16, 2021

davidpdrsn added a commit that referenced this pull request Mar 20, 2021

runtime: add Handle::block_on (#3569)

c39d986

Add `runtime::Handle::block_on`. The function enters the runtime context and blocks the current thread while the future executes. Refs: #3097 Fixes #2965, #3096

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rt: move block_on to handle #3097

rt: move block_on to handle #3097

Keruspe commented Nov 5, 2020 •

edited

Loading

Keruspe commented Nov 6, 2020

LucioFranco commented Dec 14, 2020

stuhood commented Dec 14, 2020

LucioFranco commented Dec 14, 2020

sunjay commented Dec 14, 2020

LucioFranco commented Dec 14, 2020

stuhood commented Dec 14, 2020

sunjay commented Dec 14, 2020 •

edited

Loading

LucioFranco commented Dec 16, 2020

qm3ster commented Dec 20, 2020

Darksonn commented Dec 20, 2020

JeremyRubin commented Jan 2, 2021

carllerche commented Feb 23, 2021

carllerche commented Feb 23, 2021

Darksonn commented Feb 23, 2021

carllerche commented Feb 23, 2021

carllerche commented Feb 23, 2021

LucioFranco left a comment

LucioFranco Feb 23, 2021

LucioFranco Feb 23, 2021

LucioFranco Feb 23, 2021

carllerche Feb 23, 2021

LucioFranco commented Feb 23, 2021

LucioFranco commented Feb 23, 2021

carllerche commented Feb 23, 2021

carllerche commented Mar 16, 2021

		/// Run a future to completion on the Tokio runtime. This is the
		/// runtime's entry point.

rt: move block_on to handle #3097

rt: move block_on to handle #3097

Conversation

Keruspe commented Nov 5, 2020 • edited Loading

Keruspe commented Nov 6, 2020

LucioFranco commented Dec 14, 2020

stuhood commented Dec 14, 2020

LucioFranco commented Dec 14, 2020

sunjay commented Dec 14, 2020

LucioFranco commented Dec 14, 2020

stuhood commented Dec 14, 2020

sunjay commented Dec 14, 2020 • edited Loading

LucioFranco commented Dec 16, 2020

qm3ster commented Dec 20, 2020

Darksonn commented Dec 20, 2020

JeremyRubin commented Jan 2, 2021

carllerche commented Feb 23, 2021

carllerche commented Feb 23, 2021

Darksonn commented Feb 23, 2021

carllerche commented Feb 23, 2021

carllerche commented Feb 23, 2021

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco Feb 23, 2021

Choose a reason for hiding this comment

LucioFranco Feb 23, 2021

Choose a reason for hiding this comment

LucioFranco Feb 23, 2021

Choose a reason for hiding this comment

carllerche Feb 23, 2021

Choose a reason for hiding this comment

LucioFranco commented Feb 23, 2021

LucioFranco commented Feb 23, 2021

carllerche commented Feb 23, 2021

carllerche commented Mar 16, 2021

Keruspe commented Nov 5, 2020 •

edited

Loading

sunjay commented Dec 14, 2020 •

edited

Loading