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

…ridden Fixed timeout handling. Signed-off-by: Simone Bordet <[email protected]>
jetty · Oct 4, 2022 · 2013160 · 2013160
1 parent 42335e6
commit 2013160
Show file tree

Hide file tree

Showing 6 changed files with 70 additions and 16 deletions.
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
@@ -19,6 +19,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jetty.client.api.Connection;
 import org.eclipse.jetty.client.api.Destination;
@@ -195,10 +196,14 @@ private void tunnel(HttpDestination destination, Connection connection)
             String target = destination.getOrigin().getAddress().asString();
             Origin.Address proxyAddress = destination.getConnectAddress();
             HttpClient httpClient = destination.getHttpClient();
+            long connectTimeout = httpClient.getConnectTimeout();
             Request connect = new TunnelRequest(httpClient, proxyAddress)
                 .method(HttpMethod.CONNECT)
                 .path(target)
-                .headers(headers -> headers.put(HttpHeader.HOST, target));
+                .headers(headers -> headers.put(HttpHeader.HOST, target))
+                // Use the connect timeout as a total timeout,
+                // since this request is to "connect" to the server.
+                .timeout(connectTimeout, TimeUnit.MILLISECONDS);
             ProxyConfiguration.Proxy proxy = destination.getProxy();
             if (proxy.isSecure())
                 connect.scheme(HttpScheme.HTTPS.asString());

diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java
@@ -131,7 +131,10 @@ else if (sender.isShutdown() && response.getStatus() != HttpStatus.SWITCHING_PRO
         {
             if (LOG.isDebugEnabled())
                 LOG.debug("Closing, reason: {} - {}", closeReason, connection);
-            connection.close();
+            if (result.isFailed())
+                connection.close(result.getFailure());
+            else
+                connection.close();
         }
         else
         {

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
@@ -201,13 +201,28 @@ public ByteBuffer onUpgradeFrom()
         return receiver.onUpgradeFrom();
     }
 
+    void onResponseHeaders(HttpExchange exchange)
+    {
+        HttpRequest request = exchange.getRequest();
+        if (request instanceof HttpProxy.TunnelRequest)
+        {
+            // Restore idle timeout
+            getEndPoint().setIdleTimeout(idleTimeout);
+        }
+    }
+
     public void release()
     {
         // Restore idle timeout
         getEndPoint().setIdleTimeout(idleTimeout);
         getHttpDestination().release(this);
     }
 
+    public void remove()
+    {
+        getHttpDestination().remove(this);
+    }
+
     @Override
     public void close()
     {
@@ -244,13 +259,6 @@ public boolean sweep()
         return sweeps.incrementAndGet() > 3;
     }
 
-    public void remove()
-    {
-        // Restore idle timeout
-        getEndPoint().setIdleTimeout(idleTimeout);
-        getHttpDestination().remove(this);
-    }
-
     @Override
     public String toConnectionString()
     {
@@ -300,13 +308,8 @@ 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)
-                    // Override the idle timeout in case
-                    // it is shorter than the connect timeout.
-                    .idleTimeout(2 * connectTimeout, TimeUnit.MILLISECONDS);
+                // Override the idle timeout in case it is shorter than the connect timeout.
+                request.idleTimeout(2 * getHttpClient().getConnectTimeout(), TimeUnit.MILLISECONDS);
             }
 
             HttpConversation conversation = request.getConversation();

diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java
@@ -327,6 +327,7 @@ public boolean headerComplete()
 
         // Store the EndPoint is case of upgrades, tunnels, etc.
         exchange.getRequest().getConversation().setAttribute(EndPoint.class.getName(), getHttpConnection().getEndPoint());
+        getHttpConnection().onResponseHeaders(exchange);
         return !responseHeaders(exchange);
     }
 

diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/IdleTimeout.java b/jetty-io/src/main/java/org/eclipse/jetty/io/IdleTimeout.java
@@ -80,6 +80,9 @@ public void setIdleTimeout(long idleTimeout)
         long old = _idleTimeout;
         _idleTimeout = idleTimeout;
 
+        if (LOG.isDebugEnabled())
+            LOG.debug("Setting idle timeout {} -> {} on {}", old, idleTimeout, this);
+
         // Do we have an old timeout
         if (old > 0)
         {

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
@@ -867,6 +867,45 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
         }
     }
 
+    @ParameterizedTest
+    @MethodSource("proxyTLS")
+    public void testServerLongProcessingWithRequestIdleTimeout(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);
+        // Short idle timeout for HttpClient.
+        httpClient.setIdleTimeout(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())
+                // Long idle timeout for the request, should override that of the client.
+                .idleTimeout(4 * timeout, TimeUnit.MILLISECONDS)
+                .send();
+
+            assertEquals(HttpStatus.OK_200, response.getStatus());
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
     @ParameterizedTest
     @MethodSource("proxyTLS")
     public void testProxyLongProcessing(SslContextFactory.Server proxyTLS) throws Exception