Skip to content

Commit

Permalink
Fixes #8584 - HttpRequest.send() never returns
Browse files Browse the repository at this point in the history 
Fixed handling of the idle timeout in case the SOCKS proxy does not reply to the SOCKS bytes.

Signed-off-by: Simone Bordet <[email protected]>
  • Loading branch information
@sbordet
sbordet committed Sep 30, 2022
1 parent 3c35b2d commit 88aca4f
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 11 deletions.
12 changes: 11 additions & 1 deletion jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);

This comment has been minimized.

Sign in to view

Copy link
@cowwoc

cowwoc Oct 24, 2022

Contributor

Looking at this again, why are we only closing the endpoint on failure? Should we also do it on success, or on a close for any reason?

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Oct 24, 2022

Contributor

I (personally) would not expect closure on success.
gotta follow those connection management rules on HTTP.

This comment has been minimized.

Sign in to view

Copy link
@cowwoc

cowwoc Oct 25, 2022 •
edited
Loading

Contributor

@joakime I think what you said makes sense, but as a follow up: what happens when we close the (proxy?) connection on failure? Will subsequent requests using the same proxy open a new connection? Or will the connection remain "busted" forever? :)

Is the purpose of closing the connection to force the client to run more checks the next time that the proxy is used (e.g. make sure that the proxy can still be reached)?

This comment has been minimized.

Sign in to view

Copy link
@sbordet

sbordet Oct 25, 2022

Author Contributor

The reason to close is to not leak file descriptors.

From the client point of view, it is opening a connection to the server, asynchronously, so a client callback cc will be completed when there is a connection to the server, either with a success or a failure.

However, because of the proxy, the client will instead open a connection to the proxy, which then opens a connection to the server.
However, opening a connection to the proxy is not just about TCP opening. We need to TCP open, send the SOCKS4 request, and wait for a successful SOCKS4 response, asynchronously.

The failed(Throwable) method above is called when the SOCKS4 request cannot be written.
In this case we TCP close the connection to the proxy (to avoid leaking file descriptors), and then we fail the client callback cc (code below).
From the client point of view, there was a failure trying to connect to the server, exactly like, without a proxy, it would fail to connect to a server that is down.

In case of success, you want to keep the connection open, because that is the "tunnel" that allows the client to talk to the server through the proxy.

This comment has been minimized.

Sign in to view

Copy link
@cowwoc

cowwoc Oct 25, 2022

Contributor

Got it. So if I understand correctly, the client will reconnect to the proxy on-demand if/when subsequent requests come in?

This comment has been minimized.

Sign in to view

Copy link
@sbordet

sbordet Oct 25, 2022

Author Contributor

Correct.

This comment has been minimized.

Sign in to view

Copy link
@cowwoc

cowwoc Oct 25, 2022

Contributor

Okay, LGTM. Thank you.
@SuppressWarnings("unchecked")
Promise<Connection> promise = (Promise<Connection>)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()
{
Expand Down
93 changes: 83 additions & 10 deletions jetty-client/src/test/java/org/eclipse/jetty/client/Socks4ProxyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
}
}
}

0 comments on commit 88aca4f

Please sign in to comment.