Fixes #7993 - HttpClient idleTimeout configuration being ignored/over…

…ridden The problem was that the timeout scheduling was not happening, because for TunnelRequest the timeouts were set in normalizeRequest(), which runs after the scheduling. Now a call to request.sent() is made also after normalizeRequest() so that the timeouts is scheduled (if it was not scheduled before). Signed-off-by: Simone Bordet <[email protected]>
jetty · Oct 1, 2022 · 38b1d41 · 38b1d41
1 parent 66aa945
commit 38b1d41
Show file tree

Hide file tree

Showing 8 changed files with 113 additions and 13 deletions.
diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java
@@ -106,6 +106,7 @@ protected SendFailure send(HttpChannel channel, HttpExchange exchange)
             SendFailure result;
             if (channel.associate(exchange))
             {
+                request.sent();
                 requestTimeouts.schedule(channel);
                 channel.send();
                 result = null;

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
@@ -311,6 +311,7 @@ public void send(HttpExchange exchange)
         {
             if (enqueue(exchanges, exchange))
             {
+                request.sent();
                 requestTimeouts.schedule(exchange);
                 if (!client.isRunning() && exchanges.remove(exchange))
                 {

diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpProxy.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpProxy.java
@@ -234,7 +234,7 @@ private void tunnelSucceeded(EndPoint endPoint)
 
         private void tunnelFailed(EndPoint endPoint, Throwable failure)
         {
-            endPoint.close();
+            endPoint.close(failure);
             promise.failed(failure);
         }
 

diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java
@@ -816,15 +816,17 @@ private void sendAsync(BiConsumer<HttpRequest, List<Response.ResponseListener>>
     {
         if (listener != null)
             responseListeners.add(listener);
-        sent();
         sender.accept(this, responseListeners);
     }
 
     void sent()
     {
-        long timeout = getTimeout();
-        if (timeout > 0)
-            timeoutNanoTime = NanoTime.now() + TimeUnit.MILLISECONDS.toNanos(timeout);
+        if (timeoutNanoTime == Long.MAX_VALUE)
+        {
+            long timeout = getTimeout();
+            if (timeout > 0)
+                timeoutNanoTime = NanoTime.now() + TimeUnit.MILLISECONDS.toNanos(timeout);
+        }
     }
 
     /**

diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpConnectionOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpConnectionOverHTTP.java
@@ -241,13 +241,13 @@ public boolean sweep()
     {
         if (!closed.get())
             return false;
-        if (sweeps.incrementAndGet() < 4)
-            return false;
-        return true;
+        return sweeps.incrementAndGet() > 3;
     }
 
     public void remove()
     {
+        // Restore idle timeout
+        getEndPoint().setIdleTimeout(idleTimeout);
         getHttpDestination().remove(this);
     }
 
@@ -301,8 +301,12 @@ protected void normalizeRequest(HttpRequest request)
             if (request instanceof HttpProxy.TunnelRequest)
             {
                 long connectTimeout = getHttpClient().getConnectTimeout();
+                // Use the connect timeout as a total timeout,
+                // since this request is to "connect" to the server.
                 request.timeout(connectTimeout, TimeUnit.MILLISECONDS)
-                        .idleTimeout(2 * connectTimeout, TimeUnit.MILLISECONDS);
+                    // Override the idle timeout in case
+                    // it is shorter than the connect timeout.
+                    .idleTimeout(2 * connectTimeout, TimeUnit.MILLISECONDS);
             }
 
             HttpConversation conversation = request.getConversation();

diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/SelectorManager.java b/jetty-io/src/main/java/org/eclipse/jetty/io/SelectorManager.java
@@ -166,7 +166,8 @@ protected ManagedSelector chooseSelector()
     public void connect(SelectableChannel channel, Object attachment)
     {
         ManagedSelector set = chooseSelector();
-        set.submit(set.new Connect(channel, attachment));
+        if (set != null)
+            set.submit(set.new Connect(channel, attachment));
     }
 
     /**

diff --git a/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ForwardProxyTLSServerTest.java b/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ForwardProxyTLSServerTest.java
@@ -24,6 +24,7 @@
 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.AtomicReference;
 import java.util.stream.Stream;
 import javax.net.ssl.KeyManager;
@@ -40,6 +41,7 @@
 import org.eclipse.jetty.client.api.Connection;
 import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.client.api.Destination;
+import org.eclipse.jetty.client.api.Result;
 import org.eclipse.jetty.client.http.HttpClientTransportOverHTTP;
 import org.eclipse.jetty.client.util.BasicAuthentication;
 import org.eclipse.jetty.client.util.FutureResponseListener;
@@ -61,7 +63,6 @@
 import org.eclipse.jetty.util.Promise;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
-import org.hamcrest.Matchers;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assumptions;
 import org.junit.jupiter.api.Disabled;
@@ -71,6 +72,7 @@
 import org.junit.jupiter.params.provider.MethodSource;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -413,7 +415,7 @@ public void testProxyDown(SslContextFactory.Server proxyTLS) throws Exception
                 .timeout(5, TimeUnit.SECONDS)
                 .send();
         });
-        assertThat(x.getCause(), Matchers.instanceOf(ConnectException.class));
+        assertThat(x.getCause(), instanceOf(ConnectException.class));
 
         httpClient.stop();
     }
@@ -829,6 +831,96 @@ public void onSuccess(org.eclipse.jetty.client.api.Request request)
         }
     }
 
+    @ParameterizedTest
+    @MethodSource("proxyTLS")
+    public void testServerLongProcessing(SslContextFactory.Server proxyTLS) throws Exception
+    {
+        long timeout = 500;
+        startTLSServer(new AbstractHandler()
+        {
+            @Override
+            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response)
+            {
+                sleep(3 * timeout);
+                baseRequest.setHandled(true);
+            }
+        });
+        startProxy(proxyTLS);
+        HttpClient httpClient = newHttpClient();
+        httpClient.getProxyConfiguration().getProxies().add(newHttpProxy());
+        httpClient.setConnectTimeout(timeout);
+        httpClient.setIdleTimeout(4 * timeout);
+        httpClient.start();
+
+        try
+        {
+            // The idle timeout is larger than the server processing time, request should succeed.
+            ContentResponse response = httpClient.newRequest("localhost", serverConnector.getLocalPort())
+                .scheme(HttpScheme.HTTPS.asString())
+                .send();
+
+            assertEquals(HttpStatus.OK_200, response.getStatus());
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
+    @ParameterizedTest
+    @MethodSource("proxyTLS")
+    public void testProxyLongProcessing(SslContextFactory.Server proxyTLS) throws Exception
+    {
+        long timeout = 500;
+        startTLSServer(new AbstractHandler()
+        {
+            @Override
+            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response)
+            {
+                baseRequest.setHandled(true);
+            }
+        });
+        startProxy(proxyTLS, new ConnectHandler()
+        {
+            @Override
+            protected void handleConnect(Request baseRequest, HttpServletRequest request, HttpServletResponse response, String serverAddress)
+            {
+                sleep(3 * timeout);
+                super.handleConnect(baseRequest, request, response, serverAddress);
+            }
+        });
+        HttpClient httpClient = newHttpClient();
+        httpClient.getProxyConfiguration().getProxies().add(newHttpProxy());
+        httpClient.setConnectTimeout(timeout);
+        httpClient.setIdleTimeout(10 * timeout);
+        httpClient.start();
+
+        try
+        {
+            // Connecting to the server through the proxy involves a CONNECT + 200
+            // so if the proxy delays the response, the client request interprets
+            // it as a "connect" timeout (rather than an idle timeout).
+            AtomicReference<Result> resultRef = new AtomicReference<>();
+            CountDownLatch latch = new CountDownLatch(1);
+            httpClient.newRequest("localhost", serverConnector.getLocalPort())
+                .scheme(HttpScheme.HTTPS.asString())
+                .send(result ->
+                {
+                    resultRef.set(result);
+                    latch.countDown();
+                });
+
+            assertTrue(latch.await(2 * timeout, TimeUnit.MILLISECONDS));
+            Result result = resultRef.get();
+            assertTrue(result.isFailed());
+            assertThat(result.getFailure(), instanceOf(TimeoutException.class));
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
     @Test
     @Tag("external")
     @Disabled

diff --git a/jetty-proxy/src/test/resources/jetty-logging.properties b/jetty-proxy/src/test/resources/jetty-logging.properties
@@ -1,4 +1,3 @@
-# Jetty Logging using jetty-slf4j-impl
 #org.eclipse.jetty.LEVEL=DEBUG
 #org.eclipse.jetty.client.LEVEL=DEBUG
 #org.eclipse.jetty.proxy.LEVEL=DEBUG