HTTPCLIENT-2333: update execution scope upon request redirect in order to avoid re-execution of the original request in case of an i/o error
This commit is contained in:
Oleg Kalnichevski
parent
df5ab533af
commit
a61f60b4bd
|
|
@ -29,10 +29,16 @@ package org.apache.hc.client5.testing.sync;
|
|||
import static org.hamcrest.MatcherAssert.assertThat;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.io.InterruptedIOException;
|
||||
import java.net.ConnectException;
|
||||
import java.net.NoRouteToHostException;
|
||||
import java.net.URI;
|
||||
import java.net.UnknownHostException;
|
||||
import java.util.Arrays;
|
||||
import java.util.Collections;
|
||||
import java.util.Queue;
|
||||
import java.util.concurrent.ConcurrentLinkedQueue;
|
||||
import java.util.concurrent.atomic.AtomicLong;
|
||||
|
||||
import org.apache.hc.client5.http.CircularRedirectException;
|
||||
import org.apache.hc.client5.http.ClientProtocolException;
|
||||
|
|
@ -42,6 +48,7 @@ import org.apache.hc.client5.http.classic.methods.HttpPost;
|
|||
import org.apache.hc.client5.http.config.RequestConfig;
|
||||
import org.apache.hc.client5.http.cookie.BasicCookieStore;
|
||||
import org.apache.hc.client5.http.cookie.CookieStore;
|
||||
import org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy;
|
||||
import org.apache.hc.client5.http.impl.cookie.BasicClientCookie;
|
||||
import org.apache.hc.client5.http.protocol.HttpClientContext;
|
||||
import org.apache.hc.client5.http.protocol.RedirectLocations;
|
||||
|
|
@ -54,6 +61,8 @@ import org.apache.hc.client5.testing.extension.sync.TestClient;
|
|||
import org.apache.hc.client5.testing.redirect.Redirect;
|
||||
import org.apache.hc.core5.function.Decorator;
|
||||
import org.apache.hc.core5.http.ClassicHttpRequest;
|
||||
import org.apache.hc.core5.http.ClassicHttpResponse;
|
||||
import org.apache.hc.core5.http.ConnectionClosedException;
|
||||
import org.apache.hc.core5.http.Header;
|
||||
import org.apache.hc.core5.http.HttpException;
|
||||
import org.apache.hc.core5.http.HttpHeaders;
|
||||
|
|
@ -62,12 +71,14 @@ import org.apache.hc.core5.http.HttpRequest;
|
|||
import org.apache.hc.core5.http.HttpStatus;
|
||||
import org.apache.hc.core5.http.ProtocolException;
|
||||
import org.apache.hc.core5.http.URIScheme;
|
||||
import org.apache.hc.core5.http.io.HttpRequestHandler;
|
||||
import org.apache.hc.core5.http.io.HttpServerRequestHandler;
|
||||
import org.apache.hc.core5.http.io.entity.EntityUtils;
|
||||
import org.apache.hc.core5.http.io.entity.StringEntity;
|
||||
import org.apache.hc.core5.http.message.BasicHeader;
|
||||
import org.apache.hc.core5.http.protocol.HttpContext;
|
||||
import org.apache.hc.core5.net.URIBuilder;
|
||||
import org.apache.hc.core5.util.TimeValue;
|
||||
import org.hamcrest.CoreMatchers;
|
||||
import org.junit.jupiter.api.Assertions;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
|
@ -642,4 +653,65 @@ abstract class TestRedirects extends AbstractIntegrationTestBase {
|
|||
assertThat(values.poll(), CoreMatchers.nullValue());
|
||||
}
|
||||
|
||||
@Test
|
||||
void testRetryUponRedirect() throws Exception {
|
||||
configureClient(builder -> builder
|
||||
.setRetryStrategy(new DefaultHttpRequestRetryStrategy(
|
||||
3,
|
||||
TimeValue.ofSeconds(1),
|
||||
Arrays.asList(
|
||||
InterruptedIOException.class,
|
||||
UnknownHostException.class,
|
||||
ConnectException.class,
|
||||
ConnectionClosedException.class,
|
||||
NoRouteToHostException.class),
|
||||
Arrays.asList(
|
||||
HttpStatus.SC_TOO_MANY_REQUESTS,
|
||||
HttpStatus.SC_SERVICE_UNAVAILABLE)) {
|
||||
})
|
||||
);
|
||||
|
||||
configureServer(bootstrap -> bootstrap
|
||||
.setExchangeHandlerDecorator(requestHandler -> new RedirectingDecorator(
|
||||
requestHandler,
|
||||
new OldPathRedirectResolver("/oldlocation", "/random", HttpStatus.SC_MOVED_TEMPORARILY)))
|
||||
.register("/random/*", new HttpRequestHandler() {
|
||||
|
||||
final AtomicLong count = new AtomicLong();
|
||||
|
||||
@Override
|
||||
public void handle(final ClassicHttpRequest request,
|
||||
final ClassicHttpResponse response,
|
||||
final HttpContext context) throws HttpException, IOException {
|
||||
if (count.incrementAndGet() == 1) {
|
||||
throw new IOException("Boom");
|
||||
} else {
|
||||
response.setCode(200);
|
||||
response.setEntity(new StringEntity("test"));
|
||||
}
|
||||
}
|
||||
|
||||
}));
|
||||
|
||||
final HttpHost target = startServer();
|
||||
|
||||
final TestClient client = client();
|
||||
final HttpClientContext context = HttpClientContext.create();
|
||||
|
||||
final HttpGet httpget = new HttpGet("/oldlocation/50");
|
||||
|
||||
client.execute(target, httpget, context, response -> {
|
||||
Assertions.assertEquals(HttpStatus.SC_OK, response.getCode());
|
||||
EntityUtils.consume(response.getEntity());
|
||||
return null;
|
||||
});
|
||||
final HttpRequest reqWrapper = context.getRequest();
|
||||
|
||||
Assertions.assertEquals(new URIBuilder()
|
||||
.setHttpHost(target)
|
||||
.setPath("/random/50")
|
||||
.build(),
|
||||
reqWrapper.getUri());
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -90,7 +90,6 @@ public final class AsyncRedirectExec implements AsyncExecChainHandler {
|
|||
volatile URI redirectURI;
|
||||
volatile int maxRedirects;
|
||||
volatile int redirectCount;
|
||||
volatile HttpRequest originalRequest;
|
||||
volatile HttpRequest currentRequest;
|
||||
volatile AsyncEntityProducer currentEntityProducer;
|
||||
volatile RedirectLocations redirectLocations;
|
||||
|
|
@ -109,7 +108,6 @@ public final class AsyncRedirectExec implements AsyncExecChainHandler {
|
|||
final AsyncExecChain.Scope scope = state.currentScope;
|
||||
final HttpClientContext clientContext = scope.clientContext;
|
||||
final String exchangeId = scope.exchangeId;
|
||||
final HttpRoute currentRoute = scope.route;
|
||||
chain.proceed(request, entityProducer, scope, new AsyncExecCallback() {
|
||||
|
||||
@Override
|
||||
|
|
@ -137,6 +135,7 @@ public final class AsyncRedirectExec implements AsyncExecChainHandler {
|
|||
}
|
||||
state.redirectLocations.add(redirectUri);
|
||||
|
||||
final AsyncExecChain.Scope currentScope = state.currentScope;
|
||||
final HttpHost newTarget = URIUtils.extractHost(redirectUri);
|
||||
if (newTarget == null) {
|
||||
throw new ProtocolException("Redirect URI does not specify a valid host name: " + redirectUri);
|
||||
|
|
@ -151,7 +150,7 @@ public final class AsyncRedirectExec implements AsyncExecChainHandler {
|
|||
redirectBuilder = BasicRequestBuilder.get();
|
||||
state.currentEntityProducer = null;
|
||||
} else {
|
||||
redirectBuilder = BasicRequestBuilder.copy(state.originalRequest);
|
||||
redirectBuilder = BasicRequestBuilder.copy(currentScope.originalRequest);
|
||||
}
|
||||
break;
|
||||
case HttpStatus.SC_SEE_OTHER:
|
||||
|
|
@ -159,18 +158,18 @@ public final class AsyncRedirectExec implements AsyncExecChainHandler {
|
|||
redirectBuilder = BasicRequestBuilder.get();
|
||||
state.currentEntityProducer = null;
|
||||
} else {
|
||||
redirectBuilder = BasicRequestBuilder.copy(state.originalRequest);
|
||||
redirectBuilder = BasicRequestBuilder.copy(currentScope.originalRequest);
|
||||
}
|
||||
break;
|
||||
default:
|
||||
redirectBuilder = BasicRequestBuilder.copy(state.originalRequest);
|
||||
redirectBuilder = BasicRequestBuilder.copy(currentScope.originalRequest);
|
||||
}
|
||||
redirectBuilder.setUri(redirectUri);
|
||||
state.reroute = false;
|
||||
state.redirectURI = redirectUri;
|
||||
state.originalRequest = redirectBuilder.build();
|
||||
state.currentRequest = redirectBuilder.build();
|
||||
|
||||
HttpRoute currentRoute = currentScope.route;
|
||||
if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) {
|
||||
final HttpRoute newRoute = routePlanner.determineRoute(newTarget, clientContext);
|
||||
if (!Objects.equals(currentRoute, newRoute)) {
|
||||
|
|
@ -189,21 +188,20 @@ public final class AsyncRedirectExec implements AsyncExecChainHandler {
|
|||
proxyAuthExchange.reset();
|
||||
}
|
||||
}
|
||||
state.currentScope = new AsyncExecChain.Scope(
|
||||
scope.exchangeId,
|
||||
newRoute,
|
||||
scope.originalRequest,
|
||||
scope.cancellableDependency,
|
||||
scope.clientContext,
|
||||
scope.execRuntime,
|
||||
scope.scheduler,
|
||||
scope.execCount);
|
||||
currentRoute = newRoute;
|
||||
}
|
||||
}
|
||||
}
|
||||
if (state.redirectURI != null) {
|
||||
state.currentScope = new AsyncExecChain.Scope(
|
||||
scope.exchangeId,
|
||||
currentRoute,
|
||||
redirectBuilder.build(),
|
||||
scope.cancellableDependency,
|
||||
scope.clientContext,
|
||||
scope.execRuntime,
|
||||
scope.scheduler,
|
||||
scope.execCount);
|
||||
if (LOG.isDebugEnabled()) {
|
||||
LOG.debug("{} redirecting to '{}' via {}", exchangeId, state.redirectURI, currentRoute);
|
||||
LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
|
@ -272,7 +270,6 @@ public final class AsyncRedirectExec implements AsyncExecChainHandler {
|
|||
final State state = new State();
|
||||
state.maxRedirects = config.getMaxRedirects() > 0 ? config.getMaxRedirects() : 50;
|
||||
state.redirectCount = 0;
|
||||
state.originalRequest = scope.originalRequest;
|
||||
state.currentRequest = request;
|
||||
state.currentEntityProducer = entityProducer;
|
||||
state.redirectLocations = redirectLocations;
|
||||
|
|
|
|||
|
|
@ -103,7 +103,6 @@ public final class RedirectExec implements ExecChainHandler {
|
|||
|
||||
final RequestConfig config = context.getRequestConfigOrDefault();
|
||||
final int maxRedirects = config.getMaxRedirects() > 0 ? config.getMaxRedirects() : 50;
|
||||
ClassicHttpRequest originalRequest = scope.originalRequest;
|
||||
ClassicHttpRequest currentRequest = request;
|
||||
ExecChain.Scope currentScope = scope;
|
||||
for (int redirectCount = 0;;) {
|
||||
|
|
@ -150,22 +149,22 @@ public final class RedirectExec implements ExecChainHandler {
|
|||
if (Method.POST.isSame(request.getMethod())) {
|
||||
redirectBuilder = ClassicRequestBuilder.get();
|
||||
} else {
|
||||
redirectBuilder = ClassicRequestBuilder.copy(originalRequest);
|
||||
redirectBuilder = ClassicRequestBuilder.copy(currentScope.originalRequest);
|
||||
}
|
||||
break;
|
||||
case HttpStatus.SC_SEE_OTHER:
|
||||
if (!Method.GET.isSame(request.getMethod()) && !Method.HEAD.isSame(request.getMethod())) {
|
||||
redirectBuilder = ClassicRequestBuilder.get();
|
||||
} else {
|
||||
redirectBuilder = ClassicRequestBuilder.copy(originalRequest);
|
||||
redirectBuilder = ClassicRequestBuilder.copy(currentScope.originalRequest);
|
||||
}
|
||||
break;
|
||||
default:
|
||||
redirectBuilder = ClassicRequestBuilder.copy(originalRequest);
|
||||
redirectBuilder = ClassicRequestBuilder.copy(currentScope.originalRequest);
|
||||
}
|
||||
redirectBuilder.setUri(redirectUri);
|
||||
|
||||
final HttpRoute currentRoute = currentScope.route;
|
||||
HttpRoute currentRoute = currentScope.route;
|
||||
if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) {
|
||||
final HttpRoute newRoute = this.routePlanner.determineRoute(newTarget, context);
|
||||
if (!Objects.equals(currentRoute, newRoute)) {
|
||||
|
|
@ -186,19 +185,19 @@ public final class RedirectExec implements ExecChainHandler {
|
|||
proxyAuthExchange.reset();
|
||||
}
|
||||
}
|
||||
currentScope = new ExecChain.Scope(
|
||||
currentScope.exchangeId,
|
||||
newRoute,
|
||||
currentScope.originalRequest,
|
||||
currentScope.execRuntime,
|
||||
currentScope.clientContext);
|
||||
currentRoute = newRoute;
|
||||
}
|
||||
}
|
||||
|
||||
if (LOG.isDebugEnabled()) {
|
||||
LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute);
|
||||
}
|
||||
originalRequest = redirectBuilder.build();
|
||||
currentScope = new ExecChain.Scope(
|
||||
scope.exchangeId,
|
||||
currentRoute,
|
||||
redirectBuilder.build(),
|
||||
scope.execRuntime,
|
||||
scope.clientContext);
|
||||
currentRequest = redirectBuilder.build();
|
||||
RequestEntityProxy.enhance(currentRequest);
|
||||
|
||||
|
|
|
|||
|
|
@ -122,7 +122,7 @@ class TestRedirectExec {
|
|||
redirectExec.execute(request, scope, chain);
|
||||
|
||||
final ArgumentCaptor<ClassicHttpRequest> reqCaptor = ArgumentCaptor.forClass(ClassicHttpRequest.class);
|
||||
Mockito.verify(chain, Mockito.times(2)).proceed(reqCaptor.capture(), ArgumentMatchers.same(scope));
|
||||
Mockito.verify(chain, Mockito.times(2)).proceed(reqCaptor.capture(), ArgumentMatchers.any());
|
||||
|
||||
final List<ClassicHttpRequest> allValues = reqCaptor.getAllValues();
|
||||
Assertions.assertNotNull(allValues);
|
||||
|
|
@ -395,7 +395,7 @@ class TestRedirectExec {
|
|||
Assertions.assertEquals(200, finalResponse.getCode());
|
||||
|
||||
final ArgumentCaptor<ClassicHttpRequest> reqCaptor = ArgumentCaptor.forClass(ClassicHttpRequest.class);
|
||||
Mockito.verify(chain, Mockito.times(3)).proceed(reqCaptor.capture(), ArgumentMatchers.same(scope));
|
||||
Mockito.verify(chain, Mockito.times(3)).proceed(reqCaptor.capture(), ArgumentMatchers.any());
|
||||
|
||||
final List<ClassicHttpRequest> allValues = reqCaptor.getAllValues();
|
||||
Assertions.assertNotNull(allValues);
|
||||
|
|
|
|||
Loading…
Reference in New Issue