457032 - Request sent from a failed CompleteListener due to connect t…

…imeout is failed immediately. Fixed by copying the exchange queue and failing only the exchanges that are present in the copy.
jetty · Jan 8, 2015 · 51aafc7 · 51aafc7
1 parent 9618f45
commit 51aafc7
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 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
@@ -21,6 +21,7 @@
 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 void close(Connection connection)
      */
     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
@@ -29,7 +29,6 @@
 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.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 @@ protected boolean onReadTimeout()
         }
     }
 
-    @Ignore
     @Slow
     @Test
     public void testConnectTimeoutFailsRequest() throws Exception
@@ -333,10 +330,9 @@ public void onComplete(Result result)
         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 void onComplete(Result result)
         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
     {