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

extract async client abstraction. #580

Merged
merged 28 commits into from
Dec 20, 2021
Merged

extract async client abstraction. #580

merged 28 commits into from
Dec 20, 2021

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Nov 29, 2021

Let's have a round of reviews for this too, it extracts the shareable parts of the ws client to an async client abstration.
I have called it core-client and it's not used by HTTP because it can't be extracted to a sending and receiving end.
That's we have two different transport traits for a sender and receiver such that they can be used in different tasks.

We can bikeshed on the naming....

Remaining is to expose the client transports from the wrapper crate jsonrpsee

I have tested it in subxt here

The benefit that it would be easy to add other transports such as IPC, web-sys and so on...

types/src/traits.rs Outdated Show resolved Hide resolved
@niklasad1 niklasad1 changed the title [WIP]: extract async client extract async client abstraction. Dec 15, 2021
@niklasad1 niklasad1 marked this pull request as ready for review December 15, 2021 17:20
@niklasad1 niklasad1 requested a review from a team as a code owner December 15, 2021 17:20
Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

Looks good to me, one question.

client/core-client/src/lib.rs Outdated Show resolved Hide resolved
///
/// You can also prevent the subscription being dropped by calling
/// [`Subscription::next()`](crate::types::Subscription) frequently enough such that the buffer capacity doesn't
/// exceeds.
Copy link
Member

@TarikGul TarikGul Dec 16, 2021

Choose a reason for hiding this comment

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

Suggested change
/// exceeds.
/// exceed its limit.

Im a little confused by frequently enough. Does this mean a user should monitor the subscriptions and as it starts to get close to exceeding its capacity to call [Subscription::next()], therefore the implementation on what frequently is, is based on the user?

Copy link
Member Author

@niklasad1 niklasad1 Dec 16, 2021

Choose a reason for hiding this comment

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

Maybe it's poorly phrased but it means that if the server produces items much faster than you would call Subscription::next() that might end up with the filling buffer and once it exceeds the configured max_limit that subscription is simply dropped...

In that scenario you need to either call/poll Subscription::next more often or increase the buffer max limit, k?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, okay yea that totally makes sense.

more often or increase the buffer max limit

This is what i assumed, just needed some clarification :). Thanks for the explanation

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

lgtm

client/core-client/Cargo.toml Outdated Show resolved Hide resolved
client/core-client/src/lib.rs Outdated Show resolved Hide resolved
client/core-client/src/lib.rs Outdated Show resolved Hide resolved
client/ws-client/src/lib.rs Outdated Show resolved Hide resolved
client/ws-client/src/lib.rs Outdated Show resolved Hide resolved
client/ws-client/src/lib.rs Outdated Show resolved Hide resolved
niklasad1 and others added 8 commits December 20, 2021 13:57
Co-authored-by: David <dvdplm@gmail.com>
Co-authored-by: David <dvdplm@gmail.com>
Co-authored-by: David <dvdplm@gmail.com>
Co-authored-by: David <dvdplm@gmail.com>
The `async-client` is hidden behind a new feature flag `async-client`
because it brings in additional dependecies such as tokio rt.
///
/// ```
#[derive(Clone, Debug)]
pub struct WsClientBuilder<'a> {
Copy link
Member Author

Choose a reason for hiding this comment

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

this crate could be removed but I kept for it may be easier to use than to use the core client and plug in the actual transport.

@@ -37,6 +37,14 @@ server = [
"tokio",
]
client = ["futures-util"]
async-client = [
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to introduce a feature to avoid bring in tokio and some other deps for http

let addr = run_server().await?;
let uri: Uri = format!("ws://{}", addr).parse()?;

let client: Client = WsTransportClientBuilder::default().build(uri).await?.into();
Copy link
Member Author

Choose a reason for hiding this comment

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

example, do we want this?

http-server/Cargo.toml Outdated Show resolved Hide resolved
@niklasad1
Copy link
Member Author

@maciejhirsz @dvdplm @TarikGul

can take you a quick look on this again after some changes?
if no one objects I'll merge it then :)

client/transport/Cargo.toml Outdated Show resolved Hide resolved
@dvdplm dvdplm merged commit 292bd88 into master Dec 20, 2021
@dvdplm dvdplm deleted the extract-async-client branch December 20, 2021 17:08
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.

None yet

4 participants