From 51aafc78a40304afb1eca73ecdfac50533e1f3b7 Mon Sep 17 00:00:00 2001
From: Simone Bordet <simone.bordet@gmail.com>
Date: Thu, 8 Jan 2015 16:56:59 +0100
Subject: [PATCH] 457032 - Request sent from a failed CompleteListener due to
 connect timeout is failed immediately.

Fixed by copying the exchange queue and failing only the exchanges
that are present in the copy.
---
 .../eclipse/jetty/client/HttpDestination.java |  9 ++--
 .../jetty/client/HttpClientTimeoutTest.java   | 49 +++++++++++++++++--
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java
index 3fd6dd932ae..c1d3c065ea2 100644
--- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java
+++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java
@@ -21,6 +21,7 @@ package org.eclipse.jetty.client;
 import java.io.Closeable;
 import java.io.IOException;
 import java.nio.channels.AsynchronousCloseException;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Queue;
 import java.util.concurrent.RejectedExecutionException;
@@ -233,9 +234,11 @@ public abstract class HttpDestination implements Destination, Closeable, Dumpabl
      */
     public void abort(Throwable cause)
     {
-        HttpExchange exchange;
-        // Just peek(), the abort() will remove it from the queue.
-        while ((exchange = exchanges.peek()) != null)
+        // Copy the queue of exchanges and fail only those that are queued at this moment.
+        // The application may queue another request from the failure/complete listener
+        // and we don't want to fail it immediately as if it was queued before the failure.
+        // The call to Request.abort() will remove the exchange from the exchanges queue.
+        for (HttpExchange exchange : new ArrayList<>(exchanges))
             exchange.getRequest().abort(cause);
     }
 
diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java
index d0512dcd21f..1a67edfdd8d 100644
--- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java
+++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java
@@ -29,7 +29,6 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
-
 import javax.net.ssl.SSLEngine;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
@@ -57,7 +56,6 @@ import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.hamcrest.Matchers;
 import org.junit.Assert;
 import org.junit.Assume;
-import org.junit.Ignore;
 import org.junit.Test;
 
 public class HttpClientTimeoutTest extends AbstractHttpClientServerTest
@@ -301,7 +299,6 @@ public class HttpClientTimeoutTest extends AbstractHttpClientServerTest
         }
     }
 
-    @Ignore
     @Slow
     @Test
     public void testConnectTimeoutFailsRequest() throws Exception
@@ -333,10 +330,9 @@ public class HttpClientTimeoutTest extends AbstractHttpClientServerTest
         Assert.assertNotNull(request.getAbortCause());
     }
 
-    @Ignore
     @Slow
     @Test
-    public void testConnectTimeoutIsCancelledByShorterTimeout() throws Exception
+    public void testConnectTimeoutIsCancelledByShorterRequestTimeout() throws Exception
     {
         String host = "10.255.255.1";
         int port = 80;
@@ -368,6 +364,49 @@ public class HttpClientTimeoutTest extends AbstractHttpClientServerTest
         Assert.assertNotNull(request.getAbortCause());
     }
 
+    @Test
+    public void retryAfterConnectTimeout() throws Exception
+    {
+        final String host = "10.255.255.1";
+        final int port = 80;
+        int connectTimeout = 1000;
+        assumeConnectTimeout(host, port, connectTimeout);
+
+        start(new EmptyServerHandler());
+        client.stop();
+        client.setConnectTimeout(connectTimeout);
+        client.start();
+
+        final CountDownLatch latch = new CountDownLatch(1);
+        Request request = client.newRequest(host, port);
+        request.scheme(scheme)
+                .send(new Response.CompleteListener()
+                {
+                    @Override
+                    public void onComplete(Result result)
+                    {
+                        if (result.isFailed())
+                        {
+                            // Retry
+                            client.newRequest(host, port)
+                                    .scheme(scheme)
+                                    .send(new Response.CompleteListener()
+                                    {
+                                        @Override
+                                        public void onComplete(Result result)
+                                        {
+                                            if (result.isFailed())
+                                                latch.countDown();
+                                        }
+                                    });
+                        }
+                    }
+                });
+
+        Assert.assertTrue(latch.await(333 * connectTimeout, TimeUnit.MILLISECONDS));
+        Assert.assertNotNull(request.getAbortCause());
+    }
+
     @Test
     public void testVeryShortTimeout() throws Exception
     {