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.

Sign up for GitHub

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

Jump to bottom

Make ResponseFuture implement Sync #2653

Merged
merged 2 commits into from
Oct 18, 2021
Merged

Make ResponseFuture implement Sync #2653

merged 2 commits into from
Oct 18, 2021

Conversation

Darksonn
Copy link
Contributor

By using the SyncWrapper utility we can make ResponseFuture implement the Sync trait. The fact that it doesn't currently can propagate up through many layers of types and cause annoying compilation errors about things that are not really an issue.

@chanks
Copy link

chanks commented Sep 21, 2021

(Chiming in because the last time this came up @seanmonstar asked for a specific use case - I don't feel qualified to comment on the actual diff)

It'd help me out as well if ResponseFuture was Sync - in my specific case I have a struct that contains a Client and is responsible for receiving a Request, mutating it, and then passing it to the Client to be executed. What I'd like to be able to do is swap out the actual transmission step in order to test the struct's behavior. I'm thinking that instead of giving it a Client I'd just give it an closure that takes a Request<Body> and returns a future returning a Result<Response<Body>, hyper::Error>.

Right now, though, with ResponseFuture not being Sync I'm having trouble getting that to work - I get errors like:

error: future cannot be shared between threads safely
   |
37 |             inner: Arc::new(move |req| Box::pin(async move { client.request(req).await })),
   |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future created by async block is not `Sync`
   |
   = help: the trait `std::marker::Sync` is not implemented for `(dyn futures::Future<Output = std::result::Result<hyper::Response<hyper::Body>, hyper::Error>> + std::marker::Send + 'static)`
note: future is not `Sync` as it awaits another future which is not `Sync`
   |
37 |             inner: Arc::new(move |req| Box::pin(async move { client.request(req).await })),
   |                                                              ^^^^^^^^^^^^^^^^^^^ await occurs here on type `hyper::client::ResponseFuture`, which is not `Sync`
   = note: required for the cast to the object type `dyn futures::Future<Output = std::result::Result<hyper::Response<hyper::Body>, hyper::Error>> + std::marker::Send + std::marker::Sync`

(It's totally possible there's an easier way to do this that I've missed)

@Darksonn Darksonn force-pushed the response-future-sync branch 2 times, most recently from 87be2e0 to 64a2125 Compare October 15, 2021 13:17
@seanmonstar seanmonstar merged commit bd6c35b into hyperium:master Oct 18, 2021
@Darksonn Darksonn deleted the response-future-sync branch October 18, 2021 19:45
ramosbugs added a commit to ramosbugs/oauth2-rs that referenced this pull request Mar 10, 2024
@ramosbugs
Return impl Future instead of Pin<Box<dyn Future>>
71d653b 
Returning a boxed trait object requires specifying whether the returned
future is `Send`/`Sync`, which means we can't support both `Send` and
non-`Send` async HTTP clients. The `reqwest` client returns a `Send`
future when using versions of `hyper` >= 0.14.14 (see
hyperium/hyper#2653), except in WASM builds,
for which the future is non-`Send`.

With this change, each `request_async()` method now returns
`impl Future`, which may or may not be `Send` depending on the inputs.
This provides the flexibility to support all the necessary HTTP
clients, while providing the convenience of `Send` futures for the
built-in `reqwest` client in non-WASM builds.

BREAKING CHANGES:
 - Removed `TokenRequestFuture` and `DeviceAuthorizationRequestFuture`
   type aliases.
 - Added `Future` associated type to `AsyncHttpClient` trait and changed
   `call()` method to return `Self::Future` (which may or may not be
   `Send`/`Sync`).
 - Changed each `request_async()` method to return `impl Future` instead
   of `Pin<Box<dyn Future>>`.

Fixes #255 and #260.
ramosbugs added a commit to ramosbugs/oauth2-rs that referenced this pull request Mar 10, 2024
@ramosbugs
Return impl Future instead of Pin<Box<dyn Future>>
6e583bd 
Returning a boxed trait object requires specifying whether the returned
future is `Send`/`Sync`, which means we can't support both `Send` and
non-`Send` async HTTP clients. The `reqwest` client returns a `Send`
future when using versions of `hyper` >= 0.14.14 (see
hyperium/hyper#2653), except in WASM builds,
for which the future is non-`Send`.

With this change, each `request_async()` method now returns
`impl Future`, which may or may not be `Send` depending on the inputs.
This provides the flexibility to support all the necessary HTTP
clients, while providing the convenience of `Send` futures for the
built-in `reqwest` client in non-WASM builds.

BREAKING CHANGES:
 - Removed `TokenRequestFuture` and `DeviceAuthorizationRequestFuture`
   type aliases.
 - Added `Future` associated type to `AsyncHttpClient` trait and changed
   `call()` method to return `Self::Future` (which may or may not be
   `Send`/`Sync`).
 - Changed each `request_async()` method to return `impl Future` instead
   of `Pin<Box<dyn Future>>`.

Fixes #255 and #260.
decahedron1 pushed a commit to vitri-ent/oauth2-rs that referenced this pull request Mar 10, 2024
@ramosbugs @decahedron1
Return impl Future instead of Pin<Box<dyn Future>>
ee36352 
Returning a boxed trait object requires specifying whether the returned
future is `Send`/`Sync`, which means we can't support both `Send` and
non-`Send` async HTTP clients. The `reqwest` client returns a `Send`
future when using versions of `hyper` >= 0.14.14 (see
hyperium/hyper#2653), except in WASM builds,
for which the future is non-`Send`.

With this change, each `request_async()` method now returns
`impl Future`, which may or may not be `Send` depending on the inputs.
This provides the flexibility to support all the necessary HTTP
clients, while providing the convenience of `Send` futures for the
built-in `reqwest` client in non-WASM builds.

BREAKING CHANGES:
 - Removed `TokenRequestFuture` and `DeviceAuthorizationRequestFuture`
   type aliases.
 - Added `Future` associated type to `AsyncHttpClient` trait and changed
   `call()` method to return `Self::Future` (which may or may not be
   `Send`/`Sync`).
 - Changed each `request_async()` method to return `impl Future` instead
   of `Pin<Box<dyn Future>>`.

Fixes ramosbugs#255 and ramosbugs#260.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Darksonn @chanks @seanmonstar