413387 - onResponseHeaders is not called multiple times when multiple

redirects occur.

Refactored the redirection code into HttpRedirector to ease
applications that need to access the redirect URI and then redirect.

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

@@ -37,6 +37,7 @@ import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
 import org.eclipse.jetty.client.api.AuthenticationStore;
@@ -388,7 +389,10 @@ public class HttpClient extends ContainerLifeCycle
         Request newRequest = new HttpRequest(this, oldRequest.getConversationID(), newURI);
         newRequest.method(oldRequest.getMethod())
                 .version(oldRequest.getVersion())
-                .content(oldRequest.getContent());
+                .content(oldRequest.getContent())
+                .idleTimeout(oldRequest.getIdleTimeout(), TimeUnit.MILLISECONDS)
+                .timeout(oldRequest.getTimeout(), TimeUnit.MILLISECONDS)
+                .followRedirects(oldRequest.isFollowRedirects());
         for (HttpField header : oldRequest.getHeaders())
         {
             // We have a new URI, so skip the host header if present

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

@@ -0,0 +1,320 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2013 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.client;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+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.client.util.BufferingResponseListener;
+import org.eclipse.jetty.http.HttpMethod;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
+
+/**
+ * Utility class that handles HTTP redirects.
+ * <p />
+ * Applications can disable redirection via {@link Request#followRedirects(boolean)}
+ * and then rely on this class to perform the redirect in a simpler way, for example:
+ * <pre>
+ * HttpRedirector redirector = new HttpRedirector(httpClient);
+ *
+ * Request request = httpClient.newRequest("http://host/path").followRedirects(false);
+ * ContentResponse response = request.send();
+ * while (redirector.isRedirect(response))
+ * {
+ *     // Validate the redirect URI
+ *     if (!validate(redirector.extractRedirectURI(response)))
+ *         break;
+ *
+ *     Result result = redirector.redirect(request, response);
+ *     request = result.getRequest();
+ *     response = result.getResponse();
+ * }
+ * </pre>
+ */
+public class HttpRedirector
+{
+    private static final Logger LOG = Log.getLogger(HttpRedirector.class);
+    private static final String SCHEME_REGEXP = "(^https?)";
+    private static final String AUTHORITY_REGEXP = "([^/\\?#]+)";
+    // The location may be relative so the scheme://authority part may be missing
+    private static final String DESTINATION_REGEXP = "(" + SCHEME_REGEXP + "://" + AUTHORITY_REGEXP + ")?";
+    private static final String PATH_REGEXP = "([^\\?#]*)";
+    private static final String QUERY_REGEXP = "([^#]*)";
+    private static final String FRAGMENT_REGEXP = "(.*)";
+    private static final Pattern URI_PATTERN = Pattern.compile(DESTINATION_REGEXP + PATH_REGEXP + QUERY_REGEXP + FRAGMENT_REGEXP);
+    private static final String ATTRIBUTE = HttpRedirector.class.getName() + ".redirects";
+
+    private final HttpClient client;
+    private final ResponseNotifier notifier;
+
+    public HttpRedirector(HttpClient client)
+    {
+        this.client = client;
+        this.notifier = new ResponseNotifier(client);
+    }
+
+    /**
+     * @param response the response to check for redirects
+     * @return whether the response code is a HTTP redirect code
+     */
+    public boolean isRedirect(Response response)
+    {
+        switch (response.getStatus())
+        {
+            case 301:
+            case 302:
+            case 303:
+            case 307:
+                return true;
+            default:
+                return false;
+        }
+    }
+
+    /**
+     * Redirects the given {@code response}, blocking until the redirect is complete.
+     *
+     * @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
+     * @see #redirect(Request, Response, Response.CompleteListener)
+     */
+    public Result redirect(Request request, Response response) throws InterruptedException, ExecutionException
+    {
+        final AtomicReference<Result> resultRef = new AtomicReference<>();
+        final CountDownLatch latch = new CountDownLatch(1);
+        Request redirect = redirect(request, response, new BufferingResponseListener()
+        {
+            @Override
+            public void onComplete(Result result)
+            {
+                resultRef.set(new Result(result.getRequest(),
+                        result.getRequestFailure(),
+                        new HttpContentResponse(result.getResponse(), getContent(), getEncoding()),
+                        result.getResponseFailure()));
+                latch.countDown();
+            }
+        });
+
+        try
+        {
+            latch.await();
+            Result result = resultRef.get();
+            if (result.isFailed())
+                throw new ExecutionException(result.getFailure());
+            return result;
+        }
+        catch (InterruptedException x)
+        {
+            // If the application interrupts, we need to abort the redirect
+            redirect.abort(x);
+            throw x;
+        }
+    }
+
+    /**
+     * Redirects the given {@code response} asynchronously.
+     *
+     * @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
+     */
+    public Request redirect(Request request, Response response, Response.CompleteListener listener)
+    {
+        if (isRedirect(response))
+        {
+            String location = response.getHeaders().get("Location");
+            URI newURI = extractRedirectURI(response);
+            if (newURI != null)
+            {
+                LOG.debug("Redirecting to {} (Location: {})", newURI, location);
+                return redirect(request, response, listener, newURI);
+            }
+            else
+            {
+                fail(request, response, new HttpResponseException("Invalid 'Location' header: " + location, response));
+                return null;
+            }
+        }
+        else
+        {
+            fail(request, response, new HttpResponseException("Cannot redirect: " + response, response));
+            return null;
+        }
+    }
+
+    /**
+     * Extracts and sanitizes (by making it absolute and escaping paths and query parameters)
+     * the redirect URI of the given {@code response}.
+     *
+     * @param response the response to extract the redirect URI from
+     * @return the absolute redirect URI, or null if the response does not contain a valid redirect location
+     */
+    public URI extractRedirectURI(Response response)
+    {
+        String location = response.getHeaders().get("location");
+        if (location != null)
+            return sanitize(location);
+        return null;
+    }
+
+    private URI sanitize(String location)
+    {
+        // Redirects should be valid, absolute, URIs, with properly escaped paths and encoded
+        // query parameters. However, shit happens, and here we try our best to recover.
+
+        try
+        {
+            // Direct hit first: if passes, we're good
+            return new URI(location);
+        }
+        catch (URISyntaxException x)
+        {
+            Matcher matcher = URI_PATTERN.matcher(location);
+            if (matcher.matches())
+            {
+                String scheme = matcher.group(2);
+                String authority = matcher.group(3);
+                String path = matcher.group(4);
+                String query = matcher.group(5);
+                if (query.length() == 0)
+                    query = null;
+                String fragment = matcher.group(6);
+                if (fragment.length() == 0)
+                    fragment = null;
+                try
+                {
+                    return new URI(scheme, authority, path, query, fragment);
+                }
+                catch (URISyntaxException xx)
+                {
+                    // Give up
+                }
+            }
+            return null;
+        }
+    }
+
+    private Request redirect(Request request, Response response, Response.CompleteListener listener, URI newURI)
+    {
+        if (!newURI.isAbsolute())
+            newURI = request.getURI().resolve(newURI);
+
+        int status = response.getStatus();
+        switch (status)
+        {
+            case 301:
+            {
+                String method = request.getMethod();
+                if (HttpMethod.GET.is(method) || HttpMethod.HEAD.is(method) || HttpMethod.PUT.is(method))
+                    return redirect(request, response, listener, newURI, method);
+                else if (HttpMethod.POST.is(method))
+                    return redirect(request, response, listener, newURI, HttpMethod.GET.asString());
+                fail(request, response, new HttpResponseException("HTTP protocol violation: received 301 for non GET/HEAD/POST/PUT request", response));
+                return null;
+            }
+            case 302:
+            {
+                String method = request.getMethod();
+                if (HttpMethod.HEAD.is(method) || HttpMethod.PUT.is(method))
+                    return redirect(request, response, listener, newURI, method);
+                else
+                    return redirect(request, response, listener, newURI, HttpMethod.GET.asString());
+            }
+            case 303:
+            {
+                String method = request.getMethod();
+                if (HttpMethod.HEAD.is(method))
+                    return redirect(request, response, listener, newURI, method);
+                else
+                    return redirect(request, response, listener, newURI, HttpMethod.GET.asString());
+            }
+            case 307:
+            {
+                // Keep same method
+                return redirect(request, response, listener, newURI, request.getMethod());
+            }
+            default:
+            {
+                fail(request, response, new HttpResponseException("Unhandled HTTP status code " + status, response));
+                return null;
+            }
+        }
+    }
+
+    private Request redirect(final Request request, Response response, Response.CompleteListener listener, URI location, String method)
+    {
+        HttpConversation conversation = client.getConversation(request.getConversationID(), false);
+        Integer redirects = conversation == null ? Integer.valueOf(0) : (Integer)conversation.getAttribute(ATTRIBUTE);
+        if (redirects == null)
+            redirects = 0;
+        if (redirects < client.getMaxRedirects())
+        {
+            ++redirects;
+            if (conversation != null)
+                conversation.setAttribute(ATTRIBUTE, redirects);
+
+            Request redirect = client.copyRequest(request, location);
+
+            // Use given method
+            redirect.method(method);
+
+            redirect.onRequestBegin(new Request.BeginListener()
+            {
+                @Override
+                public void onBegin(Request redirect)
+                {
+                    Throwable cause = request.getAbortCause();
+                    if (cause != null)
+                        redirect.abort(cause);
+                }
+            });
+
+            redirect.send(listener);
+            return redirect;
+        }
+        else
+        {
+            fail(request, response, new HttpResponseException("Max redirects exceeded " + redirects, response));
+            return null;
+        }
+    }
+
+    protected void fail(Request request, Response response, Throwable failure)
+    {
+        HttpConversation conversation = client.getConversation(request.getConversationID(), false);
+        conversation.updateResponseListeners(null);
+        List<Response.ResponseListener> listeners = conversation.getResponseListeners();
+        notifier.notifyFailure(listeners, response, failure);
+        notifier.notifyComplete(listeners, new Result(request, response, failure));
+    }
+}

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

@@ -466,12 +466,12 @@ public class HttpRequest implements Request
         FutureResponseListener listener = new FutureResponseListener(this);
         send(this, listener);
 
-        long timeout = getTimeout();
-        if (timeout <= 0)
-            return listener.get();
-
         try
         {
+            long timeout = getTimeout();
+            if (timeout <= 0)
+                return listener.get();
+
             return listener.get(timeout, TimeUnit.MILLISECONDS);
         }
         catch (InterruptedException | TimeoutException x)

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

@@ -18,53 +18,23 @@
 
 package org.eclipse.jetty.client;
 
-import java.net.URI;
-import java.net.URISyntaxException;
-import java.util.List;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
 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.http.HttpMethod;
-import org.eclipse.jetty.util.log.Log;
-import org.eclipse.jetty.util.log.Logger;
 
 public class RedirectProtocolHandler extends Response.Listener.Empty implements ProtocolHandler
 {
-    private static final Logger LOG = Log.getLogger(RedirectProtocolHandler.class);
-    private static final String SCHEME_REGEXP = "(^https?)";
-    private static final String AUTHORITY_REGEXP = "([^/\\?#]+)";
-    // The location may be relative so the scheme://authority part may be missing
-    private static final String DESTINATION_REGEXP = "(" + SCHEME_REGEXP + "://" + AUTHORITY_REGEXP + ")?";
-    private static final String PATH_REGEXP = "([^\\?#]*)";
-    private static final String QUERY_REGEXP = "([^#]*)";
-    private static final String FRAGMENT_REGEXP = "(.*)";
-    private static final Pattern URI_PATTERN = Pattern.compile(DESTINATION_REGEXP + PATH_REGEXP + QUERY_REGEXP + FRAGMENT_REGEXP);
-    private static final String ATTRIBUTE = RedirectProtocolHandler.class.getName() + ".redirects";
-
-    private final HttpClient client;
-    private final ResponseNotifier notifier;
+    private final HttpRedirector redirector;
 
     public RedirectProtocolHandler(HttpClient client)
     {
-        this.client = client;
-        this.notifier = new ResponseNotifier(client);
+        redirector = new HttpRedirector(client);
     }
 
     @Override
     public boolean accept(Request request, Response response)
     {
-        switch (response.getStatus())
-        {
-            case 301:
-            case 302:
-            case 303:
-            case 307:
-                return request.isFollowRedirects();
-        }
-        return false;
+        return redirector.isRedirect(response) && request.isFollowRedirects();
     }
 
     @Override
@@ -75,164 +45,12 @@ public class RedirectProtocolHandler extends Response.Listener.Empty implements
 
     @Override
     public void onComplete(Result result)
-    {
-        if (!result.isFailed())
-        {
-            Request request = result.getRequest();
-            Response response = result.getResponse();
-            String location = response.getHeaders().get("location");
-            if (location != null)
-            {
-                URI newURI = sanitize(location);
-                LOG.debug("Redirecting to {} (Location: {})", newURI, location);
-                if (newURI != null)
-                {
-                    if (!newURI.isAbsolute())
-                        newURI = request.getURI().resolve(newURI);
-
-                    int status = response.getStatus();
-                    switch (status)
-                    {
-                        case 301:
-                        {
-                            String method = request.getMethod();
-                            if (HttpMethod.GET.is(method) || HttpMethod.HEAD.is(method) || HttpMethod.PUT.is(method))
-                                redirect(result, method, newURI);
-                            else if (HttpMethod.POST.is(method))
-                                redirect(result, HttpMethod.GET.asString(), newURI);
-                            else
-                                fail(result, new HttpResponseException("HTTP protocol violation: received 301 for non GET/HEAD/POST/PUT request", response));
-                            break;
-                        }
-                        case 302:
-                        {
-                            String method = request.getMethod();
-                            if (HttpMethod.HEAD.is(method) || HttpMethod.PUT.is(method))
-                                redirect(result, method, newURI);
-                            else
-                                redirect(result, HttpMethod.GET.asString(), newURI);
-                            break;
-                        }
-                        case 303:
-                        {
-                            String method = request.getMethod();
-                            if (HttpMethod.HEAD.is(method))
-                                redirect(result, method, newURI);
-                            else
-                                redirect(result, HttpMethod.GET.asString(), newURI);
-                            break;
-                        }
-                        case 307:
-                        {
-                            // Keep same method
-                            redirect(result, request.getMethod(), newURI);
-                            break;
-                        }
-                        default:
-                        {
-                            fail(result, new HttpResponseException("Unhandled HTTP status code " + status, response));
-                            break;
-                        }
-                    }
-                }
-                else
-                {
-                    fail(result, new HttpResponseException("Malformed Location header " + location, response));
-                }
-            }
-            else
-            {
-                fail(result, new HttpResponseException("Missing Location header " + location, response));
-            }
-        }
-        else
-        {
-            fail(result, result.getFailure());
-        }
-    }
-
-    private URI sanitize(String location)
-    {
-        // Redirects should be valid, absolute, URIs, with properly escaped paths and encoded
-        // query parameters. However, shit happens, and here we try our best to recover.
-
-        try
-        {
-            // Direct hit first: if passes, we're good
-            return new URI(location);
-        }
-        catch (URISyntaxException x)
-        {
-            Matcher matcher = URI_PATTERN.matcher(location);
-            if (matcher.matches())
-            {
-                String scheme = matcher.group(2);
-                String authority = matcher.group(3);
-                String path = matcher.group(4);
-                String query = matcher.group(5);
-                if (query.length() == 0)
-                    query = null;
-                String fragment = matcher.group(6);
-                if (fragment.length() == 0)
-                    fragment = null;
-                try
-                {
-                    return new URI(scheme, authority, path, query, fragment);
-                }
-                catch (URISyntaxException xx)
-                {
-                    // Give up
-                }
-            }
-            return null;
-        }
-    }
-
-    private void redirect(Result result, String method, URI location)
-    {
-        final Request request = result.getRequest();
-        HttpConversation conversation = client.getConversation(request.getConversationID(), false);
-        Integer redirects = (Integer)conversation.getAttribute(ATTRIBUTE);
-        if (redirects == null)
-            redirects = 0;
-
-        if (redirects < client.getMaxRedirects())
-        {
-            ++redirects;
-            conversation.setAttribute(ATTRIBUTE, redirects);
-
-            Request redirect = client.copyRequest(request, location);
-
-            // Use given method
-            redirect.method(method);
-
-            redirect.onRequestBegin(new Request.BeginListener()
-            {
-                @Override
-                public void onBegin(Request redirect)
-                {
-                    Throwable cause = request.getAbortCause();
-                    if (cause != null)
-                        redirect.abort(cause);
-                }
-            });
-
-            redirect.send(null);
-        }
-        else
-        {
-            fail(result, new HttpResponseException("Max redirects exceeded " + redirects, result.getResponse()));
-        }
-    }
-
-    private void fail(Result result, Throwable failure)
     {
         Request request = result.getRequest();
         Response response = result.getResponse();
-        HttpConversation conversation = client.getConversation(request.getConversationID(), false);
-        conversation.updateResponseListeners(null);
-        List<Response.ResponseListener> listeners = conversation.getResponseListeners();
-        notifier.notifyFailure(listeners, response, failure);
-        notifier.notifyComplete(listeners, new Result(request, response, failure));
+        if (result.isSucceeded())
+            redirector.redirect(request, response, null);
+        else
+            redirector.fail(request, response, result.getFailure());
     }
 }

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

@@ -22,6 +22,7 @@ import java.io.IOException;
 import java.net.URLDecoder;
 import java.nio.ByteBuffer;
 import java.nio.channels.UnresolvedAddressException;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -31,6 +32,7 @@ import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.client.api.Response;
+import org.eclipse.jetty.client.api.Result;
 import org.eclipse.jetty.client.util.ByteBufferContentProvider;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
@@ -44,8 +46,6 @@ import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
-import static org.junit.Assert.fail;
-
 public class HttpClientRedirectTest extends AbstractHttpClientServerTest
 {
     public HttpClientRedirectTest(SslContextFactory sslContextFactory)
@@ -123,7 +123,7 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
                     .path("/301/localhost/done")
                     .timeout(5, TimeUnit.SECONDS)
                     .send();
-            fail();
+            Assert.fail();
         }
         catch (ExecutionException x)
         {
@@ -164,7 +164,7 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
                     .path("/303/localhost/302/localhost/done")
                     .timeout(5, TimeUnit.SECONDS)
                     .send();
-            fail();
+            Assert.fail();
         }
         catch (ExecutionException x)
         {
@@ -331,6 +331,43 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
         testSameMethodRedirect(HttpMethod.PUT, HttpStatus.TEMPORARY_REDIRECT_307);
     }
 
+    @Test
+    public void testHttpRedirector() throws Exception
+    {
+        final HttpRedirector redirector = new HttpRedirector(client);
+
+        org.eclipse.jetty.client.api.Request request1 = client.newRequest("localhost", connector.getLocalPort())
+                .scheme(scheme)
+                .path("/303/localhost/302/localhost/done")
+                .timeout(5, TimeUnit.SECONDS)
+                .followRedirects(false);
+        ContentResponse response1 = request1.send();
+
+        Assert.assertEquals(303, response1.getStatus());
+        Assert.assertTrue(redirector.isRedirect(response1));
+
+        Result result = redirector.redirect(request1, response1);
+        org.eclipse.jetty.client.api.Request request2 = result.getRequest();
+        Response response2 = result.getResponse();
+
+        Assert.assertEquals(302, response2.getStatus());
+        Assert.assertTrue(redirector.isRedirect(response2));
+
+        final CountDownLatch latch = new CountDownLatch(1);
+        redirector.redirect(request2, response2, new Response.CompleteListener()
+        {
+            @Override
+            public void onComplete(Result result)
+            {
+                Response response3 = result.getResponse();
+                Assert.assertEquals(200, response3.getStatus());
+                Assert.assertFalse(redirector.isRedirect(response3));
+                latch.countDown();
+            }
+        });
+        Assert.assertTrue(latch.await(5, TimeUnit.SECONDS));
+    }
+
     private void testSameMethodRedirect(final HttpMethod method, int redirectCode) throws Exception
     {
         testMethodRedirect(method, method, redirectCode);