From 88aca4fac53c2ef0e4e5e2754fe1944934e7ac4d Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 30 Sep 2022 16:38:02 +0200 Subject: [PATCH] Fixes #8584 - HttpRequest.send() never returns Fixed handling of the idle timeout in case the SOCKS proxy does not reply to the SOCKS bytes. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/client/Socks4Proxy.java | 12 ++- .../eclipse/jetty/client/Socks4ProxyTest.java | 93 +++++++++++++++++-- 2 files changed, 94 insertions(+), 11 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java b/jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java index 529715ffde4f..212653c60619 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java @@ -19,6 +19,7 @@ import java.nio.charset.StandardCharsets; import java.util.Map; import java.util.concurrent.Executor; +import java.util.concurrent.TimeoutException; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -140,12 +141,21 @@ public void succeeded() @Override public void failed(Throwable x) { - close(); + if (LOG.isDebugEnabled()) + LOG.debug("SOCKS4 failure", x); + getEndPoint().close(x); @SuppressWarnings("unchecked") Promise promise = (Promise)context.get(HttpClientTransport.HTTP_CONNECTION_PROMISE_CONTEXT_KEY); promise.failed(x); } + @Override + public boolean onIdleExpired() + { + failed(new TimeoutException("Idle timeout expired")); + return false; + } + @Override public void onFillable() { diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/Socks4ProxyTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/Socks4ProxyTest.java index 3b5b70fbbae2..97770100b265 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/Socks4ProxyTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/Socks4ProxyTest.java @@ -13,6 +13,7 @@ package org.eclipse.jetty.client; +import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.InetSocketAddress; @@ -21,11 +22,15 @@ import java.nio.channels.SocketChannel; import java.nio.charset.StandardCharsets; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLSocket; +import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.http.HttpClientTransportOverHTTP; +import org.eclipse.jetty.client.util.FutureResponseListener; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.io.ClientConnector; import org.eclipse.jetty.util.ssl.SslContextFactory; @@ -34,19 +39,22 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +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; public class Socks4ProxyTest { - private ServerSocketChannel server; + private ServerSocketChannel proxy; private HttpClient client; @BeforeEach public void prepare() throws Exception { - server = ServerSocketChannel.open(); - server.bind(new InetSocketAddress("localhost", 0)); + proxy = ServerSocketChannel.open(); + proxy.bind(new InetSocketAddress("localhost", 0)); ClientConnector connector = new ClientConnector(); QueuedThreadPool clientThreads = new QueuedThreadPool(); @@ -62,13 +70,13 @@ public void prepare() throws Exception public void dispose() throws Exception { client.stop(); - server.close(); + proxy.close(); } @Test public void testSocks4Proxy() throws Exception { - int proxyPort = server.socket().getLocalPort(); + int proxyPort = proxy.socket().getLocalPort(); client.getProxyConfiguration().getProxies().add(new Socks4Proxy("localhost", proxyPort)); CountDownLatch latch = new CountDownLatch(1); @@ -91,7 +99,7 @@ public void testSocks4Proxy() throws Exception latch.countDown(); }); - try (SocketChannel channel = server.accept()) + try (SocketChannel channel = proxy.accept()) { int socks4MessageLength = 9; ByteBuffer buffer = ByteBuffer.allocate(socks4MessageLength); @@ -130,7 +138,7 @@ public void testSocks4Proxy() throws Exception @Test public void testSocks4ProxyWithSplitResponse() throws Exception { - int proxyPort = server.socket().getLocalPort(); + int proxyPort = proxy.socket().getLocalPort(); client.getProxyConfiguration().getProxies().add(new Socks4Proxy("localhost", proxyPort)); CountDownLatch latch = new CountDownLatch(1); @@ -150,7 +158,7 @@ public void testSocks4ProxyWithSplitResponse() throws Exception result.getFailure().printStackTrace(); }); - try (SocketChannel channel = server.accept()) + try (SocketChannel channel = proxy.accept()) { int socks4MessageLength = 9; ByteBuffer buffer = ByteBuffer.allocate(socks4MessageLength); @@ -189,7 +197,7 @@ public void testSocks4ProxyWithSplitResponse() throws Exception public void testSocks4ProxyWithTLSServer() throws Exception { String proxyHost = "localhost"; - int proxyPort = server.socket().getLocalPort(); + int proxyPort = proxy.socket().getLocalPort(); String serverHost = "127.0.0.13"; // Server host different from proxy host. int serverPort = proxyPort + 1; // Any port will do. @@ -221,7 +229,7 @@ public void testSocks4ProxyWithTLSServer() throws Exception result.getFailure().printStackTrace(); }); - try (SocketChannel channel = server.accept()) + try (SocketChannel channel = proxy.accept()) { int socks4MessageLength = 9; ByteBuffer buffer = ByteBuffer.allocate(socks4MessageLength); @@ -269,4 +277,69 @@ public void testSocks4ProxyWithTLSServer() throws Exception assertTrue(latch.await(5, TimeUnit.SECONDS)); } } + + @Test + public void testRequestTimeoutWhenSocksProxyDoesNotRespond() throws Exception + { + String proxyHost = "localhost"; + int proxyPort = proxy.socket().getLocalPort(); + client.getProxyConfiguration().getProxies().add(new Socks4Proxy(proxyHost, proxyPort)); + + long timeout = 1000; + Request request = client.newRequest("localhost", proxyPort + 1) + .timeout(timeout, TimeUnit.MILLISECONDS); + FutureResponseListener listener = new FutureResponseListener(request); + request.send(listener); + + try (SocketChannel ignored = proxy.accept()) + { + // Accept the connection, but do not reply and don't close. + + ExecutionException x = assertThrows(ExecutionException.class, () -> listener.get(2 * timeout, TimeUnit.MILLISECONDS)); + assertThat(x.getCause(), instanceOf(TimeoutException.class)); + } + } + + @Test + public void testIdleTimeoutWhenSocksProxyDoesNotRespond() throws Exception + { + String proxyHost = "localhost"; + int proxyPort = proxy.socket().getLocalPort(); + client.getProxyConfiguration().getProxies().add(new Socks4Proxy(proxyHost, proxyPort)); + long idleTimeout = 1000; + client.setIdleTimeout(idleTimeout); + + Request request = client.newRequest("localhost", proxyPort + 1); + FutureResponseListener listener = new FutureResponseListener(request); + request.send(listener); + + try (SocketChannel ignored = proxy.accept()) + { + // Accept the connection, but do not reply and don't close. + + ExecutionException x = assertThrows(ExecutionException.class, () -> listener.get(2 * idleTimeout, TimeUnit.MILLISECONDS)); + assertThat(x.getCause(), instanceOf(TimeoutException.class)); + } + } + + @Test + public void testSocksProxyClosesConnectionImmediately() throws Exception + { + String proxyHost = "localhost"; + int proxyPort = proxy.socket().getLocalPort(); + client.getProxyConfiguration().getProxies().add(new Socks4Proxy(proxyHost, proxyPort)); + + Request request = client.newRequest("localhost", proxyPort + 1); + FutureResponseListener listener = new FutureResponseListener(request); + request.send(listener); + + try (SocketChannel channel = proxy.accept()) + { + // Immediately close the connection. + channel.close(); + + ExecutionException x = assertThrows(ExecutionException.class, () -> listener.get(5, TimeUnit.SECONDS)); + assertThat(x.getCause(), instanceOf(IOException.class)); + } + } }