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

fix(provider): Add Send bound to return type of JsonRpcClient::request #2072

Merged

Conversation

wigy-opensource-developer
Copy link
Contributor

Motivation

#[async_trait] implementations require return types to be Send. So except for trivial cases the JsonRpcClient::request demanded implementors to be impossibly generic.

Solution

Add Send bound to the method and follow-up on its callers inside the monorepo.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • Breaking changes

@gakonst
Copy link
Owner

gakonst commented Jan 27, 2023

#[async_trait] implementations require return types to be Send. So except for trivial cases the JsonRpcClient::request demanded implementors to be impossibly generic.

Got it - could you please add a test to avoid regressions? cc @mattsse is this too restrictive?

@wigy-opensource-developer
Copy link
Contributor Author

Do you mean a test that breaks the build if someone removes this Send bound? I will, but they would probably remove the test as well then 😃

@gakonst
Copy link
Owner

gakonst commented Jan 27, 2023

Do you mean a test that breaks the build if someone removes this Send bound? I will, but they would probably remove the test as well then 😃

A test that would only compile if the Send bound is satisfied, i.e. a test that would fail on main but pass on this branch

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I think this is reasonable since we don't expect that the response type is not Send because it's also DeserializeOwned

@gakonst
Copy link
Owner

gakonst commented Jan 27, 2023

Hm I guess that makes sense? DeserializeOwned is owned, so it's probably going to be Send? Surprised they haven't done that themselves on the trait level? Are there any cases when that's not the case? Probably w/ unsafe?

@mattsse
Copy link
Collaborator

mattsse commented Jan 27, 2023

unsure, there's the rc feature which allows deserializing directly into an Rc which would no longer work after this PR, but imo this is perfectly fine.
I don't think there are any response types that are deserialized into a type !Send fields like Cell,Rc,etc.. so this should be okay imo.

RawValue is Send

https://docs.rs/serde_json/latest/serde_json/value/struct.RawValue.html

@gakonst
Copy link
Owner

gakonst commented Jan 27, 2023

SGTM - let's send it and keep an eye open.

@gakonst gakonst merged commit 7da559b into gakonst:master Jan 27, 2023
@wigy-opensource-developer wigy-opensource-developer deleted the fix/async_fn_returns_send branch January 28, 2023 05:26
@wigy-opensource-developer wigy-opensource-developer restored the fix/async_fn_returns_send branch January 28, 2023 05:29
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

3 participants