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

Switch from sync to a tokio runtime #3367

Merged
merged 49 commits into from
May 30, 2024
Merged

Conversation

rbtcollins
Copy link
Contributor

@rbtcollins rbtcollins commented May 26, 2023

Being able to use async where relevant across the codebase makes it possible to use async constructs where relevant. rustup has many places where that can be useful: downloading channel metadata, distribution content, unpacking that content to disk, are work that could usefully proceed in parallel - and async provides a good abstraction for that.

We already have a sophisticated disk IO layer that accomodates various OS latency-inducing behaviours, and adapting that to async without any regressions could be very interesting too - but for now, it co-exists nicely with an async core.

One not necessarily obvious caveat - we can't use tokio::main because it doesn't provide the per-worker-thread injection that we use to facilitate isolated tests. (tl;dr std::env::var/args being global reflect reality well, but not writing tests that interact with those layers).

@rbtcollins rbtcollins force-pushed the tokio-runtime branch 2 times, most recently from 230c766 to 65a24a8 Compare May 29, 2023 22:36
@djc
Copy link
Contributor

djc commented Aug 15, 2023

Having briefly skimmed over some parts of this: what limits us from just using #[tokio::main] and making things natively async, rather than all the callback stuff and block_on() type calls?

@rbtcollins rbtcollins force-pushed the tokio-runtime branch 2 times, most recently from 3a86fbb to 6c2f960 Compare August 15, 2023 22:29
@rbtcollins
Copy link
Contributor Author

Having briefly skimmed over some parts of this: what limits us from just using #[tokio::main] and making things natively async, rather than all the callback stuff and block_on() type calls?

This is in draft mode at the moment because it is incomplete.

The block-on stuff is scaffolding during the transition.

The starting point was:
sync main()
...
reqwest blocking http client
tokio runtime invoked here

With an async main, that will blow up at runtime, because you get
tokio runtime
async main()
...
reqwest blocking http client
tokio runtime invoked here - panic

If that is changed naively by just making main() async, and reqwest async, then we get rust's async function tainting everything and will change all the relevant calls to async, but there are a number of places where types need to change to be compatible with async requirements; so the commit becomes huge and unwieldy very quickly.

So the approach I'm taking is some scaffolding that lets us work this incrementally, and once complete it will just be nice async code.

@djc
Copy link
Contributor

djc commented Aug 16, 2023

Makes sense, thanks for the explanation. Can you give an example of where/how types need to change? I worry that with this approach, you'll need another big set of commits to unwind all the block_on() stuff again after switching main. Instead, maybe it would be possible to use the top-down approach to isolate the required type changes in their own commits, and then jump directly to full async mode from there by adding async keywords all over the place?

@rbtcollins
Copy link
Contributor Author

Once done the aggregate diff should be precisely the changes needed, and I can rebase squash it all away. We did this with the removal of failure. Many many small steps, nice outcome :)

@djc
Copy link
Contributor

djc commented Aug 16, 2023

Oh, I guess that makes sense. Other projects I help maintain prefer to rebase-merge so that the small commits make it into main, which can be helpful if you need to chase down a regression later.

@rbtcollins
Copy link
Contributor Author

Thats also valid to do; lets assess when this is at the next inflection point (which will be async native with no thunk, but no refactorings for better behaviour)

@rbtcollins
Copy link
Contributor Author

This is nearly complete, I think its mostly mechanical now, except for the closures in the test suite - there's some lifetime challenges to address there

@rbtcollins rbtcollins force-pushed the tokio-runtime branch 4 times, most recently from c06eacb to a1ca230 Compare January 28, 2024 11:54
@rbtcollins rbtcollins requested review from rami3l and djc January 28, 2024 11:56
@rbtcollins rbtcollins marked this pull request as ready for review January 28, 2024 11:56
@rbtcollins
Copy link
Contributor Author

This now complete modulo any refactoring of the commit series we might want to do.

The key notes are:

  • we can't use tokio::main because it doesn't provide the per-worker-thread injection that we use to facilitate isolated tests. (tl;dr std::env::var/args being global reflect reality well, but not writing tests that interact with those layers).

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

This looks mostly okay to me.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
download/src/lib.rs Outdated Show resolved Hide resolved
download/src/lib.rs Outdated Show resolved Hide resolved
src/bin/rustup-init.rs Show resolved Hide resolved
src/cli/self_update.rs Outdated Show resolved Hide resolved
src/cli/self_update.rs Show resolved Hide resolved
src/cli/setup_mode.rs Outdated Show resolved Hide resolved
src/currentprocess.rs Outdated Show resolved Hide resolved
src/currentprocess.rs Outdated Show resolved Hide resolved
@rbtcollins rbtcollins force-pushed the tokio-runtime branch 2 times, most recently from 0c59e2b to 6927cc8 Compare April 10, 2024 22:19
@djc djc force-pushed the tokio-runtime branch from 83d61e5 to d14b3e8 Compare May 30, 2024 12:28
@djc djc enabled auto-merge May 30, 2024 12:30
@djc djc added this pull request to the merge queue May 30, 2024
Merged via the queue into rust-lang:master with commit 3ba08da May 30, 2024
22 checks passed
@rami3l
Copy link
Member

rami3l commented May 30, 2024

Thanks a lot for your hard work, @rbtcollins and @djc!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants