feat(redirect): Using `FollowRedirect` from `tower-http` to handle the `redirect` loop by linyihai · Pull Request #2617 · seanmonstar/reqwest

linyihai · 2025-03-28T06:58:01Z

What does this want to do

Working on #2576

This PR mainly split the redirect logic in the loop into three place

Let FollowRedirect<S, P> to decide whether a redirection is needed
Let the HyperService and H3Client to handle the cookies, H3Client had implemented the Service trait as well.
Let the TowerRedirectPolicy to handle the other logics, like check the URI, remove_sensitive_headers, insertreferer header

There is still exists the retry logic in the loop, I don't really want to copy them into HyperService and H3Client.

How to test

The first commit add the Limit and Custom policy test. The second commit make the implementation and updated the test.

Others

Closes #2576

Xuanwo

Hi, thank you @linyihai for this change.

I understand the motivation that @seanmonstar described in #2576, but the current changes don't seem quite right to me. The implementation feels rather ad-hoc and makes future extensions more difficult.

We're introducing a new dependency for reqwest and a new Mutex for the internal service. Additionally, we'll need to use tower_http::follow_redirect::ResponseFuture throughout the codebase.

The API design of tower_http doesn't align with the core concept of the reqwest client, where Client only takes &self for building and sending requests. This is also why we have to add a Mutex.

What if we keep the redirect policy as it is and encourage users to use tower_http::follow_redirect outside of reqwest::Client, since we've already implemented the tower layer? We can build a new API, such as client.layer(tower_http::follow_redirect), and gradually deprecate our existing redirect policy.

I'm not sure what @seanmonstar's long-term plan is for reqwest's architecture. If the goal is to transform the reqwest client into a purely tower_layer-based framework, and the current implementation is just a transitional stage, I'm fine with that.

seanmonstar · 2025-05-07T22:03:55Z

                    .expect("valid request parts");
                *req.headers_mut() = self.headers.clone();
-                ResponseFuture::Default(self.client.hyper.request(req))
+                let mut hyper = self.client.hyper.lock().unwrap();


An alternative to using a Mutex is if the client can be cheaply and safely cloned, then the clone can be mutable.

linyihai · 2025-05-08T13:50:29Z

warning: unused manifest key: lints
Updating crates.io index
Downloading crates ...
Downloaded iri-string v0.7.8
Downloaded tower-http v0.6.3
error: package tower-http v0.6.3 cannot be built because it requires rustc 1.66 or newer, while the currently active rustc version is 1.64.0
Error: Process completed with exit code 101.

Hmm, the biggest compatible rustc version of tower-http is 0.4.4, but the 0.4.4 is so different from "0.6.x", becasuse '0.4.4' are too old that many trait implementation so different.

seanmonstar · 2025-05-09T20:25:08Z

Yea, don't try to use an older version. I suspect I might be able to get tower-http to drop its MSRV back down some, it seems it was bumped just for a dependency.

If not, then we'll have to increase it in reqwest, but I'm hopeful.

seanmonstar · 2025-05-12T15:28:24Z

tower-http 0.6.4 was just published with a lower MSRV, so I'm restarting that CI job.

linyihai · 2025-05-13T14:40:55Z

I'm content with the TowerRedirectPolicy, but Policy can't touch the Response, so it has no way to handle the cookies in redirections. HyperService and H3Client have to handle the cookies.

Ok, I think this PR is ready for review.

seanmonstar

This looks really good! Thanks so much!

I think this does everything, right? Is there anything we missed, that you can think of?

seanmonstar · 2025-05-20T18:44:48Z

                        #[cfg(feature = "http2")]
-                        if self.as_mut().retry_error(&e) {
-                            continue;
+                        if e.is_request() {


What's the change here for? Is it because the error was pushing into a reqwest::Error type instead of the underlying hyper error? That sounds right, now that I type is out...

Yes, in the HyperService::call where had wrapped the crate::error::request error outside already.

Box::pin(async move { inner.call(req).await.map_err(crate::error::request) })

linyihai · 2025-05-22T11:58:19Z

I think this does everything, right? Is there anything we missed, that you can think of?

I checked the loop loginc again, can't find anything needs to do.

BTW, there is something could be improved in feature:

make the cookies handle logic as a standalone middleware (I don't know how difficult it is)
make the retry handle logic as a standalone middleware also, so that the loop can be totally removed in the PendingReqest::poll
migrate the checking HTTP schema logic into tower-http if necessary.

seanmonstar · 2025-05-26T20:21:27Z

Sounds good! Sorry, it looks merging another PR caused conflicts. Could you take a look at those? I'll merge quickly after so we don't have that again :)

… over the redirect loop

linyihai · 2025-05-27T08:09:50Z

it looks merging another PR caused conflicts

After merging that PR，I added the Sync trait to the Future, thanks for it.

@seanmonstar Could you merge this PR if there is nothing left?

seanmonstar

Thanks so much! This is stellar, I appreciate you sticking with it.

Bumps reqwest from 0.12.15 to 0.12.16. Release notes Sourced from reqwest's releases. v0.12.16 Highlights Add ClientBuilder::http3_congestion_bbr() to enable BBR congestion control. Add ClientBuilder::http3_send_grease() to configure whether to send use QUIC grease. Add ClientBuilder::http3_max_field_section_size() to configure the maximum response headers. Add ClientBuilder::tcp_keepalive_interval() to configure TCP probe interval. Add ClientBuilder::tcp_keepalive_retries() to configure TCP probe count. Add Proxy::headers() to add extra headers that should be sent to a proxy. Fix redirect::Policy::limit() which had an off-by-1 error, allowing 1 more redirect than specified. Fix HTTP/3 to support streaming request bodies. (wasm) Fix null bodies when calling Response::bytes_stream(). What's Changed Clarify that Response::content_length() is not derived from a Content-Length header in docs by @babolivier in seanmonstar/reqwest#2588 docs: link to char::REPLACEMENT_CHARACTER by @marcospb19 in seanmonstar/reqwest#1880 feat: add H3 client config support by @smalls0098 in seanmonstar/reqwest#2609 chore: update brotli to v7 by @nyurik in seanmonstar/reqwest#2620 Do not pull in an entirely different DEFLATE implementation just for tests by @Shnatsel in seanmonstar/reqwest#2625 chore: fix some typos in comment by @xixishidibei in seanmonstar/reqwest#2628 fix(wasm): handle null body in bytes_stream by @alongubkin in seanmonstar/reqwest#2632 ClientBuilder::interface on macOS/Solarish OSes by @hawkw in seanmonstar/reqwest#2623 ci: use ubuntu-latest in nightly job by @seanmonstar in seanmonstar/reqwest#2646 feat: BBR congestion control for http3 by @threeninesixseven in seanmonstar/reqwest#2642 feat: Add extentions for Request by @Xuanwo in seanmonstar/reqwest#2647 refactor: Store request timeout in request extensions instead by @Xuanwo in seanmonstar/reqwest#2650 chore: make ci pass by @linyihai in seanmonstar/reqwest#2666 update h3 dependencys by @Ruben2424 in seanmonstar/reqwest#2670 Document reqwest can make TLS and cookie requests with Wasm by @nickbabcock in seanmonstar/reqwest#2661 fix(redirect): make the number of redirects of policy matches its maximum limit. by @linyihai in seanmonstar/reqwest#2664 Exposed hyper tcp keepalive interval and retries parameters by @mackliet in seanmonstar/reqwest#2675 refactor: use hyper-util's proxy::Matcher by @seanmonstar in seanmonstar/reqwest#2681 Support streaming request body in HTTP/3 by @ducaale in seanmonstar/reqwest#2673 refactor: use hyper-util Tunnel by @seanmonstar in seanmonstar/reqwest#2684 Upgrade webpki-roots to 1 by @djc in seanmonstar/reqwest#2688 refactor: remove futures-util unless using stream/multipart/compression/blocking by @paolobarbolini in seanmonstar/reqwest#2692 chore: replace rustls-pemfile with rustls-pki-types by @tottoto in seanmonstar/reqwest#2541 Ensure H3ResponseFuture Implements Sync by @ducaale in seanmonstar/reqwest#2685 feat(redirect): Using FollowRedirect from tower-http to handle the redirect loop by @linyihai in seanmonstar/reqwest#2617 feat: add customizable headers in proxy mode by @chanbengz in seanmonstar/reqwest#2600 Prepare v0.12.16 by @seanmonstar in seanmonstar/reqwest#2694 New Contributors @babolivier made their first contribution in seanmonstar/reqwest#2588 @marcospb19 made their first contribution in seanmonstar/reqwest#1880 @smalls0098 made their first contribution in seanmonstar/reqwest#2609 @Shnatsel made their first contribution in seanmonstar/reqwest#2625 @xixishidibei made their first contribution in seanmonstar/reqwest#2628 @alongubkin made their first contribution in seanmonstar/reqwest#2632 ... (truncated) Changelog Sourced from reqwest's changelog. v0.12.16 Add ClientBuilder::http3_congestion_bbr() to enable BBR congestion control. Add ClientBuilder::http3_send_grease() to configure whether to send use QUIC grease. Add ClientBuilder::http3_max_field_section_size() to configure the maximum response headers. Add ClientBuilder::tcp_keepalive_interval() to configure TCP probe interval. Add ClientBuilder::tcp_keepalive_retries() to configure TCP probe count. Add Proxy::headers() to add extra headers that should be sent to a proxy. Fix redirect::Policy::limit() which had an off-by-1 error, allowing 1 more redirect than specified. Fix HTTP/3 to support streaming request bodies. (wasm) Fix null bodies when calling Response::bytes_stream(). Commits 99259cb v0.12.16 57670ac feat: add customizable headers for reqwest::Proxy (#2600) d9cf60e refactor: Using FollowRedirect from tower-http to handle the redirect l... 75f62f2 fix: ensure H3ResponseFuture is sync (#2685) 0e1d188 chore: replace rustls-pemfile with rustls-pki-types (#2541) 705b613 refactor: remove futures-util unless using stream/multipart/compression... 7b80718 Upgrade webpki-roots to 1 (#2688) 152a560 refactor: use hyper-util Tunnel (#2684) df09c9e feat: support streaming request body in HTTP/3 (#2673) 4ec1fe5 refactor: use hyper-util's proxy::Matcher (#2681) 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)

cschramm · 2025-06-01T09:39:08Z

+        if next_url.scheme() != "http" && next_url.scheme() != "https" {
+            return Err(crate::error::url_bad_scheme(next_url));


I might be missing something, but isn't this a pretty bad breaking change?

Given a request that returns a redirect to something else, say myscheme://, I have previously been able to get that response with a client with Policy::none(). Now I am getting this error instead, as the next URL is expected to be executable without checking the policy first.

I guess checks on next_url should only be done if the policy returned Follow.

I guess checks on next_url should only be done if the policy returned Follow.

Thanks for point this out. I would make a patch for it.

linyihai commented Mar 28, 2025

View reviewed changes

Comment thread tests/redirect.rs Outdated

linyihai changed the title ~~feat(redirect): Using tower-http policy instead.~~ feat(redirect): Coverting policy into tower-http policy. Mar 28, 2025

linyihai mentioned this pull request Apr 2, 2025

feat(redirect): add new method for Attempt tower-rs/tower-http#555

Closed

linyihai force-pushed the redirect-refactor branch from b400be6 to 4cf8a0f Compare April 2, 2025 11:14

linyihai force-pushed the redirect-refactor branch from 4cf8a0f to 88c542e Compare April 30, 2025 03:21

Xuanwo mentioned this pull request Apr 30, 2025

feat: Add request level redirect policy config #2658

Open

linyihai force-pushed the redirect-refactor branch from 88c542e to 3a1064d Compare April 30, 2025 03:40

linyihai changed the title ~~feat(redirect): Coverting policy into tower-http policy.~~ feat(redirect): Using FollowRedirect from tower-http to handle the redirect loop Apr 30, 2025

linyihai commented Apr 30, 2025

View reviewed changes

Comment thread tests/redirect.rs Outdated

This was referenced Apr 30, 2025

Refactor redirects to use tower-http #2576

Closed

reqwest::redirect::Policy::limited redirect less than the exact count #2659

Closed

linyihai force-pushed the redirect-refactor branch from 3a1064d to 02927c4 Compare April 30, 2025 09:11

linyihai marked this pull request as ready for review May 6, 2025 03:37

linyihai force-pushed the redirect-refactor branch from 738184e to f7f14fa Compare May 6, 2025 03:39

linyihai marked this pull request as draft May 6, 2025 11:57

linyihai force-pushed the redirect-refactor branch 3 times, most recently from 23e4f11 to 234de4f Compare May 7, 2025 02:59

Xuanwo reviewed May 7, 2025

View reviewed changes

seanmonstar reviewed May 7, 2025

View reviewed changes

linyihai force-pushed the redirect-refactor branch 3 times, most recently from faf04e5 to db0be61 Compare May 8, 2025 12:25

linyihai force-pushed the redirect-refactor branch 4 times, most recently from f895f06 to 5b9addf Compare May 13, 2025 09:18

linyihai force-pushed the redirect-refactor branch from 5b9addf to cde40f4 Compare May 13, 2025 13:36

linyihai marked this pull request as ready for review May 13, 2025 14:41

seanmonstar reviewed May 20, 2025

View reviewed changes

linyihai force-pushed the redirect-refactor branch from cde40f4 to 29a0c8f Compare May 22, 2025 03:29

linyihai commented May 22, 2025

View reviewed changes

Comment thread src/redirect.rs

test(redirect): add redirect policy test.

52df5ed

linyihai force-pushed the redirect-refactor branch from 29a0c8f to d4a1e38 Compare May 27, 2025 07:12

feat(redirect): use tower_http::follow_redirect::FollowRedirect takes…

bb43a97

… over the redirect loop

linyihai force-pushed the redirect-refactor branch from d4a1e38 to bb43a97 Compare May 27, 2025 07:40

seanmonstar approved these changes May 27, 2025

View reviewed changes

seanmonstar merged commit d9cf60e into seanmonstar:master May 27, 2025
36 checks passed

linyihai deleted the redirect-refactor branch May 28, 2025 00:59

cschramm reviewed Jun 1, 2025

View reviewed changes

linyihai mentioned this pull request Jun 1, 2025

fix: only check scheme after Policy return follow #2710

Merged

konstin mentioned this pull request Jun 24, 2025

Update to reqwest 0.12.20 astral-sh/uv#14243

Closed

		if next_url.scheme() != "http" && next_url.scheme() != "https" {
		return Err(crate::error::url_bad_scheme(next_url));

Uh oh!

Conversation

linyihai commented Mar 28, 2025 • edited by seanmonstar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this want to do

How to test

Others

Uh oh!

Uh oh!

Uh oh!

Xuanwo left a comment

Choose a reason for hiding this comment

Uh oh!

seanmonstar May 7, 2025

Choose a reason for hiding this comment

Uh oh!

linyihai May 8, 2025

Choose a reason for hiding this comment

Uh oh!

linyihai commented May 8, 2025

Uh oh!

seanmonstar commented May 9, 2025

Uh oh!

seanmonstar commented May 12, 2025

Uh oh!

linyihai commented May 13, 2025

Uh oh!

seanmonstar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seanmonstar May 20, 2025

Choose a reason for hiding this comment

Uh oh!

linyihai May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

linyihai commented May 22, 2025

Uh oh!

seanmonstar commented May 26, 2025

Uh oh!

linyihai commented May 27, 2025

Uh oh!

seanmonstar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cschramm Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linyihai Jun 1, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linyihai commented Mar 28, 2025 •

edited by seanmonstar

Loading

cschramm Jun 1, 2025 •

edited

Loading