Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Remove Sync requirement from reqwest::Body::wrap_stream() #2361

Merged
merged 4 commits into from
Jul 19, 2024
Merged

Remove Sync requirement from reqwest::Body::wrap_stream() #2361

merged 4 commits into from
Jul 19, 2024

Conversation

nipunn1313
Copy link
Contributor

The Sync requirement is unnecessary as far as I can tell.

Notably it makes it harder to work with the UnsyncBoxBody stream that the latest axum has moved to using

https://docs.rs/axum/latest/axum/body/struct.Body.html#method.into_data_stream

@nipunn1313
Copy link
Contributor Author

ok this is clearly not as simple as I thought. It would still be nice, but may require a bit of fiddling.

@nipunn1313 nipunn1313 closed this Jul 17, 2024
@nipunn1313 nipunn1313 reopened this Jul 17, 2024
@nipunn1313
Copy link
Contributor Author

ok - I believe I got it working here. I switched ResponseBody and Inner::Streaming to use UnsyncBoxBody which simply has less requirements. If I am understanding the change correctly, these are internal functionalities so the only external facing impacts are

  • wrap_stream has less requirements on the input (Sync not required)
  • reqwest::Body is no longer Sync

Everything else appears to affect only internal methods

The impact to wrap_stream seems largely good, but the impact to reqwest::Body does seem backward incompatible and not necessarily desirable.

I could imagine mirroring and having reqwest::Body and reqwest::BodyUnsync - or having a wrap_stream_unsync that does a dance with a Mutex to make it work.

@seanmonstar
Copy link
Owner

I think it can be done by internally using sync_wrapper in this specific case.

@nipunn1313
Copy link
Contributor Author

this worked great!

I changed both the pub wrap_stream and pub(crate) stream methods to remove the Sync requirement. I had the pub(crate) stream method use sync_wrapper internally.

It seems like a nice zero-cost abstraction! Thanks for making me aware.

seanmonstar
seanmonstar approved these changes Jul 19, 2024
View reviewed changes
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Nice! I thought we could probably do that, but I hadn't found the time to dig. Thank you!

@seanmonstar seanmonstar merged commit b9d62a0 into seanmonstar:master Jul 19, 2024
36 checks passed
@abdolence abdolence mentioned this pull request Aug 2, 2024
Closed
kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Aug 20, 2024
@dependabot
chore: Bump reqwest from 0.12.5 to 0.12.7 (#325)
0440dcf 
Bumps reqwest from 0.12.5 to 0.12.7.

Release notes
Sourced from reqwest's releases.

v0.12.7
What's Changed

Revert adding impl Service<http::Request<_>> for Client.

Full Changelog: seanmonstar/reqwest@v0.12.6...v0.12.7
v0.12.6
What's Changed

Add support for danger_accept_invalid_hostnames for rustls.
Add impl Service<http::Request<Body>> for Client and &'_ Client.
Add support for !Sync bodies in Body::wrap_stream().
Enable happy eyeballs when hickory-dns is used.
Fix Proxy so that HTTP(S)_PROXY values take precendence over ALL_PROXY.
Fix blocking::RequestBuilder::header() from unsetting sensitive on passed header values.

New Contributors

@​schopin-pro made their first contribution in seanmonstar/reqwest#2341
@​Ten0 made their first contribution in seanmonstar/reqwest#2353
@​thalesfragoso made their first contribution in seanmonstar/reqwest#2249
@​nipunn1313 made their first contribution in seanmonstar/reqwest#2361
@​Threated made their first contribution in seanmonstar/reqwest#2370
@​FlowerCode made their first contribution in seanmonstar/reqwest#2380
@​zeling made their first contribution in seanmonstar/reqwest#2378
@​murongshaozong made their first contribution in seanmonstar/reqwest#2385
@​camio made their first contribution in seanmonstar/reqwest#2388
@​alekseysidorov made their first contribution in seanmonstar/reqwest#2356

Thanks again

@​seanmonstar
@​nyurik

Full Changelog: seanmonstar/reqwest@v0.12.5...v0.12.6



Changelog
Sourced from reqwest's changelog.

v0.12.7

Revert adding impl Service<http::Request<_>> for Client.

v0.12.6

Add support for danger_accept_invalid_hostnames for rustls.
Add impl Service<http::Request<Body>> for Client and &'_ Client.
Add support for !Sync bodies in Body::wrap_stream().
Enable happy eyeballs when hickory-dns is used.
Fix Proxy so that HTTP(S)_PROXY values take precendence over ALL_PROXY.
Fix blocking::RequestBuilder::header() from unsetting sensitive on passed header values.




Commits

88bd9be v0.12.7
68127f0 Revert "feat: Add impl Service\<http::Request<Body>> for Client and `&'_ C...
b2a28f5 v0.12.6
522216e feat: Add impl Service\<http::Request<Body>> for Client and &'_ Client (...
646b1f8 chore: update macOS system-configuration dep (#2368)
85dd6da dns: improve error message for hickory-dns and warn in docs (#2389)
bfd31be docs: Improve RequestBuilder::multipart's documentation (#2388)
8c7f338 chore: bump dev-dependency libflate (#2382)
dddf877 chore: bump h3 dependency
a53c944 chore: fix some comments
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Nutomic pushed a commit to Nutomic/reqwest that referenced this pull request Nov 7, 2024
@nipunn1313 @Nutomic
Remove Sync requirement from reqwest::Body::wrap_stream() (seanmonsta…
29bd44b 
…r#2361)

Closes seanmonstar#2273
# for free to join this conversation on GitHub. Already have an account? # 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.
2 participants
@nipunn1313 @seanmonstar