From a61f60b4bdb3891a848246cf541a3e4ff47fd231 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sat, 27 Jul 2024 12:30:53 +0200 Subject: [PATCH] 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 --- .../client5/testing/sync/TestRedirects.java | 72 +++++++++++++++++++ .../http/impl/async/AsyncRedirectExec.java | 35 +++++---- .../http/impl/classic/RedirectExec.java | 23 +++--- .../http/impl/classic/TestRedirectExec.java | 4 +- 4 files changed, 101 insertions(+), 33 deletions(-) diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java index 3f1b30a76..fa8da0c2f 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java @@ -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()); + } + } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java index b213abe51..5d44d68cf 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java @@ -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; diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java index 47715621a..3c21d28e5 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java @@ -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); diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java index 2bbedebe6..45889442e 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java @@ -122,7 +122,7 @@ class TestRedirectExec { redirectExec.execute(request, scope, chain); final ArgumentCaptor 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 allValues = reqCaptor.getAllValues(); Assertions.assertNotNull(allValues); @@ -395,7 +395,7 @@ class TestRedirectExec { Assertions.assertEquals(200, finalResponse.getCode()); final ArgumentCaptor 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 allValues = reqCaptor.getAllValues(); Assertions.assertNotNull(allValues);