Walkthrough

The project has embraced asynchronous capabilities, enhancing a memcached client library with tokio for async operations. This update introduces an AsyncClient, enriches async methods in client.rs, and refines testing to include async scenarios. These changes bolster the library's adaptability and efficiency in managing concurrent tasks.

Changes

Poem

🐇✨
In the realm of code and byte,
A rabbit leapt through async's might.
With tokio stars and await moon,
It waltzed to the memcached tune.
🚀🌟
Now swifter than ever, our cache does zoom!

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

I may have gone a bit overboard with this one? Let me know. Essentially having the async feature flag turned on makes everything async by default and the blocking client must be explicitly accessed by calling .blocking() on the async client.

Additionally, all doc comments and doctests are updated based on whether or not the async feature flag is active. Users will get different tooltips in their autocomplete as well as different code out of cargo doc depending on whether the flag is on or not.

If this is too excessive, the implementation that allows both blocking and non blocking clients at the same time is up to 11eb050

I think we can add the new nonblock test to a new file, to avoid the change to exist codes.

Sorry, I read the code, but found that the newly added async client is just a set of wrapper async methods, which will call the existing non-async client's methods.

I'm not sure if my understanding is correct, but if so, this async implementation isn't useful. It will block the event loop (Tokio or other components), and there is no difference in using it with the existing non-async client under the Tokio runtime.

In most languages that have async features like Rust, async will spread from IO syscalls to top-level user function calls. If we want to fully utilize the async feature, we need to call the TCP functions provided by Tokio.

Sorry, I read the code, but found that the newly added async client is just a set of wrapper async methods, which will call the existing non-async client's methods.

I'm not sure if my understanding is correct, but if so, this async implementation isn't useful. It will block the event loop (Tokio or other components), and there is no difference in using it with the existing non-async client under the Tokio runtime.

In most languages that have async features like Rust, async will spread from IO syscalls to top-level user function calls. If we want to fully utilize the async feature, we need to call the TCP functions provided by Tokio.

Correct, the new code simply wraps the synchronous code in an async block. The goal here is to support async executors (not only tokio, but also async-std for example).

The code would not block the event loop however, unless the user just calls .await on every function, which would cause the runtime to block until the future has returned the ready state (this would happen regardless of whether or not we used tokio::net, as this is the behavior of executors since they can only see the top level future and not the futures contained within those).

However this does allow us to eventually add more functionality within the protocol and use the tokio::net primitives for example, though I didn't include that functionality in this as I didn't want to make the library only compatible with tokio.

The intention behind this PR is only to provide a client which returns futures for all of it's calls in such a way that allows us to incrementally improve the underlying functionality without needing to make large sweeping prs to do so, and also without locking compatibility to only one executor type.

I plan to slowly add more functionality to the protocol trait implementations to allow tokio as the default executor and use tokio::net, while also allowing feature flags to use other executors and adding the changes to do so as part of those PRs.

Sorry I didn't clarify this in the PR description.

Tl;dr
This PR will only add async blocks so we can return futures, and there will be more to come in future PRs.

I think we can add the new nonblock test to a new file, to avoid the change to exist codes.

I can add this sometime next week, I will be busy until April 22nd so I may not be able to complete this until then. I don't want to block the other PR however so it may be best to merge that and I will take care of any conflicts when I am back.

The code would not block the event loop however, unless the user just calls .await on every function

No, the runtime will be blocked by the self.inner.get or other method even if no there is no .await. This is just like calling std::thread::sleep in an async function.

I plan to slowly add more functionality to the protocol trait implementations to allow tokio as the default executor and use tokio::net, while also allowing feature flags to use other executors and adding the changes to do so as part of those PRs.

Ok, I think I've got it, I totally agree that we should split a large PR into some small PRs and finish the feature gradually.

But I think we should start the process from bottom to top, not from top to bottom. Some API design is affected by the underlying runtime, so we should build it around the runtime.

The code would not block the event loop however, unless the user just calls .await on every function

No, the runtime will be blocked by the self.inner.get or other method even if no there is no .await. This is just like calling std::thread::sleep in an async function.

I plan to slowly add more functionality to the protocol trait implementations to allow tokio as the default executor and use tokio::net, while also allowing feature flags to use other executors and adding the changes to do so as part of those PRs.

Ok, I think I've got it, I totally agree that we should split a large PR into some small PRs and finish the feature gradually.

But I think we should start the process from bottom to top, not from top to bottom. Some API design is affected by the underlying runtime, so we should build it around the runtime.

If you prefer for us to start with the tokio net structs we can, but it will be a larger pr as I will need to make an async version of ProtocolTrait and the network stack as well. We can do that if you prefer, however if we do then we won't be runtime agnostic by default but rather tokio by default. That's fine as long as we document it though, and consumers who want async-std will have to wait for the feature flag.

An easy workaround for now is I can change the wrappers to call tokio::spawn_blocking and return the join handle. That way it'll be async while also letting tokio know to use the blocking thread pool, and also let us keep the client facing interface in place while we make changes to the internals and improve over time.

Let me know what you think.

I did some initial looking and making the underlying streams use tokio will be a very very large pr. There are also going to be some differences in the interfaces as well, eg tokio TcpStream does not have a set_read_timeout or set_write_timeout function, rather just has a set_ttl function. I'm sure there are more too.

Yes, support async intend to be a big project.