Provide async support for direct request/response communication #521

Niederb · 2023-04-05T10:35:29Z

Provide async support for direct request/response communication
- This means communication based on the request() method
- Subscription based communication is still to do
Use the maybe-async crate to provide a sync and async API with little changes in the code
Is breaking backwards compatibility because compilation with default-features=false now defaults to compiling the api in async mode
- This is because the default mode of maybe-async is async
Is a first step towards Support async calls #278

Niederb · 2023-04-11T07:01:33Z

src/api/api_client.rs

+		let metadata_future = Self::get_metadata(&client);
+		let runtime_version_future = Self::get_runtime_version(&client);
+
+		let (genesis_hash, metadata, runtime_version) = futures::future::try_join3(


Here I'm not quite sure: On the one hand it's nice to actually take advantage of async by awaiting the futures in parallel. On the other hand the function is duplicated because there is a second implementation for the sync case.

I think that's fine. The async improvement is worth the small duplication.

src/api/api_client.rs

Niederb · 2023-04-11T07:02:47Z

src/lib.rs

@@ -16,6 +16,7 @@
 */
 #![cfg_attr(not(feature = "std"), no_std)]
 #![feature(error_in_core)]
+#![feature(async_fn_in_trait)]


Using async in traits is not available on stable yet

Niederb · 2023-04-11T07:47:51Z

@haerdib This is not finished yet, but can you have a first look?
Most of it is boilerplate of converting functions to async and using await.
Some thoughts (also see comments):

We need a new nightly feature to have support for async in traits
It would be nice to use features from futures::future but this needs std
I'm not sure what the situation is for executor support in no_std. For example in wasm. I'm just wondering if we limit our potential use-cases by switching to async.

haerdib

I'm not sure what the situation is for executor support in no_std. For example in wasm. I'm just wondering if we limit our potential use-cases by switching to async.

This would certainly need thorough investigation. But instead of switching completely to async, have you considered providing both, async and sync client? That would solve the question if it's available in wasm and such - if it's not, one can simply use the sync client.

Ofc, that would need quite some thought on how to provide two clients without having to duplicate everything.

Niederb · 2023-04-11T13:23:09Z

I'm not sure what the situation is for executor support in no_std. For example in wasm. I'm just wondering if we limit our potential use-cases by switching to async.

This would certainly need thorough investigation. But instead of switching completely to async, have you considered providing both, async and sync client? That would solve the question if it's available in wasm and such - if it's not, one can simply use the sync client.

Ofc, that would need quite some thought on how to provide two clients without having to duplicate everything.

It certainly would be nice if we can support both use cases sync and async without duplicating all the code.
Our problem is basically described here nicely: https://blog.rust-lang.org/inside-rust/2022/07/27/keyword-generics.html#memories-of-the-present-async-today
Except in our case the block_on is also unfortunate, because this typically this is problematic for no_std.
maybe-async looks quite interesting though. It seems to "remove" the async/await keywords completely and I think then we could manage without block_on

Niederb · 2023-04-13T08:12:37Z

I looked a bit more into maybe-async. It uses the async_trait crate to support async on traits (instead of the nightly feature).
- I opened an issue asking about support for nightly: Async traits implementation from nightly fMeow/maybe-async-rs#17
- On the plus side it automatically add the async_trait attribute so we don't need two different attributes
The nightly implementation currently does not support dynamic dispatch which seems quite limiting to me so we might be better of using async-trait anyway.

compose-macros/src/lib.rs

Niederb · 2023-04-13T13:45:31Z

Still not finished but I think it is closer to what we want.
The approach is now quite different:

Use maybe-async and async-trait crates instead of nightly implementation.
- This allows us to provide a sync and an async API by changing a compiler flag
- Examples compile without changes
The only difference between async and sync is in JsonrpseeClient::request. There is now a separate implementation for each.
maybe-async does not work with macros, so I had to take the nonce out of the macro. This is not so nice.
@haerdib What do you think?

haerdib

Examples compile without changes

Does that mean that this PR does not introduce any breaking changes? E.g. the users do not need to add the #[maybe_async::maybe_async(?Send)] in case they use the good old sync variant?

Cargo.toml

compose-macros/src/lib.rs

Niederb · 2023-04-17T08:56:42Z

Examples compile without changes

Does that mean that this PR does not introduce any breaking changes? E.g. the users do not need to add the #[maybe_async::maybe_async(?Send)] in case they use the good old sync variant?

Yes, at least it's my understanding that as long as we compile maybe-async with features = ["is_sync"] then all the async and await will basically be removed by the macro. Therefore it compiles as before.

…h async and the other one sync

# Conflicts: # Cargo.lock # src/api/rpc_api/author.rs # src/api/rpc_api/chain.rs # src/api/rpc_api/events.rs # src/api/rpc_api/frame_system.rs # src/api/rpc_api/pallet_transaction_payment.rs # src/api/rpc_api/state.rs

# Conflicts: # src/api/rpc_api/chain.rs

Niederb · 2023-04-19T10:44:01Z

I think this is now ready for merging. The nonce handling is now the same as before.
I created a follow up task for implementing an example #543

flipchan · 2023-04-19T13:13:19Z

@Niederb awesome new feature! is this stable?

haerdib

Absolutely awesome, thanks a lot!

One thing: Could you rename the PR to a little more meaningful name and link the corresponding issue (though not close it I guess because Subscription is not yet async)

haerdib · 2023-04-19T13:11:18Z

Cargo.toml

@@ -24,11 +24,13 @@ members = [

 [dependencies]
 # crates.io no_std
+async-trait = "0.1.68"


Still needed?

Yes, it is a transitive dependency from maybe-async and it does not compile without it. Dependencies for macros is a bit of a mystery for me...

Interesting. Thanks for the explanation!

Cargo.toml

compose-macros/src/lib.rs

haerdib · 2023-04-19T13:13:02Z

compose-macros/src/lib.rs

 	$module: expr,
 	$call: expr
 	$(, $args: expr) *) => {
 		{
-            use $crate::log::debug;
+			use $crate::log::debug;


Is that indent correct?

No, I fixed it now. Seems cargo fmt works differently in macros. Interesting

compose-macros/src/lib.rs

haerdib · 2023-04-19T13:17:13Z

src/api/api_client.rs

+		let metadata_future = Self::get_metadata(&client);
+		let runtime_version_future = Self::get_runtime_version(&client);
+
+		let (genesis_hash, metadata, runtime_version) = futures::future::try_join3(


I think that's fine. The async improvement is worth the small duplication.

Co-authored-by: Bigna Härdi <73821294+haerdib@users.noreply.github.com>

Niederb · 2023-04-19T13:51:36Z

@Niederb awesome new feature! is this stable?

@flipchan Thanks! I'm not quite sure what you mean by stable? I hope this PR will be merged very soon and then async will be available for the more simple communication that uses a single request/response (more complex communication based on subscriptions is not done yet).
substrate-api-client needs a nightly rust compiler because of other features we need.

Niederb · 2023-04-19T13:59:07Z

Absolutely awesome, thanks a lot!

One thing: Could you rename the PR to a little more meaningful name and link the corresponding issue (though not close it I guess because Subscription is not yet async)

Done, but linking without closing is apparently not supported. See https://github.com/orgs/community/discussions/17308
I guess we can just repopen the issue after the merge 😉

haerdib · 2023-04-19T14:11:03Z

My workaround for that was to simply add a link in the PR description. E.g. something like "related to #278".

haerdib

👏

haerdib · 2023-04-19T14:11:31Z

Cargo.toml

@@ -24,11 +24,13 @@ members = [

 [dependencies]
 # crates.io no_std
+async-trait = "0.1.68"


Interesting. Thanks for the explanation!

echevrier

Thanks a lot. Really nice!

Niederb commented Apr 11, 2023

View reviewed changes

src/api/api_client.rs Show resolved Hide resolved

Niederb commented Apr 11, 2023

View reviewed changes

Niederb requested a review from haerdib April 11, 2023 07:48

Niederb self-assigned this Apr 11, 2023

haerdib reviewed Apr 11, 2023

View reviewed changes

Niederb added 5 commits April 13, 2023 14:23

Only make the request method async without breaking external APIs

80bf9e5

Make all the functions async and use await

368028c

Make futures non-optional for now

4caaad7

Use the maybe_async crate to support async and sync APIs

e32b7ac

Fix Cargo.toml formatting

1260997

Niederb force-pushed the tn/async-experiment branch from 6add742 to 1260997 Compare April 13, 2023 12:24

Niederb commented Apr 13, 2023

View reviewed changes

compose-macros/src/lib.rs Show resolved Hide resolved

Revert changes to futures crate config

ac270e9

Niederb requested a review from haerdib April 13, 2023 13:46

haerdib reviewed Apr 17, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

compose-macros/src/lib.rs Show resolved Hide resolved

Put comments back in

f642b75

Niederb added 8 commits April 18, 2023 11:58

Provide two implementations of the compose_extrinsic macro: One wit…

2a809e0

…h async and the other one sync

Merge branch 'master' into tn/async-experiment

d3c6189

# Conflicts: # Cargo.lock # src/api/rpc_api/author.rs # src/api/rpc_api/chain.rs # src/api/rpc_api/events.rs # src/api/rpc_api/frame_system.rs # src/api/rpc_api/pallet_transaction_payment.rs # src/api/rpc_api/state.rs

Fix merge

2737a77

Fix taplo formatting

07457d7

Provide two functions that take advantage of async

8954b70

Extract shared logic between sync and async into a separate macro

4f99a94

Merge branch 'master' into tn/async-experiment

ea07799

# Conflicts: # src/api/rpc_api/chain.rs

Fix async compilation and provide features flags

e9631d9

Niederb added 5 commits April 19, 2023 10:51

Try again with single quotes

f75b4ce

Rename feature to sync-api and try fix to quoting issue in ci.yml

4a5813d

Try yet again to make github action happy

7e4bf3c

Set feature sync-api for the no_std tests

c8fb72f

Improve documentation

72dc009

Niederb added the E2-breaksapi label Apr 19, 2023

Niederb marked this pull request as ready for review April 19, 2023 10:38

Niederb requested review from haerdib and echevrier April 19, 2023 10:44

haerdib reviewed Apr 19, 2023

View reviewed changes

Apply suggestions from code review

02e124f

Co-authored-by: Bigna Härdi <73821294+haerdib@users.noreply.github.com>

Niederb changed the title ~~Experiment with async~~ Provide async support for direct request/response communication Apr 19, 2023

Fix formatting

b410e95

Niederb linked an issue Apr 19, 2023 that may be closed by this pull request

Support async calls #278

Closed

Niederb requested a review from haerdib April 19, 2023 13:59

haerdib approved these changes Apr 19, 2023

View reviewed changes

haerdib added the F8-newfeature Introduces a new feature label Apr 19, 2023

Niederb removed a link to an issue Apr 20, 2023

Support async calls #278

Closed

Niederb mentioned this pull request Apr 20, 2023

Support async calls #278

Closed

echevrier approved these changes Apr 20, 2023

View reviewed changes

Niederb merged commit 303eceb into master Apr 20, 2023

Niederb deleted the tn/async-experiment branch April 20, 2023 08:07

haerdib mentioned this pull request Apr 26, 2023

Support async for subscriptions #549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide async support for direct request/response communication #521

Provide async support for direct request/response communication #521

Niederb commented Apr 5, 2023 •

edited

Loading

Niederb Apr 11, 2023 •

edited

Loading

haerdib Apr 19, 2023

Niederb Apr 11, 2023

Niederb commented Apr 11, 2023

haerdib left a comment

Niederb commented Apr 11, 2023

Niederb commented Apr 13, 2023 •

edited

Loading

Niederb commented Apr 13, 2023 •

edited

Loading

haerdib left a comment

Niederb commented Apr 17, 2023

Niederb commented Apr 19, 2023

flipchan commented Apr 19, 2023

haerdib left a comment

haerdib Apr 19, 2023

Niederb Apr 19, 2023

haerdib Apr 19, 2023

haerdib Apr 19, 2023

Niederb Apr 19, 2023

haerdib Apr 19, 2023

Niederb commented Apr 19, 2023

Niederb commented Apr 19, 2023

haerdib commented Apr 19, 2023

haerdib left a comment

haerdib Apr 19, 2023

echevrier left a comment

Provide async support for direct request/response communication #521

Provide async support for direct request/response communication #521

Conversation

Niederb commented Apr 5, 2023 • edited Loading

Niederb Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niederb commented Apr 11, 2023

haerdib left a comment

Choose a reason for hiding this comment

Niederb commented Apr 11, 2023

Niederb commented Apr 13, 2023 • edited Loading

Niederb commented Apr 13, 2023 • edited Loading

haerdib left a comment

Choose a reason for hiding this comment

Niederb commented Apr 17, 2023

Niederb commented Apr 19, 2023

flipchan commented Apr 19, 2023

haerdib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niederb commented Apr 19, 2023

Niederb commented Apr 19, 2023

haerdib commented Apr 19, 2023

haerdib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echevrier left a comment

Choose a reason for hiding this comment

Niederb commented Apr 5, 2023 •

edited

Loading

Niederb Apr 11, 2023 •

edited

Loading

Niederb commented Apr 13, 2023 •

edited

Loading

Niederb commented Apr 13, 2023 •

edited

Loading