Remove Unpin requirement from Transport #399
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Here I was unable to convert SubscriptionStream to `impl Stream` because it has additional methods and runs code on drop. To get rid of the Unpin requirement anyway we use pin_project (https://docs.rs/pin-project/1.0.1/pin_project/).
e00E
commented
Oct 30, 2020
| @@ -20,10 +24,10 @@ impl<A, B, AOut, BOut> Transport for Either<A, B> | |||
| where | |||
| A: Transport<Out = AOut>, | |||
| B: Transport<Out = BOut>, | |||
| AOut: futures::Future<Output = error::Result<rpc::Value>> + Unpin + 'static, | |||
| BOut: futures::Future<Output = error::Result<rpc::Value>> + Unpin + 'static, | |||
| AOut: futures::Future<Output = error::Result<rpc::Value>> + 'static + Send, | |||
There was a problem hiding this comment.
I think previously EitherTransport did not require Send but now it does because of BoxFuture. I wasn't able to get the types to work out otherwise. Technically one could duplicate this code and use LocalBoxFuture to get rid of the Send requirement.
There was a problem hiding this comment.
IMHO acceptable trade-off for async code for now.
tomusdrw
approved these changes
Nov 1, 2020
| @@ -20,10 +24,10 @@ impl<A, B, AOut, BOut> Transport for Either<A, B> | |||
| where | |||
| A: Transport<Out = AOut>, | |||
| B: Transport<Out = BOut>, | |||
| AOut: futures::Future<Output = error::Result<rpc::Value>> + Unpin + 'static, | |||
| BOut: futures::Future<Output = error::Result<rpc::Value>> + Unpin + 'static, | |||
| AOut: futures::Future<Output = error::Result<rpc::Value>> + 'static + Send, | |||
There was a problem hiding this comment.
IMHO acceptable trade-off for async code for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is based on the other PR for eth_subscribe so it includes that commit too. After making use of pin-project there I realized I could do this directly for CallFuture and thus make Transport no longer require Unpin even without removing all of the uses of CallFuture. There are still uses of Unpin in a couple of tests, http.rs and ws.rs . It looks like those are benign because everything still compiles but I am a little bit wary that this might have some unexpected consequence.