From 42335e62a8c8ae6a574bddd935af7efcfdff70fa Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sat, 1 Oct 2022 10:23:53 +0200 Subject: [PATCH] Fixes #7993 - HttpClient idleTimeout configuration being ignored/overridden 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 --- .../eclipse/jetty/client/HttpConnection.java | 1 + .../eclipse/jetty/client/HttpDestination.java | 1 + .../org/eclipse/jetty/client/HttpProxy.java | 2 +- .../org/eclipse/jetty/client/HttpRequest.java | 10 +- .../client/http/HttpConnectionOverHTTP.java | 12 ++- .../org/eclipse/jetty/io/SelectorManager.java | 3 +- .../proxy/ForwardProxyTLSServerTest.java | 96 ++++++++++++++++++- .../test/resources/jetty-logging.properties | 1 - 8 files changed, 113 insertions(+), 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 index 7120db1c1591..adb16b46745c 100644 --- 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 index 669346496a0d..2fc7766e9f81 100644 --- 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 index 14ac751d08de..8d91de7da517 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 @@ -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 index b8f2291114df..28a259ab4d12 100644 --- 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> { 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 index c875f4989df6..31e8f107540b 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 @@ -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 index 34f555e49656..351c3d43adbc 100644 --- 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 index e1835d4884a8..cb0c3205732c 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 @@ -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 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 index 7e7e5ce63843..d3a9e3c5f43f 100644 --- 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