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

✨ add support for async clients #147

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

Shahab96
Copy link
Contributor

@Shahab96 Shahab96 commented Apr 10, 2024

resolves #20

While we use Tokio for testing, this will support other runtimes too.

Summary by CodeRabbit

  • New Features

    • Added asynchronous functionality for interacting with a memcached server.
    • Introduced an AsyncClient with async-specific methods for asynchronous operations.
  • Enhancements

    • Updated the tokio dependency to version 1.37 with additional features for improved performance and functionality.
  • Documentation

    • Publicly exposed new asynchronous methods and types for easier integration and usage by developers.

While we use Tokio for testing, this will support other runtimes too.
Copy link
Contributor

coderabbitai bot commented Apr 10, 2024

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

File(s) Change Summary
Cargo.toml Added async feature and updated tokio to v1.37 with additional features.
src/client.rs, src/async_client.rs Introduced AsyncClient and async methods for memcached operations.
src/lib.rs Added async_client module and connect_async function; AsyncClient type is now exported with the async feature.
.github/workflows/ci.yaml Updated GitHub Actions to test with --all-features.
tests/test_ascii.rs, tests/tests.rs Enhanced tests to accommodate async operations and added new test cases for async functionalities.

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!


Recent Review Details

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between c9a651f and 864cc89.
Files selected for processing (1)
  • .github/workflows/ci.yaml (1 hunks)
Additional comments not posted (2)
.github/workflows/ci.yaml (2)

36-36: Adding a test run without the async feature ensures backward compatibility. Good practice!


43-50: Running tests with the --all-features flag is essential to ensure that the new async functionality integrates well with the existing codebase. Well done!


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Shahab96
Copy link
Contributor Author

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.

@Shahab96
Copy link
Contributor Author

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.

@@ -40,6 +40,14 @@ jobs:
env:
CARGO_INCREMENTAL: 0
RUSTFLAGS: "-Zprofile -Ccodegen-units=1 -Cinline-threshold=0 -Clink-dead-code -Coverflow-checks=off"
- name: Run tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1 test with sync and 1 with async

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

src/client.rs Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
src/client.rs Show resolved Hide resolved
@@ -12,11 +12,13 @@ edition = "2018"
[features]
default = ["tls"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will leave async off by default, as turning it on would break users.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

tests/test_ascii.rs Show resolved Hide resolved
@Shahab96
Copy link
Contributor Author

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

@aisk
Copy link
Owner

aisk commented Apr 12, 2024

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

@aisk
Copy link
Owner

aisk commented Apr 12, 2024

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.

@Shahab96
Copy link
Contributor Author

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.

@Shahab96
Copy link
Contributor Author

Shahab96 commented Apr 12, 2024

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.

@aisk
Copy link
Owner

aisk commented Apr 13, 2024

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.

@Shahab96
Copy link
Contributor Author

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.

@Shahab96
Copy link
Contributor Author

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.

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.

support tokio as event loop
2 participants