Fixes #3411 - HttpClient does not timeout during multiple redirection.

Removed timeout after copying the request in case of redirects
(and authentications), to avoid that the timeout listener is
notified of intermediate exchanges and resets the timeout.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java

@@ -23,6 +23,7 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.TimeUnit;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -217,6 +218,8 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler
                     path = request.getPath();
                 }
                 Request newRequest = client.copyRequest(request, requestURI);
+                // Disable the timeout so that only the one from the initial request applies.
+                newRequest.timeout(0, TimeUnit.MILLISECONDS);
                 if (path != null)
                     newRequest.path(path);
 

jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java

@@ -928,7 +928,7 @@ public class HttpClient extends ContainerLifeCycle
     }
 
     /**
-     * @return the max number of HTTP redirects that are followed
+     * @return the max number of HTTP redirects that are followed in a conversation
      * @see #setMaxRedirects(int)
      */
     public int getMaxRedirects()
@@ -937,7 +937,7 @@ public class HttpClient extends ContainerLifeCycle
     }
 
     /**
-     * @param maxRedirects the max number of HTTP redirects that are followed
+     * @param maxRedirects the max number of HTTP redirects that are followed in a conversation, or -1 for unlimited redirects
      * @see #setFollowRedirects(boolean)
      */
     public void setMaxRedirects(int maxRedirects)

jetty-client/src/main/java/org/eclipse/jetty/client/HttpConversation.java

@@ -25,9 +25,13 @@ import java.util.concurrent.ConcurrentLinkedDeque;
 
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.util.AttributesMap;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
 
 public class HttpConversation extends AttributesMap
 {
+    private static final Logger LOG = Log.getLogger(HttpConversation.class);
+
     private final Deque<HttpExchange> exchanges = new ConcurrentLinkedDeque<>();
     private volatile List<Response.ResponseListener> listeners;
 
@@ -118,6 +122,7 @@ public class HttpConversation extends AttributesMap
         HttpExchange lastExchange = exchanges.peekLast();
         if (firstExchange == lastExchange)
         {
+            // We don't have a conversation, just a single request.
             if (overrideListener != null)
                 listeners.add(overrideListener);
             else
@@ -125,13 +130,16 @@ public class HttpConversation extends AttributesMap
         }
         else
         {
-            // Order is important, we want to notify the last exchange first
+            // We have a conversation (e.g. redirect, authentication).
+            // Order is important, we want to notify the last exchange first.
             listeners.addAll(lastExchange.getResponseListeners());
             if (overrideListener != null)
                 listeners.add(overrideListener);
             else
                 listeners.addAll(firstExchange.getResponseListeners());
         }
+        if (LOG.isDebugEnabled())
+            LOG.debug("Exchanges in conversation {}, override={}, listeners={}", exchanges.size(), overrideListener, listeners);
         this.listeners = listeners;
     }
 

jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java

@@ -490,8 +490,12 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest
                 exchanges.size(),
                 connectionPool);
     }
-    
+
-    // The TimeoutTask that expires when the next check of expiry is needed
+    /**
+     * This class enforces the total timeout for exchanges that are still in the queue.
+     * The total timeout for exchanges that are not in the destination queue is enforced
+     * by {@link HttpChannel}.
+     */
     private class TimeoutTask extends CyclicTimeout
     {
         private final AtomicLong nextTimeout = new AtomicLong(Long.MAX_VALUE);
@@ -504,6 +508,9 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest
         @Override
         public void onTimeoutExpired()
         {
+            if (LOG.isDebugEnabled())
+                LOG.debug("{} timeout expired", this);
+
             nextTimeout.set(Long.MAX_VALUE);
             long now = System.nanoTime();
             long nextExpiresAt = Long.MAX_VALUE;
@@ -536,12 +543,16 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest
             if (timeoutAt != expiresAt)
             {
                 long delay = expiresAt - System.nanoTime();
-                if (LOG.isDebugEnabled())
-                    LOG.debug("Scheduled timeout in {} ms", TimeUnit.NANOSECONDS.toMillis(delay));
                 if (delay <= 0)
+                {
                     onTimeoutExpired();
+                }
                 else
+                {
                     schedule(delay, TimeUnit.NANOSECONDS);
+                    if (LOG.isDebugEnabled())
+                        LOG.debug("{} scheduled timeout in {} ms", this, TimeUnit.NANOSECONDS.toMillis(delay));
+                }
             }
         }
     }

jetty-client/src/main/java/org/eclipse/jetty/client/HttpRedirector.java

@@ -21,8 +21,10 @@ package org.eclipse.jetty.client;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.List;
+import java.util.Objects;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -101,11 +103,11 @@ public class HttpRedirector
     /**
      * Redirects the given {@code response}, blocking until the redirect is complete.
      *
-     * @param request the original request that triggered the redirect
+     * @param request  the original request that triggered the redirect
      * @param response the response to the original request
      * @return a {@link Result} object containing the request to the redirected location and its response
      * @throws InterruptedException if the thread is interrupted while waiting for the redirect to complete
-     * @throws ExecutionException if the redirect failed
+     * @throws ExecutionException   if the redirect failed
      * @see #redirect(Request, Response, Response.CompleteListener)
      */
     public Result redirect(Request request, Response response) throws InterruptedException, ExecutionException
@@ -144,7 +146,7 @@ public class HttpRedirector
     /**
      * Redirects the given {@code response} asynchronously.
      *
-     * @param request the original request that triggered the redirect
+     * @param request  the original request that triggered the redirect
      * @param response the response to the original request
      * @param listener the listener that receives response events
      * @return the request to the redirected location
@@ -157,6 +159,14 @@ public class HttpRedirector
             URI newURI = extractRedirectURI(response);
             if (newURI != null)
             {
+                boolean absolute = newURI.isAbsolute();
+                URI uri = request.getURI();
+                if (absolute && newURI.equals(uri) ||
+                        !absolute && Objects.equals(newURI.getPath(), uri.getPath()))
+                {
+                    fail(request, response, new HttpResponseException("Redirect to same URI: " + location, response));
+                    return null;
+                }
                 if (LOG.isDebugEnabled())
                     LOG.debug("Redirecting to {} (Location: {})", newURI, location);
                 return redirect(request, response, listener, newURI);
@@ -292,7 +302,8 @@ public class HttpRedirector
         Integer redirects = (Integer)conversation.getAttribute(ATTRIBUTE);
         if (redirects == null)
             redirects = 0;
-        if (redirects < client.getMaxRedirects())
+        int maxRedirects = client.getMaxRedirects();
+        if (maxRedirects < 0 || redirects < maxRedirects)
         {
             ++redirects;
             conversation.setAttribute(ATTRIBUTE, redirects);
@@ -310,19 +321,17 @@ public class HttpRedirector
         try
         {
             Request redirect = client.copyRequest(httpRequest, location);
+            // Disable the timeout so that only the one from the initial request applies.
+            redirect.timeout(0, TimeUnit.MILLISECONDS);
 
             // Use given method
             redirect.method(method);
 
-            redirect.onRequestBegin(new Request.BeginListener()
+            redirect.onRequestBegin(request ->
             {
-                @Override
+                Throwable cause = httpRequest.getAbortCause();
-                public void onBegin(Request redirect)
+                if (cause != null)
-                {
+                    request.abort(cause);
-                    Throwable cause = httpRequest.getAbortCause();
-                    if (cause != null)
-                        redirect.abort(cause);
-                }
             });
 
             redirect.send(listener);

jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java

@@ -26,7 +26,6 @@ import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.client.api.Result;
 import org.eclipse.jetty.io.CyclicTimeout;
-import org.eclipse.jetty.util.component.Destroyable;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 import org.eclipse.jetty.util.thread.Scheduler;
@@ -47,7 +46,7 @@ public class TimeoutCompleteListener extends CyclicTimeout implements Response.C
     {
         Request request = this.request.getAndSet(null);
         if (LOG.isDebugEnabled())
-            LOG.debug("Total timeout {} ms elapsed for {}", request.getTimeout(), request);
+            LOG.debug("Total timeout {} ms elapsed for {} on {}", request.getTimeout(), request, this);
         if (request != null)
             request.abort(new TimeoutException("Total timeout " + request.getTimeout() + " ms elapsed"));
     }
@@ -60,7 +59,7 @@ public class TimeoutCompleteListener extends CyclicTimeout implements Response.C
         {
             boolean cancelled = cancel();
             if (LOG.isDebugEnabled())
-                LOG.debug("Cancelled ({}) timeout for {}", cancelled, request);
+                LOG.debug("Cancelled ({}) timeout for {} on {}", cancelled, request, this);
         }
     }
 
@@ -69,12 +68,16 @@ public class TimeoutCompleteListener extends CyclicTimeout implements Response.C
         if (this.request.compareAndSet(null, request))
         {
             long delay = timeoutAt - System.nanoTime();
-            if (LOG.isDebugEnabled())
-                LOG.debug("Scheduled timeout in {} ms for {}", TimeUnit.NANOSECONDS.toMillis(delay), request);
             if (delay <= 0)
+            {
                 onTimeoutExpired();
+            }
             else
+            {
                 schedule(delay, TimeUnit.NANOSECONDS);
+                if (LOG.isDebugEnabled())
+                    LOG.debug("Scheduled timeout in {} ms for {} on {}", TimeUnit.NANOSECONDS.toMillis(delay), request, this);
+            }
         }
     }
 }

jetty-client/src/main/java/org/eclipse/jetty/client/api/Request.java

@@ -261,12 +261,14 @@ public interface Request
     Request idleTimeout(long timeout, TimeUnit unit);
 
     /**
-     * @return the total timeout for this request, in milliseconds
+     * @return the total timeout for this request, in milliseconds;
+     * zero or negative if the timeout is disabled
      */
     long getTimeout();
 
     /**
-     * @param timeout the total timeout for the request/response conversation
+     * @param timeout the total timeout for the request/response conversation;
+     *                use zero or a negative value to disable the timeout
      * @param unit the timeout unit
      * @return this request object
      */

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java

@@ -44,9 +44,11 @@ import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.client.api.Response.Listener;
 import org.eclipse.jetty.client.api.Result;
+import org.eclipse.jetty.client.util.AbstractAuthentication;
 import org.eclipse.jetty.client.util.BasicAuthentication;
 import org.eclipse.jetty.client.util.DeferredContentProvider;
 import org.eclipse.jetty.client.util.DigestAuthentication;
+import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.security.Authenticator;
 import org.eclipse.jetty.security.ConstraintMapping;
@@ -57,7 +59,6 @@ import org.eclipse.jetty.security.authentication.BasicAuthenticator;
 import org.eclipse.jetty.security.authentication.DigestAuthenticator;
 import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.server.Server;
-import org.eclipse.jetty.server.handler.AbstractHandler;
 import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
 import org.eclipse.jetty.util.Attributes;
 import org.eclipse.jetty.util.IO;
@@ -67,6 +68,7 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ArgumentsSource;
 
+import static org.eclipse.jetty.client.api.Authentication.ANY_REALM;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalToIgnoringCase;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -137,7 +139,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
     {
         startBasic(scenario, new EmptyServerHandler());
         URI uri = URI.create(scenario.getScheme() + "://localhost:" + connector.getLocalPort());
-        test_Authentication(scenario, new BasicAuthentication(uri, Authentication.ANY_REALM, "basic", "basic"));
+        test_Authentication(scenario, new BasicAuthentication(uri, ANY_REALM, "basic", "basic"));
     }
 
     @ParameterizedTest
@@ -155,7 +157,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
     {
         startDigest(scenario, new EmptyServerHandler());
         URI uri = URI.create(scenario.getScheme() + "://localhost:" + connector.getLocalPort());
-        test_Authentication(scenario, new DigestAuthentication(uri, Authentication.ANY_REALM, "digest", "digest"));
+        test_Authentication(scenario, new DigestAuthentication(uri, ANY_REALM, "digest", "digest"));
     }
 
     private void test_Authentication(final Scenario scenario, Authentication authentication) throws Exception
@@ -227,16 +229,19 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
     @ArgumentsSource(ScenarioProvider.class)
     public void test_BasicAuthentication_ThenRedirect(Scenario scenario) throws Exception
     {
-        startBasic(scenario, new AbstractHandler()
+        startBasic(scenario, new EmptyServerHandler()
         {
             private final AtomicInteger requests = new AtomicInteger();
 
             @Override
-            public void handle(String target, org.eclipse.jetty.server.Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
+            protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
             {
-                baseRequest.setHandled(true);
+                int r = requests.incrementAndGet();
-                if (requests.incrementAndGet() == 1)
+                if (r == 1)
-                    response.sendRedirect(URIUtil.newURI(scenario.getScheme(), request.getServerName(), request.getServerPort(), request.getRequestURI(), null));
+                {
+                    String path = request.getRequestURI() + "/" + r;
+                    response.sendRedirect(URIUtil.newURI(scenario.getScheme(), request.getServerName(), request.getServerPort(), path, null));
+                }
             }
         });
 
@@ -269,12 +274,11 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
     @ArgumentsSource(ScenarioProvider.class)
     public void test_Redirect_ThenBasicAuthentication(Scenario scenario) throws Exception
     {
-        startBasic(scenario, new AbstractHandler()
+        startBasic(scenario, new EmptyServerHandler()
         {
             @Override
-            public void handle(String target, org.eclipse.jetty.server.Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
+            protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
             {
-                baseRequest.setHandled(true);
                 if (request.getRequestURI().endsWith("/redirect"))
                     response.sendRedirect(URIUtil.newURI(scenario.getScheme(), request.getServerName(), request.getServerPort(), "/secure", null));
             }
@@ -571,61 +575,57 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
         assertTrue(resultLatch.await(5, TimeUnit.SECONDS));
     }
 
-    private static class GeneratingContentProvider implements ContentProvider
+    @ParameterizedTest
+    @ArgumentsSource(ScenarioProvider.class)
+    public void test_InfiniteAuthentication(Scenario scenario) throws Exception
     {
-        private static final ByteBuffer DONE = ByteBuffer.allocate(0);
+        String authType = "Authenticate";
-
+        start(scenario, new EmptyServerHandler()
-        private final IntFunction<ByteBuffer> generator;
-
-        private GeneratingContentProvider(IntFunction<ByteBuffer> generator)
         {
-            this.generator = generator;
+            @Override
-        }
+            protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response)
-
-        @Override
-        public long getLength()
-        {
-            return -1;
-        }
-
-        @Override
-        public boolean isReproducible()
-        {
-            return true;
-        }
-
-        @Override
-        public Iterator<ByteBuffer> iterator()
-        {
-            return new Iterator<ByteBuffer>()
             {
-                private int index;
+                // Always reply with a 401 to see if the client
-                public ByteBuffer current;
+                // can handle an infinite authentication loop.
+                response.setStatus(HttpStatus.UNAUTHORIZED_401);
+                response.setHeader(HttpHeader.WWW_AUTHENTICATE.asString(), authType);
+            }
+        });
 
-                @Override
+        AuthenticationStore authenticationStore = client.getAuthenticationStore();
-                @SuppressWarnings("ReferenceEquality")
+        URI uri = URI.create(scenario.getScheme() + "://localhost:" + connector.getLocalPort());
-                public boolean hasNext()
+        authenticationStore.addAuthentication(new AbstractAuthentication(uri, Authentication.ANY_REALM)
+        {
+            @Override
+            public String getType()
+            {
+                return authType;
+            }
+
+            @Override
+            public Result authenticate(Request request, ContentResponse response, HeaderInfo headerInfo, Attributes context)
+            {
+                return new Result()
                 {
-                    if (current == null)
+                    @Override
+                    public URI getURI()
                     {
-                        current = generator.apply(index++);
+                        return uri;
-                        if (current == null)
-                            current = DONE;
                     }
-                    return current != DONE;
-                }
 
-                @Override
+                    @Override
-                public ByteBuffer next()
+                    public void apply(Request request)
-                {
+                    {
-                    ByteBuffer result = current;
+                    }
-                    current = null;
+                };
-                    if (result == null)
+            }
-                        throw new NoSuchElementException();
+        });
-                    return result;
+
-                }
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
-            };
+                .scheme(scenario.getScheme())
-        }
+                .send();
+
+        assertEquals(HttpStatus.UNAUTHORIZED_401, response.getStatus());
     }
 
     @Test
@@ -801,4 +801,61 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
         assertEquals("thermostat", headerInfo.getParameter("realm"));
         assertEquals(headerInfo.getParameter("nonce"), "1523430383=");
     }
+
+    private static class GeneratingContentProvider implements ContentProvider
+    {
+        private static final ByteBuffer DONE = ByteBuffer.allocate(0);
+
+        private final IntFunction<ByteBuffer> generator;
+
+        private GeneratingContentProvider(IntFunction<ByteBuffer> generator)
+        {
+            this.generator = generator;
+        }
+
+        @Override
+        public long getLength()
+        {
+            return -1;
+        }
+
+        @Override
+        public boolean isReproducible()
+        {
+            return true;
+        }
+
+        @Override
+        public Iterator<ByteBuffer> iterator()
+        {
+            return new Iterator<ByteBuffer>()
+            {
+                private int index;
+                public ByteBuffer current;
+
+                @Override
+                @SuppressWarnings("ReferenceEquality")
+                public boolean hasNext()
+                {
+                    if (current == null)
+                    {
+                        current = generator.apply(index++);
+                        if (current == null)
+                            current = DONE;
+                    }
+                    return current != DONE;
+                }
+
+                @Override
+                public ByteBuffer next()
+                {
+                    ByteBuffer result = current;
+                    current = null;
+                    if (result == null)
+                        throw new NoSuchElementException();
+                    return result;
+                }
+            };
+        }
+    }
 }

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java

@@ -18,14 +18,6 @@
 
 package org.eclipse.jetty.client;
 
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.jupiter.api.Assertions.assertArrayEquals;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.IOException;
 import java.net.URLDecoder;
 import java.nio.ByteBuffer;
@@ -34,7 +26,9 @@ import java.nio.charset.StandardCharsets;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
 
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
@@ -48,13 +42,20 @@ import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.server.Request;
-import org.eclipse.jetty.server.handler.AbstractHandler;
 import org.eclipse.jetty.toolchain.test.IO;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ArgumentsSource;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 public class HttpClientRedirectTest extends AbstractHttpClientServerTest
 {
     @ParameterizedTest
@@ -128,14 +129,13 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     {
         start(scenario, new RedirectHandler());
 
-        ExecutionException x = assertThrows(ExecutionException.class, ()->{
+        ExecutionException x = assertThrows(ExecutionException.class, () ->
-            client.newRequest("localhost", connector.getLocalPort())
+                client.newRequest("localhost", connector.getLocalPort())
-                    .scheme(scenario.getScheme())
+                        .scheme(scenario.getScheme())
-                    .method(HttpMethod.DELETE)
+                        .method(HttpMethod.DELETE)
-                    .path("/301/localhost/done")
+                        .path("/301/localhost/done")
-                    .timeout(5, TimeUnit.SECONDS)
+                        .timeout(5, TimeUnit.SECONDS)
-                    .send();
+                        .send());
-        });
         HttpResponseException xx = (HttpResponseException)x.getCause();
         Response response = xx.getResponse();
         assertNotNull(response);
@@ -170,13 +170,12 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
         start(scenario, new RedirectHandler());
         client.setMaxRedirects(1);
 
-        ExecutionException x = assertThrows(ExecutionException.class, ()->{
+        ExecutionException x = assertThrows(ExecutionException.class, () ->
-            client.newRequest("localhost", connector.getLocalPort())
+                client.newRequest("localhost", connector.getLocalPort())
-                    .scheme(scenario.getScheme())
+                        .scheme(scenario.getScheme())
-                    .path("/303/localhost/302/localhost/done")
+                        .path("/303/localhost/302/localhost/done")
-                    .timeout(5, TimeUnit.SECONDS)
+                        .timeout(5, TimeUnit.SECONDS)
-                    .send();
+                        .send());
-        });
         HttpResponseException xx = (HttpResponseException)x.getCause();
         Response response = xx.getResponse();
         assertNotNull(response);
@@ -269,12 +268,11 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     @ArgumentsSource(ScenarioProvider.class)
     public void testRedirectWithWrongScheme(Scenario scenario) throws Exception
     {
-        start(scenario, new AbstractHandler()
+        start(scenario, new EmptyServerHandler()
         {
             @Override
-            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
+            protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response)
             {
-                baseRequest.setHandled(true);
                 response.setStatus(303);
                 response.setHeader("Location", "ssh://localhost:" + connector.getLocalPort() + "/path");
             }
@@ -439,12 +437,11 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
     public void testRedirectWithCorruptedBody(Scenario scenario) throws Exception
     {
         byte[] bytes = "ok".getBytes(StandardCharsets.UTF_8);
-        start(scenario, new AbstractHandler()
+        start(scenario, new EmptyServerHandler()
         {
             @Override
-            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
+            protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
             {
-                baseRequest.setHandled(true);
                 if (target.startsWith("/redirect"))
                 {
                     response.setStatus(HttpStatus.SEE_OTHER_303);
@@ -471,6 +468,64 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
         assertArrayEquals(bytes, response.getContent());
     }
 
+    @ParameterizedTest
+    @ArgumentsSource(ScenarioProvider.class)
+    public void testRedirectToSameURL(Scenario scenario) throws Exception
+    {
+        start(scenario, new EmptyServerHandler()
+        {
+            @Override
+            protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response)
+            {
+                response.setStatus(HttpStatus.SEE_OTHER_303);
+                response.setHeader(HttpHeader.LOCATION.asString(), request.getRequestURI());
+            }
+        });
+
+        ExecutionException x = assertThrows(ExecutionException.class, () ->
+        {
+            client.setMaxRedirects(-1);
+            client.newRequest("localhost", connector.getLocalPort())
+                    .scheme(scenario.getScheme())
+                    .timeout(5, TimeUnit.SECONDS)
+                    .send();
+        });
+        assertThat(x.getCause(), Matchers.instanceOf(HttpResponseException.class));
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(ScenarioProvider.class)
+    public void testInfiniteRedirectLoopMustTimeout(Scenario scenario) throws Exception
+    {
+        AtomicLong counter = new AtomicLong();
+        start(scenario, new EmptyServerHandler()
+        {
+            @Override
+            protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response)
+            {
+                try
+                {
+                    Thread.sleep(200);
+                    response.setStatus(HttpStatus.SEE_OTHER_303);
+                    response.setHeader(HttpHeader.LOCATION.asString(), "/" + counter.getAndIncrement());
+                }
+                catch (InterruptedException x)
+                {
+                    throw new RuntimeException(x);
+                }
+            }
+        });
+
+        assertThrows(TimeoutException.class, () ->
+        {
+            client.setMaxRedirects(-1);
+            client.newRequest("localhost", connector.getLocalPort())
+                    .scheme(scenario.getScheme())
+                    .timeout(1, TimeUnit.SECONDS)
+                    .send();
+        });
+    }
+
     private void testSameMethodRedirect(final Scenario scenario, final HttpMethod method, int redirectCode) throws Exception
     {
         testMethodRedirect(scenario, method, method, redirectCode);
@@ -519,10 +574,10 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
         assertEquals(200, response.getStatus());
     }
 
-    private class RedirectHandler extends AbstractHandler
+    private class RedirectHandler extends EmptyServerHandler
     {
         @Override
-        public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
+        protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
         {
             try
             {
@@ -551,10 +606,6 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
                 // Echo content back
                 IO.copy(request.getInputStream(), response.getOutputStream());
             }
-            finally
-            {
-                baseRequest.setHandled(true);
-            }
         }
     }
 }

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java

@@ -18,16 +18,6 @@
 
 package org.eclipse.jetty.client;
 
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.instanceOf;
-import static org.junit.jupiter.api.Assertions.assertArrayEquals;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -106,6 +96,16 @@ import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ArgumentsSource;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 @ExtendWith(WorkDirExtension.class)
 public class HttpClientTest extends AbstractHttpClientServerTest
 {