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

feature(async): Add tokio as an executor option #36

Merged
merged 3 commits into from
Jan 28, 2023

Conversation

ceejbot
Copy link
Contributor

@ceejbot ceejbot commented Jan 25, 2023

This PR is based on the work @alexschrod did in PR #29. All I did was carry it over the finish line.

This PR adds a feature to the crate named tokio-runtime. If you disable default features and enable this new one, cacache uses tokio as its async executor. This makes integrating cacache with tokio-using projects easier, because the file types leak out if you use anything more than the top-level convenience functions.

The PR implements the feature using shims in a new submodule named async_lib. This module conditionally uses either async-std or tokio based on feature selection, and hides some differences with convenience functions.

This change should not be a breaking change, because the default is still async-std.

There are a few other small changes in this PR worth noting.

  • The README shows how to switch runtimes.
  • There's a justfile to run common tasks, including those in makefile.toml. The default shell is sh, so this might not work out of the box for Windows users.
  • The tests can now run under either runtime. The justfile has a recipe that runs them both.
  • The benchmarks can also run under either runtime. The justfile has two recipe for this, one using bench and the other using criterion's runner.
  • The dependencies now pull in async-attributes by default along with async-std. This made it easier to swap runtimes in the tests.
  • All dependency versions have been bumped.

Co-authored-by: @alexschrod

ilyvion and others added 2 commits January 25, 2023 12:56
This PR is based on the work @alexschrod did in PR zkat#29. All
I did was carry it over the finish line.

This PR adds a feature to the crate named `tokio-runtime`. If
you disable default features and enable this new one, cacache
uses tokio as its async executor. This makes integrating cacache
with tokio-using projects easier, because the file types leak out
if you use anything more than the top-level convenience functions.

The PR implements the feature using shims in a new submodule named
`async_lib`. This module conditionally uses either async-std
or tokio based on feature selection, and hides some differences with
convenience functions.

This change should not be a breaking change, because the default is
still async-std.

There are a few other small changes in this PR worth noting.

- The README shows how to switch runtimes.
- There's a justfile to run common tasks, including those in makefile.toml.
  The default shell is `sh`, so this might not work out of the box for
  Windows users.
- The tests can now run under either runtime. The justfile has a recipe
  that runs them both.
- The benchmarks can also run under either runtime. The justfile has two
  recipe for this, one using bench and the other using criterion's runner.
- The dependencies now pull in async-attributes by default along with
  async-std. This made it easier to swap runtimes in the tests.
- All dependency versions have been bumped.

Co-authored-by: @alexschrod
Copy link
Owner

@zkat zkat 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 good! Just a couple of comments/questions and I think we're good to merge! (assuming CI is happy).

Cargo.toml Show resolved Hide resolved
"io-util",
"macros",
"rt",
"rt-multi-thread",
Copy link
Owner

Choose a reason for hiding this comment

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

correct me if I'm wrong, but we don't need rt and rt-multi-threaded unless we're running benchmarks, right? Should we have those features as dev-deps? Is that even possible?

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been researching this one and I think it might not be possible right now. See rust-lang/cargo#5133 (comment)

#[cfg(feature = "tokio")]
#[inline]
pub fn unwrap_joinhandle_value<T>(value: Result<T, tokio::task::JoinError>) -> T {
value.unwrap()
Copy link
Owner

Choose a reason for hiding this comment

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

When do we expect this unwrap to actually fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a shim @alexschrod added to paper over a difference between Tokio's join (which is fallible) and async-std's (which is not). I am not sure what to do here or if it in practice it will ever happen.

src/content/write.rs Outdated Show resolved Hide resolved
(I have this exact urge too but missed this one)
@zkat zkat merged commit e34dcfd into zkat:main Jan 28, 2023
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