-
Notifications
You must be signed in to change notification settings - Fork 1.6k
When Request Method is "HEAD" and setFollowRedirect is set to true. The method of the second redirect request is GET. #1728
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
Comments
@lakxtxue hi - I've applied to maintain this library, so I'm taking a look at open issues to see whether I can maybe help out. As a general note, a paste of the code tends to help in these issues - if not for better comprehension then at least to save a few lines of reproduction:) Just to make sure I follow: you've got a large resource located at Since the resource hiding behind What I assume you'd like to happen is for the same request type that was originally dispatched to be used in the followup requests as well. Is this a correct assessment? |
@Tomgs hi, yes, that's what I mean, because I use http to download large files, I need to get the size of the file first. When accessing this file resource, there will be a redirection to the real address. I use the HEAD request method to get the file size. AsyncHttpClient encounters a 301 When it was directed, it did not continue to access with a HEAD request but used a GET request to access the redirected address. Now I use HttpURLConnection instead, and I get the result I want correctly.
I used AsyncHttpClient to do the same thing. I also set the HEAD request method and configured setFollowRedirect to true. The result was a timeout. I found through packet capture that AsyncHttpClient used GET request when redirecting and started to download files. I hope it will behave like HttpURLConnection. I don’t know if the expression is clear or not, because my English is not very good. I have found a lot of http libraries. AsyncHttpClient is the only one that can provide me with the greatest flexibility. I really hope it will get better and better. |
@lakxtxue I see what you're saying, and it's perfectly clear. Your English is much better than many of the non-native English speakers I've met (I'm also not a native speaker, btw!). I actually wasn't sure what the HTTP protocol specification says about method handling in redirects. I didn't go to the spec itself, but MDN says that when a user-agent (like AsyncHttpClient) sends a request and encounters a 301, only I am still familiarising myself with the library, but an initial search seems to show that it's related to the underlying Netty provider this library is using. I'd say that it's reasonable for the method to persist between redirects, but would have to dig deeper later tonight to see how it can be actually implemented and open a PR. If I forget to follow up please ping me in a few days! |
Alright, so I think I found the culprit (not sure why it's not embedding the code lines properly): It basically switches to Will dig a little further and get to the root of this - @slandelle this was before your time, but if there's any chance you remember why there's a default to |
So I asked around and received other use cases where it makes sense to have a the next request in the cycle be a I'll write a PR |
Here’s what I do in Gatling: gatling/gatling@a87729b |
Ah, that makes sense, this way you allow it for |
Still need to open a PR, just not getting to it. Will do so sometime this week (need to re-write the tests so Maven won't be angry). |
Out of the two failing tests, one of them - That leaves us with one problematic test,
Looking now. Full test dump shows that fixing the interceptor with @slandelle's fix works fine - each following request is a HEAD:
|
One of the really fun things about long-standing repos is that each piece of code has a rich history behind it, with various people facing the same(-ish) problem at different points in time, adding fixes and thought-nuggets into the code over the years. This seems to be exactly one of those cases - looking at https://github.com/TomGranot/async-http-client/commits/master/client/src/test/java/org/asynchttpclient/Head302Test.java reveals that this test actually has been fixed in the past. Specifically in our case, it seems that TomGranot@3c25a42#diff-64fc1f6f6a5d572c250342b4cdfda122 made this very test added the behaviour that causes this endless loop we're looking at right now - the In plain terms, this means that the test expects a And.... PR done. See below. |
Originally, when setFollowRedirect is set to true (like in Head302Test), the expected flow is HEAD --> GET (based on the behaviour of Redirect30xInterceptor - see AsyncHttpClient@3c25a42 for the reasoning). Following a change to the interceptor (to fix AsyncHttpClient#1728), Head302Test started going on an infinite loop caused by the way the handler in the test was set up (to account for the original HEAD --> GET flow and not the new HEAD --> HEAD --> HEAD.... flow). This PR does 3 things: * Changes Redirect30xInterceptor to set all redirects of a HEAD request to be HEADs as well * Change Head302Test to account for the new flow * Notes a flaky test in AsyncStreamHandlerTest that was found during work on the PR
Originally, when setFollowRedirect is set to true (like in Head302Test), the expected flow is HEAD --> GET (based on the behavior of Redirect30xInterceptor - see 3c25a42 for the reasoning). Following a change to the interceptor (to fix #1728), Head302Test started going on an infinite loop caused by the way the handler in the tent was set up (to account for the original HEAD --> GET flow and not the new HEAD --> HEAD --> HEAD.... flow). This commit does 3 things: * Changes Redirect30xInterceptor to set all redirects of a HEAD request to be HEADs as well * Change Head302Test to account for the new flow * Notes a flaky test in AsyncStreamHandlerTest that was found during work on the PR
@lakxtxue Merged to master, should be deployed in 2.12.3. |
When Request Method is "HEAD" and setFollowRedirect is set to true. The method of the second redirect request is GET.
When I download a large file, it will time out here, in fact it has been downloading
Caused by: java.util.concurrent.TimeoutException: Request timeout to 127.0.0.1/127.0.0.1:8888 after 60000 ms at org.asynchttpclient.netty.timeout.TimeoutTimerTask.expire(TimeoutTimerTask.java:43) at org.asynchttpclient.netty.timeout.RequestTimeoutTimerTask.run(RequestTimeoutTimerTask.java:50) at io.netty.util.HashedWheelTimer$HashedWheelTimeout.expire(HashedWheelTimer.java:672) at io.netty.util.HashedWheelTimer$HashedWheelBucket.expireTimeouts(HashedWheelTimer.java:747) at io.netty.util.HashedWheelTimer$Worker.run(HashedWheelTimer.java:472) at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) at java.base/java.lang.Thread.run(Thread.java:834)
The text was updated successfully, but these errors were encountered: