Skip to content

Commit a74c398

Browse files
okhttp: Set socket read timeout during TLS handshake (#13081)
In `OkHttpClientTransport`, direct TLS connection setup calls `OkHttpTlsUpgrader.upgrade(...)` without configuring a socket read timeout (`setSoTimeout`). When a middlebox or unresponsive server completes the TCP three-way handshake and acknowledges the TLS `ClientHello` without sending a `ServerHello` or `RST`, `sslSocket.startHandshake()` blocks indefinitely in socket read. Because `this.socket` and `asyncSink` are only assigned after `OkHttpTlsUpgrader.upgrade(...)` returns, channel shutdown cannot close the underlying socket either, leaving the subchannel permanently stuck in `CONNECTING`. Configure `sock.setSoTimeout(proxySocketTimeout)` before `OkHttpTlsUpgrader.upgrade(...)` and reset it to `0` once the TLS upgrade completes, matching the timeout handling in `createHttpProxySocket`. Also close `sock` via `GrpcUtil.closeQuietly(sock)` in `catch (Exception e)` so the socket is not leaked when handshake setup fails before `asyncSink.becomeConnected(...)` takes ownership.
1 parent 1084e97 commit a74c398

2 files changed

Lines changed: 36 additions & 0 deletions

File tree

‎okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -739,9 +739,13 @@ public void close() {
739739
}
740740
}
741741
if (sslSocketFactory != null) {
742+
sock.setSoTimeout(proxySocketTimeout);
742743
SSLSocket sslSocket = OkHttpTlsUpgrader.upgrade(
743744
sslSocketFactory, hostnameVerifier, sock, getOverridenHost(), getOverridenPort(),
744745
connectionSpec);
746+
// As the socket will be used for RPCs from here on, we want the socket
747+
// timeout back to zero.
748+
sock.setSoTimeout(0);
745749
sslSession = sslSocket.getSession();
746750
sock = sslSocket;
747751
}
@@ -761,6 +765,7 @@ sslSocketFactory, hostnameVerifier, sock, getOverridenHost(), getOverridenPort()
761765
startGoAway(0, ErrorCode.INTERNAL_ERROR, e.getStatus());
762766
return;
763767
} catch (Exception e) {
768+
GrpcUtil.closeQuietly(sock);
764769
onException(e);
765770
return;
766771
} finally {

‎okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import com.google.common.base.Preconditions;
5050
import com.google.common.base.Stopwatch;
5151
import com.google.common.base.Supplier;
52+
import com.google.common.base.Throwables;
5253
import com.google.common.base.Ticker;
5354
import com.google.common.collect.ImmutableList;
5455
import com.google.common.util.concurrent.Futures;
@@ -101,6 +102,7 @@
101102
import java.net.ServerSocket;
102103
import java.net.Socket;
103104
import java.net.SocketAddress;
105+
import java.net.SocketTimeoutException;
104106
import java.util.ArrayDeque;
105107
import java.util.ArrayList;
106108
import java.util.Arrays;
@@ -2099,6 +2101,35 @@ public void proxy_serverHangs() throws Exception {
20992101
sock.close();
21002102
}
21012103

2104+
@Test
2105+
public void tls_serverHangs() throws Exception {
2106+
ServerSocket serverSocket = new ServerSocket(0);
2107+
clientTransport =
2108+
new OkHttpClientTransport(
2109+
channelBuilder.useTransportSecurity().buildTransportFactory(),
2110+
new InetSocketAddress("localhost", serverSocket.getLocalPort()),
2111+
"authority",
2112+
"userAgent",
2113+
EAG_ATTRS,
2114+
NO_PROXY,
2115+
tooManyPingsRunnable,
2116+
null);
2117+
clientTransport.proxySocketTimeout = 10;
2118+
clientTransport.start(transportListener);
2119+
2120+
Socket sock = serverSocket.accept();
2121+
serverSocket.close();
2122+
2123+
ArgumentCaptor<Status> statusCaptor = ArgumentCaptor.forClass(Status.class);
2124+
verify(transportListener, timeout(200))
2125+
.transportShutdown(statusCaptor.capture(), any(DisconnectError.class));
2126+
verify(transportListener, timeout(TIME_OUT_MS)).transportTerminated();
2127+
assertThat(statusCaptor.getValue().getCode()).isEqualTo(Status.Code.UNAVAILABLE);
2128+
assertThat(Throwables.getRootCause(statusCaptor.getValue().getCause()))
2129+
.isInstanceOf(SocketTimeoutException.class);
2130+
sock.close();
2131+
}
2132+
21022133
@Test
21032134
public void goAway_notUtf8() throws Exception {
21042135
initTransport();

0 commit comments

Comments
 (0)