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: Refactor Runtime::block_on to take &self #2782

Merged
merged 11 commits into from Aug 28, 2020
Merged

Conversation

LucioFranco
Copy link
Member

@LucioFranco LucioFranco commented Aug 20, 2020

This PR is the first in a series of PRs to solve #2720. This change introduces a few things.

  1. Handle has been removed from the public API (only used internally until we refactor it out, likely in an upcoming PR)
  2. Change Runtime::block_on to take &self. This change for the threaded runtime is quite trivial, but for basic/shell runtimes we use a Mutex<Option<Runtime>> to allow the first call to block_on to do all the parking will subsequent ones (while the first one is not done yet) will just use the context from that first block_on.

Most of this PR is removing methods from Handle, refactoring the fact that block_on is now immutable and making CachedThreadParker and friends available without any feature flags (I think this is correct there are no deps for it beyond std ones).

Future work:

  • Refactor basic_scheduler/shell to embed the block_on implementation within their own structs rather than within Runtime::block_on. This means making those block_on's take &self not &mut self. Improve stealing the root parker in situations where it gets reset back into the mutex and another thread needs to now drive it.
  • Review runtime shutdown process.
  • Refactor runtime::Builder to remove threaded_scheduler/basic_scheduler and refactor core_threads to do the behavior described in rt: Runtime refinements #2720.
  • Refactor #[tokio::main] as described in chore: prepare to release 0.2.22 #2672.

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-runtime Module: tokio/runtime labels Aug 23, 2020
@LucioFranco LucioFranco force-pushed the lucio/runtime-refactor branch 2 times, most recently from 5569946 to 099da4c Compare August 27, 2020 15:58
@LucioFranco LucioFranco marked this pull request as ready for review August 27, 2020 16:51
@LucioFranco LucioFranco requested review from carllerche, hawkw, jonhoo and a team August 27, 2020 16:51
tokio/src/park/thread.rs Show resolved Hide resolved
tokio/src/runtime/basic_scheduler.rs Show resolved Hide resolved
tokio/src/runtime/builder.rs Show resolved Hide resolved
tokio/src/runtime/mod.rs Outdated Show resolved Hide resolved
tokio/src/runtime/mod.rs Outdated Show resolved Hide resolved
tokio/src/runtime/mod.rs Outdated Show resolved Hide resolved
tokio/src/runtime/mod.rs Outdated Show resolved Hide resolved
tokio/src/runtime/mod.rs Outdated Show resolved Hide resolved
tokio/src/runtime/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@carllerche carllerche mentioned this pull request Aug 27, 2020
6 tasks
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 taking this on 👍 Looks good overall. I left comments inline. I'll let @hawkw +1 the PR once the changes are done.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

lgtm overall --- a couple questions/nits.

Comment on lines +22 to +25
pub struct TokioContext<'a, F> {
#[pin]
inner: F,
handle: Handle,
handle: &'a Runtime,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm...unlike the prior version, this now has a borrow in it, so it's not 'static. You'd have to write

let rt = ...;
some_other_future_executor::spawn(async move {
    rt.wrap(some_future).await
});

rather than

let rt = ...;
some_other_future_executor::spawn(rt.wrap(some_future));

which seems like a minor ergonomic speed bump in what i imagine is more or less the most common use case for this?

Maybe we should just clone it...what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

So Runtime doesn't implement clone anymore. But you can achieve the same thing if you async move { rt.wrap(fut) } and if you warp the Runtime with an Arc you can clone before moving.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can revisit this in the near future.

Copy link
Member

Choose a reason for hiding this comment

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

yup,

So Runtime doesn't implement clone anymore. But you can achieve the same thing if you async move { rt.wrap(fut) } and if you warp the Runtime with an Arc you can clone before moving.

yup, i missed Carl's review feedback while looking at this. seems good to me, now that the arc is factored out. 👍

tokio/src/runtime/builder.rs Show resolved Hide resolved
tokio/tests/rt_common.rs Show resolved Hide resolved
tokio/tests/rt_common.rs Outdated Show resolved Hide resolved
tokio/tests/rt_common.rs Show resolved Hide resolved
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Aug 27, 2020

I unfortunately do not have the spare cycles these days to review this, even though I wish I did :)

@LucioFranco
Copy link
Member Author

@jonhoo its all good :)

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

lgtm!

@LucioFranco LucioFranco merged commit d600ab9 into master Aug 28, 2020
@LucioFranco LucioFranco deleted the lucio/runtime-refactor branch August 28, 2020 00:05
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-enhancement Category: A PR with an enhancement or bugfix. M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants