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

make everything async #144

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

Conversation

gregdhill
Copy link
Contributor

@gregdhill gregdhill commented Oct 3, 2020

Signed-off-by: Gregory Hill gregorydhill@outlook.com

This still requires some work, but I wanted to open a PR to assess interest - perhaps this belongs in a separate repository.

Uses the reqwest library to perform async http requests and async-trait to support async traits.

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Copy link
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

Awesome, this has been on our wishlist for a while!

Some questions:

  • (1) did you consider contributing to the jsonrpc crate and implement async functionality there first as well?
  • (2) I'd like to keep using non-async as a possibility. How would you feel about having a compile feature and duplicating all methods, a sync and async variant? That would also make that we can keep the integration test simpler by keeping it sync.

client/src/client.rs Show resolved Hide resolved
@@ -22,25 +22,24 @@ pub struct RetryClient {
const INTERVAL: u64 = 1000;
const RETRY_ATTEMPTS: u8 = 10;

#[async_trait]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to drop this dependency by just writing the functions more verbose using Future?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it may be a little bit more involved, see: https://github.com/dtolnay/async-trait#explanation

Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
@gregdhill
Copy link
Contributor Author

  • (1) That would make sense, I will look into it.
  • (2) Possibly, of course any async method can be run synchronously. Is there a benefit to a distinct separation?

@sgeisler
Copy link
Contributor

sgeisler commented Oct 9, 2020

I'd like to keep using non-async as a possibility

@stevenroose BDK has implemented something like that (usage example). Maybe we could use that too?

@stevenroose
Copy link
Collaborator

I'd like to keep using non-async as a possibility

@stevenroose BDK has implemented something like that (usage example). Maybe we could use that too?

How does that work? Does it automatically add both async and sync methods on the traits?

@sgeisler
Copy link
Contributor

How does that work? Does it automatically add both async and sync methods on the traits?

To me it looks like they generate two versions of each function and activate only one depending on a feature flag. It's maybe a bit too much magic, idk. So you have to decide to go full async or not.

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.

4 participants