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.

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

Use original method in redirects for HEAD / OPTIONS requests #1736

Conversation

TomGranot
Copy link
Contributor

Originally, when setFollowRedirect is set to true (like in Head302Test), the expected flow is HEAD --> GET (based on the behaviour 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 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 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
@TomGranot
Copy link
Contributor Author

TomGranot commented Sep 28, 2020

Failing tests:

Tests run: 885, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 196.279 sec <<< FAILURE! - in TestSuite
testMaxTotalConnections(org.asynchttpclient.channel.MaxTotalConnectionTest)  Time elapsed: 0.103 sec  <<< FAILURE!
java.lang.AssertionError: expected [null] but found [java.net.UnknownHostException: DNS name not found [response code 3]]
	at org.asynchttpclient.channel.MaxTotalConnectionTest.testMaxTotalConnections(MaxTotalConnectionTest.java:108)
asyncOptionsTest(org.asynchttpclient.AsyncStreamHandlerTest)  Time elapsed: 0.303 sec  <<< FAILURE!
java.lang.AssertionError: expected [4] but found [5]
	at org.asynchttpclient.AsyncStreamHandlerTest.lambda$null$17(AsyncStreamHandlerTest.java:461)
	at org.asynchttpclient.AsyncStreamHandlerTest.lambda$asyncOptionsTest$18(AsyncStreamHandlerTest.java:437)
	at org.asynchttpclient.AsyncStreamHandlerTest.asyncOptionsTest(AsyncStreamHandlerTest.java:436)

The first one does not fail locally - looks like something on the build server itself. @slandelle if you can re-run it I'd appreciate it.

The second one is flaky (#1735).

@TomGranot
Copy link
Contributor Author

@slandelle If you get a minute to re-jig travis-ci that would be great - see my comments above re the tests.

@TomGranot TomGranot merged commit bd7b5bd into AsyncHttpClient:master Jan 5, 2021
@TomGranot TomGranot deleted the bugfix/setfollowredirect-follow-the-method branch January 5, 2021 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant