From cda4af3ec90731dfedce70bca5b4d08272baef33 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 25 Mar 2014 18:18:14 +0100 Subject: [PATCH] 431103 - Complete listener not called if request times out before processing exchange. Fixed by forcing the abort of the exchange in [Pooling|Multiplex]HttpDestination. --- .../org/eclipse/jetty/client/HttpClient.java | 2 -- .../eclipse/jetty/client/HttpExchange.java | 2 +- .../client/MultiplexHttpDestination.java | 6 ++++-- .../jetty/client/PoolingHttpDestination.java | 9 ++++---- .../jetty/client/TimeoutCompleteListener.java | 3 ++- .../jetty/client/HttpClientTimeoutTest.java | 21 +++++++++++++++++++ 6 files changed, 32 insertions(+), 11 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java index 6fdec4ac606..79c59629ba7 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java @@ -105,7 +105,6 @@ public class HttpClient extends ContainerLifeCycle private static final Logger LOG = Log.getLogger(HttpClient.class); private final ConcurrentMap destinations = new ConcurrentHashMap<>(); - private final ConcurrentMap conversations = new ConcurrentHashMap<>(); private final List handlers = new ArrayList<>(); private final List requestListeners = new ArrayList<>(); private final AuthenticationStore authenticationStore = new HttpAuthenticationStore(); @@ -237,7 +236,6 @@ public class HttpClient extends ContainerLifeCycle destination.close(); destinations.clear(); - conversations.clear(); requestListeners.clear(); authenticationStore.clearAuthentications(); authenticationStore.clearAuthenticationResults(); diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpExchange.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpExchange.java index e87c25b1da7..704c9c3ccef 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpExchange.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpExchange.java @@ -43,7 +43,6 @@ public class HttpExchange private final HttpResponse response; private volatile Throwable requestFailure; private volatile Throwable responseFailure; - public HttpExchange(HttpDestination destination, HttpRequest request, List listeners) { @@ -205,6 +204,7 @@ public class HttpExchange { if (update(0b0101, cause) == 0b0101) { + LOG.debug("Failing {}: {}", this, cause); destination.getRequestNotifier().notifyFailure(request, cause); List listeners = getConversation().getResponseListeners(); ResponseNotifier responseNotifier = destination.getResponseNotifier(); diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java index 61bf60f4942..7404ce3ed1b 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/MultiplexHttpDestination.java @@ -102,9 +102,11 @@ public abstract class MultiplexHttpDestination extends Htt Throwable cause = request.getAbortCause(); if (cause != null) { - // If we have a non-null abort cause, it means that someone - // else has already aborted and notified, nothing do to here. LOG.debug("Aborted before processing {}: {}", exchange, cause); + // It may happen that the request is aborted before the exchange + // is created. Aborting the exchange a second time will result in + // a no-operation, so we just abort here to cover that edge case. + exchange.abort(cause); } else { diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java index 2fd57425ec3..6bba6a58205 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java @@ -96,9 +96,6 @@ public abstract class PoolingHttpDestination extends HttpD LOG.debug("Processing exchange {} on connection {}", exchange, connection); if (exchange == null) { - // TODO: review this part... may not be 100% correct - // TODO: e.g. is client is not running, there should be no need to close the connection - if (!connectionPool.release(connection)) connection.close(); @@ -114,9 +111,11 @@ public abstract class PoolingHttpDestination extends HttpD Throwable cause = request.getAbortCause(); if (cause != null) { - // If we have a non-null abort cause, it means that someone - // else has already aborted and notified, nothing do to here. LOG.debug("Aborted before processing {}: {}", exchange, cause); + // It may happen that the request is aborted before the exchange + // is created. Aborting the exchange a second time will result in + // a no-operation, so we just abort here to cover that edge case. + exchange.abort(cause); } else { diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java b/jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java index 652e1c59dd0..80859a439b5 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/TimeoutCompleteListener.java @@ -58,13 +58,14 @@ public class TimeoutCompleteListener implements Response.CompleteListener, Runna Scheduler.Task task = scheduler.schedule(this, timeout, TimeUnit.MILLISECONDS); if (this.task.getAndSet(task) != null) throw new IllegalStateException(); - LOG.debug("Scheduled timeout task {} in {} ms", task, timeout); + LOG.debug("Scheduled timeout task {} in {} ms for {}", task, timeout, request); return true; } @Override public void run() { + LOG.debug("Executing timeout task {} for {}", task, request); request.abort(new TimeoutException("Total timeout elapsed")); } } 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 4881aadaf00..84a18b66fec 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 @@ -364,6 +364,27 @@ public class HttpClientTimeoutTest extends AbstractHttpClientServerTest Assert.assertNotNull(request.getAbortCause()); } + @Test + public void testVeryShortTimeout() throws Exception + { + start(new EmptyServerHandler()); + + final CountDownLatch latch = new CountDownLatch(1); + client.newRequest("localhost", connector.getLocalPort()) + .scheme(scheme) + .timeout(1, TimeUnit.MILLISECONDS) // Very short timeout + .send(new Response.CompleteListener() + { + @Override + public void onComplete(Result result) + { + latch.countDown(); + } + }); + + Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); + } + private void assumeConnectTimeout(String host, int port, int connectTimeout) throws IOException { try (Socket socket = new Socket())