Skip to content

Commit 51da4d5

Browse files
committed
Account for recurring HEADs in Head302Test
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
1 parent 12f4b2a commit 51da4d5

File tree

3 files changed

+16
-5
lines changed

3 files changed

+16
-5
lines changed

Diff for: client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@
3838

3939
import static io.netty.handler.codec.http.HttpHeaderNames.*;
4040
import static org.asynchttpclient.util.HttpConstants.Methods.GET;
41+
import static org.asynchttpclient.util.HttpConstants.Methods.HEAD;
42+
import static org.asynchttpclient.util.HttpConstants.Methods.OPTIONS;
4143
import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.*;
4244
import static org.asynchttpclient.util.HttpUtils.followRedirect;
4345
import static org.asynchttpclient.util.MiscUtils.isNonEmpty;
@@ -87,7 +89,7 @@ public boolean exitAfterHandlingRedirect(Channel channel,
8789

8890
String originalMethod = request.getMethod();
8991
boolean switchToGet = !originalMethod.equals(GET)
90-
&& (statusCode == MOVED_PERMANENTLY_301 || statusCode == SEE_OTHER_303 || (statusCode == FOUND_302 && !config.isStrict302Handling()));
92+
&& !originalMethod.equals(OPTIONS) && !originalMethod.equals(HEAD) && (statusCode == MOVED_PERMANENTLY_301 || statusCode == SEE_OTHER_303 || (statusCode == FOUND_302 && !config.isStrict302Handling()));
9193
boolean keepBody = statusCode == TEMPORARY_REDIRECT_307 || statusCode == PERMANENT_REDIRECT_308 || (statusCode == FOUND_302 && config.isStrict302Handling());
9294

9395
final RequestBuilder requestBuilder = new RequestBuilder(switchToGet ? GET : originalMethod)
@@ -126,7 +128,6 @@ else if (isNonEmpty(request.getBodyParts())) {
126128
HttpHeaders responseHeaders = response.headers();
127129
String location = responseHeaders.get(LOCATION);
128130
Uri newUri = Uri.create(future.getUri(), location);
129-
130131
LOGGER.debug("Redirecting to {}", newUri);
131132

132133
CookieStore cookieStore = config.getCookieStore();

Diff for: client/src/test/java/org/asynchttpclient/AsyncStreamHandlerTest.java

+2
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,8 @@ public Integer onCompleted() {
428428
}));
429429
}
430430

431+
// This test is flaky - see https://github.com/AsyncHttpClient/async-http-client/issues/1728#issuecomment-699962325
432+
// For now, just run again if fails
431433
@Test(groups = "online")
432434
public void asyncOptionsTest() throws Throwable {
433435

Diff for: client/src/test/java/org/asynchttpclient/Head302Test.java

+11-3
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,17 @@ public Response onCompleted(Response response) throws Exception {
7070
private static class Head302handler extends AbstractHandler {
7171
public void handle(String s, org.eclipse.jetty.server.Request r, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
7272
if ("HEAD".equalsIgnoreCase(request.getMethod())) {
73-
response.setStatus(HttpServletResponse.SC_FOUND); // 302
74-
response.setHeader("Location", request.getPathInfo() + "_moved");
75-
} else if ("GET".equalsIgnoreCase(request.getMethod())) {
73+
// See https://github.com/AsyncHttpClient/async-http-client/issues/1728#issuecomment-700007980
74+
// When setFollowRedirect == TRUE, a follow-up request to a HEAD request will also be a HEAD.
75+
// This will cause an infinite loop, which will error out once the maximum amount of redirects is hit (default 5).
76+
// Instead, we (arbitrarily) choose to allow for 3 redirects and then return a 200.
77+
if(request.getRequestURI().endsWith("_moved_moved_moved")) {
78+
response.setStatus(HttpServletResponse.SC_OK);
79+
} else {
80+
response.setStatus(HttpServletResponse.SC_FOUND); // 302
81+
response.setHeader("Location", request.getPathInfo() + "_moved");
82+
}
83+
} else if ("GET".equalsIgnoreCase(request.getMethod()) ) {
7684
response.setStatus(HttpServletResponse.SC_OK);
7785
} else {
7886
response.setStatus(HttpServletResponse.SC_FORBIDDEN);

0 commit comments

Comments
 (0)