Skip to content

Commit

Permalink
okhttp: Add client transport proxy socket timeout (#9586)
Browse files Browse the repository at this point in the history
Not having a timeout when reading the response from a proxy server can
cause a hang if network connectivity is lost at the same time.
  • Loading branch information
temawi committed Oct 4, 2022
1 parent 6b80efc commit dd35ae5
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 1 deletion.
13 changes: 12 additions & 1 deletion okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java
Expand Up @@ -221,6 +221,9 @@ protected void handleNotInUse() {
@Nullable
final HttpConnectProxiedSocketAddress proxiedAddr;

@VisibleForTesting
int proxySocketTimeout = 30000;

// The following fields should only be used for test.
Runnable connectingCallback;
SettableFuture<Void> connectedFuture;
Expand Down Expand Up @@ -626,8 +629,8 @@ private void sendConnectionPrefaceAndSettings() {

private Socket createHttpProxySocket(InetSocketAddress address, InetSocketAddress proxyAddress,
String proxyUsername, String proxyPassword) throws StatusException {
Socket sock = null;
try {
Socket sock;
// The proxy address may not be resolved
if (proxyAddress.getAddress() != null) {
sock = socketFactory.createSocket(proxyAddress.getAddress(), proxyAddress.getPort());
Expand All @@ -636,6 +639,9 @@ private Socket createHttpProxySocket(InetSocketAddress address, InetSocketAddres
socketFactory.createSocket(proxyAddress.getHostName(), proxyAddress.getPort());
}
sock.setTcpNoDelay(true);
// A socket timeout is needed because lost network connectivity while reading from the proxy,
// can cause reading from the socket to hang.
sock.setSoTimeout(proxySocketTimeout);

Source source = Okio.source(sock);
BufferedSink sink = Okio.buffer(Okio.sink(sock));
Expand Down Expand Up @@ -682,8 +688,13 @@ private Socket createHttpProxySocket(InetSocketAddress address, InetSocketAddres
statusLine.code, statusLine.message, body.readUtf8());
throw Status.UNAVAILABLE.withDescription(message).asException();
}
// As the socket will be used for RPCs from here on, we want the socket timeout back to zero.
sock.setSoTimeout(0);
return sock;
} catch (IOException e) {
if (sock != null) {
GrpcUtil.closeQuietly(sock);
}
throw Status.UNAVAILABLE.withDescription("Failed trying to connect with proxy").withCause(e)
.asException();
}
Expand Down
31 changes: 31 additions & 0 deletions okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
Expand Up @@ -1877,6 +1877,37 @@ public void proxy_immediateServerClose() throws Exception {
verify(transportListener, timeout(TIME_OUT_MS)).transportTerminated();
}

@Test
public void proxy_serverHangs() throws Exception {
ServerSocket serverSocket = new ServerSocket(0);
InetSocketAddress targetAddress = InetSocketAddress.createUnresolved("theservice", 80);
clientTransport = new OkHttpClientTransport(
channelBuilder.buildTransportFactory(),
targetAddress,
"authority",
"userAgent",
EAG_ATTRS,
HttpConnectProxiedSocketAddress.newBuilder()
.setTargetAddress(targetAddress)
.setProxyAddress(new InetSocketAddress("localhost", serverSocket.getLocalPort()))
.build(),
tooManyPingsRunnable);
clientTransport.proxySocketTimeout = 10;
clientTransport.start(transportListener);

Socket sock = serverSocket.accept();
serverSocket.close();

BufferedReader reader = new BufferedReader(new InputStreamReader(sock.getInputStream(), UTF_8));
assertEquals("CONNECT theservice:80 HTTP/1.1", reader.readLine());
assertEquals("Host: theservice:80", reader.readLine());
while (!"".equals(reader.readLine())) {}

verify(transportListener, timeout(200)).transportShutdown(any(Status.class));
verify(transportListener, timeout(TIME_OUT_MS)).transportTerminated();
sock.close();
}

@Test
public void goAway_notUtf8() throws Exception {
initTransport();
Expand Down

0 comments on commit dd35ae5

Please sign in to comment.