Add support for choosing between async-std and Tokio

[ilyvion]

Hello! 👋

I wanted to use this library, but all my async code is using Tokio as the runtime, so the fact that this library was using async-std meant that I couldn't use it. However, after investigating a little, I realized that for the two runtimes are nearly drop-in replaceable, so I added a small "compatibility-layer" module (named async_lib) to the project, which exports the currently selected runtime's primitives (and occasional shims, where required) so that the actual business logic code remains mostly the same as it did before.

While the code passes regular tests, I have not yet modified the documentation tests/examples, because I'm honestly not sure how to best ensure that the example uses the right runtime without it looking messy in the documentation. So the doctests currently only pass when async-std is selected as the runtime.

As for the benchmarks, dev-dependencies are not allowed to be optional, so I couldn't find a way to make it work for both runtimes, and so I've left the benchmarks only compatible with async-std for now.

I updated the README.md to state that Tokio is now supported, but I didn't specify how to pick the runtime, since I wasn't quite sure where to fit it in. I left async-std as the default runtime, and if a user wants tokio instead, they must do something like

cacache = { version = "*", default-features = false, features = ["tokio-runtime"] }

Let me know if you have any thoughts on what I should do with doctests and/or benchmarks if that is currently an issue for accepting this PR, as well as any other feedback you may have. ☺️

[zkat]

Wow, thanks for the PR! This is great and I'm definitely interested in making this Tokio-compatible, since that seems to be pretty popular, too.

Is making async_std optional really important? I'd rather not pick up a breaking change if I don't have to.

That said, if we're gonna do a breaking change, I'd rather port cacache to smol, which would be WAY more lightweight than async-std and it would be more acceptable to have it "always" pulled in, if that makes sense?

[zkat]

Is this necessary?

[ilyvion]

Combined with

[features]
default = ["async-std"]

it doesn't really change anything for your existing users unless they've put default-features = false on the dependency already, which would be a really odd thing to do on a crate which had no features.

[ilyvion]

I'm pretty sure that only one runtime can be pulled in at a time, since switching from one to the other means replacing all calls to async_std::task::spawn_blocking with tokio::task::spawn_blocking, e.g. That's why async-std has to be made optional, so it can be turned off when choosing Tokio instead.

At least with the way I did it (which was to move all that selecting code to a single module from which all the rest of the code calls them), only one runtime "feature" can be active at once, else you get a bunch of warnings about duplicate definitions:

error[E0252]: the name `read` is defined multiple times
  --> src\async_lib.rs:34:9
   |
31 | #[cfg(feature = "async-std")]
32 | pub use async_std::fs::read;
   |         ------------------- previous import of the value `read` here
33 | #[cfg(feature = "tokio")]
34 | pub use tokio::fs::read;
   |         ^^^^^^^^^^^^^^^ `read` reimported here
   |
   = note: `read` must be defined only once in the value namespace of this module

Incidentally, this approach should make it possible to support other runtimes too, assuming they provide the same facilities required as the others do. I haven't looked into smol very much, but one thing I noticed that could potentially be an issue at least is that there's no spawn_blocking function.

(Oh, also, that Clippy error from the CI comes from existing code that I hadn't touched, but I can add a quick commit to fix it if you'd like to make the CI happy.)

[zkat]

You can actually pull in multiple runtimes, though generally you only execute one at a time. Futures generated by libraries using one runtime's library functions can be used on the other runtime, and vice-versa, by using https://docs.rs/async-compat for bidirectional conversion.

The main catch of using async-compat is it adds a bit of noise to your code, but I've used it a lot and consider it relatively minor. The other concern is package size, which is why I'm thinking of switching cacache to use smol exclusively, and then direct folks towards async-compat for the rest.

Would this work for you as a solution? Sorry if I seem resistant, but I'm not sure how I feel about the added complexity of forcing users to specify feature flags, or putting out a semver-breaking change when async-compat already exists. Does that make sense? Happy to keep the conversation going, btw.

[ilyvion]

Oh, I didn't think to give async-compat a try. I'm not opposed to that solution if it works. I'll give that a shot, thanks for reminding me of its existence! Might take me a little while to get to though, but at least there's no rush about resolving this PR.

I understand your hesitation for sure, increased complexity is always something that should be weighed carefully against the potential benefits it brings, and if async-compat just works, then I agree that it probably isn't worth it.

I mean, there's the issue of documenting these new things (the fact that you can change runtime using feature flags) and potentially complicating doctests (which runtime should be used in them? Only the default one? (But then code in doctests only work for the default one) Both? (But how?) Not to mention the benchmarks; where you can't even have optional dependencies chosen by features. It's a bit of a mess, I see that. 😅

As for semver, like I said in the comment I left on your review comment, I don't think this would be a breaking change for any downstream user unless they have done something very unexpected (set default-features = false on the dependency) for no good reason, since I made sure to make the existing runtime be the one enabled by default.

[ilyvion]

So, finally having time to play around with this again, I thought I would, just for fun, try using cacache directly without the async-compat in-between, expecting something like the typical message you get when trying to use something that requires Tokio running:

thread 'main' panicked at 'not currently running on the Tokio runtime.'

Instead, this ran without issue. I'm not quite sure how this works, but... it just does.

[dependencies]
tokio = { version = "1.13", features = ["full"] }
cacache = "9"

#[tokio::main]
async fn main() {
    cacache::write("test", "hello", "world").await.unwrap();

    let list = cacache::list_sync("test");
    assert_eq!(list.count(), 1);
    let mut list = cacache::list_sync("test");
    assert_eq!(list.next().unwrap().unwrap().key, "hello");

    let value = String::from_utf8(cacache::read("test", "hello").await.unwrap()).unwrap();
    assert_eq!(value, "world");
}

I... don't know how to feel, lol.

[zkat]

oh wow. I guess Tokio is trying to be more compatible now or something???

[06chaynes]

Thought I'd leave a note on this that I've been using cacache in a trait with async methods supported via async-trait. I have one implementation of that trait that uses async-std and one that uses tokio, so far I haven't had any issues when using either runtime.

[zkat]

Closing in favor of #36, which builds on this work. Thanks again, @alexschrod!