From 20131601c188534a0f336e5b7cd6bdccb574602c Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 4 Oct 2022 15:17:19 +0200 Subject: [PATCH] Fixes #7993 - HttpClient idleTimeout configuration being ignored/overridden Fixed timeout handling. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/client/HttpProxy.java | 7 +++- .../client/http/HttpChannelOverHTTP.java | 5 ++- .../client/http/HttpConnectionOverHTTP.java | 31 ++++++++------- .../client/http/HttpReceiverOverHTTP.java | 1 + .../org/eclipse/jetty/io/IdleTimeout.java | 3 ++ .../proxy/ForwardProxyTLSServerTest.java | 39 +++++++++++++++++++ 6 files changed, 70 insertions(+), 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 index 8d91de7da517..9d32b20ec825 100644 --- 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 index 2f8dd692cf68..9ee061ce2f6e 100644 --- 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 index 31e8f107540b..d6df85d30e20 100644 --- 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,6 +201,16 @@ 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 @@ -208,6 +218,11 @@ public void release() 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 index 5440a0401ad7..f380fed317eb 100644 --- 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 index bfb18080375d..24040405717e 100644 --- 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 index cb0c3205732c..9c190a99db04 100644 --- 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