Skip to content

jsonrpc: Add async support#558

Open
jamillambert wants to merge 4 commits intorust-bitcoin:masterfrom
jamillambert:0417-jsonrpc-async
Open

jsonrpc: Add async support#558
jamillambert wants to merge 4 commits intorust-bitcoin:masterfrom
jamillambert:0417-jsonrpc-async

Conversation

@jamillambert
Copy link
Copy Markdown
Collaborator

Pulled out of #505 and polished.

The aim is to add async support without breaking existing sync downstream. This includes keeping the reexports of the current sync Client at the crate root.

Code copy only to make it easier in the next patch to see what the
changes are for async.
Remove code related to fuzzing from the new async files. Fuzzing of the
async client can be added back later if needed.
In preparation for adding another async client change the Debug impl to
refer to the local sync Client so that there is no conflict when adding
the second async Client struct.
Copy link
Copy Markdown
Contributor

@satsfy satsfy left a comment

Choose a reason for hiding this comment

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

tACK 5906ce1 - Ran some async tests locally with the new client.


impl BitreqHttpTransport {
/// Constructs a new [`BitreqHttpTransport`] with default parameters.
pub fn new() -> Self { BitreqHttpTransport::default() }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a comment. Should we consider some validation here? So that the user cannot shoot themselves on the foot like:

use jsonrpc::bitreq_http_async::Builder;
Builder::new().url("not_a_url").is_ok();

I suppose a validation could happen at new or at fn build(self). No change required, because sync client also does it too. But it's a thought for production.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Raise an issue man, your thoughts are valuable and will be lost if left here!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Worth raising an issue. But not_a_url is a valid URL. I'm not sure what validation would be reasonable.

Copy link
Copy Markdown
Contributor

@satsfy satsfy Apr 18, 2026

Choose a reason for hiding this comment

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

Thanks I'll put it on the production client's existing issue as an additional consideration.

@satsfy have you spent much time with async code?

@tcharding I don't have enough async experience for a decisive review.

Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 5906ce1 - but I know nothing about async code.

@tcharding
Copy link
Copy Markdown
Member

Since we are going to cut a release that includes code I know nothing about (async) I'd love to see PRs up that test everything before we release. Would you consider doing:

  • A release tracking PR for jsonrpc (bumps version and updates local crates that depend on it).
  • Rebase the prod RPC client and use the branch/PR created above

Then with green CI on those two PRs we'd have confidence we got things right and we can go ahead and cut the releases.

And if someone with async experience has ten minutes to cast an eye over 5906ce1 please that would be very much appreciated.

@tcharding
Copy link
Copy Markdown
Member

@satsfy have you spent much time with async code?

@apoelstra
Copy link
Copy Markdown
Member

I have a reasonable amount of experience with async. I'll take a look.

pub fn builder() -> Builder { Builder::new() }

fn request<R>(&self, req: impl serde::Serialize) -> Result<R, Error>
/// Returns the timeout in whole seconds, rounding positive sub-second values up to one.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In 5906ce1:

This is fine, but shouldn't it be done in a separate commit and be done for the sync version too?

&'a self,
req: Request<'a>,
) -> BoxFuture<'a, Result<Response, crate::Error>> {
Box::pin(async move { Ok(self.request(req).await?) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In 5906ce1:

This mess can be replaced with Box::pin(self.request(req))

&'a self,
reqs: &'a [Request<'a>],
) -> BoxFuture<'a, Result<Vec<Response>, crate::Error>> {
Box::pin(async move { Ok(self.request(reqs).await?) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In 5906ce1:

...and this one with Box::pin(self.request(reqs))

fn send_request(&self, _: Request) -> Result<Response, Error> { Err(Error::NonceMismatch) }
fn send_batch(&self, _: &[Request]) -> Result<Vec<Response>, Error> { Ok(vec![]) }
fn send_request<'a>(&'a self, _: Request<'a>) -> BoxFuture<'a, Result<Response, Error>> {
Box::pin(async { Err(Error::NonceMismatch) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In 5906ce1:

This is just a style thing but you can write Box::pin(futures::err(Error::NonceMismatch)) here. It might even compile faster or more efficiently.

&'a self,
_: &'a [Request<'a>],
) -> BoxFuture<'a, Result<Vec<Response>, Error>> {
Box::pin(async { Ok(vec![]) })
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In 5906ce1:

Similarly, futures::ok(vec![])

pub fn send_request(&self, request: Request) -> Result<Response, Error> {
self.transport.send_request(request)
pub async fn send_request(&self, request: Request<'_>) -> Result<Response, Error> {
self.transport.send_request(request).await
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In 5906ce1:

It might not be worth the abstraction leak, but you can change this method to return a BoxFuture and then you don't need async or await (and the resulting code will be smaller and faster to compile since it saves you a dummy state machine).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants