Skip to content

Add connectTimeoutMs option to bound connection establishment#1748

Open
jackyzha0 wants to merge 1 commit into
connectrpc:mainfrom
jackyzha0:connect-establishment-timeout
Open

Add connectTimeoutMs option to bound connection establishment#1748
jackyzha0 wants to merge 1 commit into
connectrpc:mainfrom
jackyzha0:connect-establishment-timeout

Conversation

@jackyzha0

Copy link
Copy Markdown

Background

Connection establishment in Http2SessionManager has no timeout. The operating system's TCP timeouts bound an attempt that receives no answer at all, but nothing bounds an attempt that dies after the TCP handshake - for example when the network path is lost while the TLS handshake is in flight. At that point there is nothing left for TCP to retransmit, TLS clients in Node.js have no handshake timeout, and http2.connect() adds none, so the attempt stays pending indefinitely. The manager pins in the "connecting" state, and every request queued on it fails on its own deadline (or hangs, if it has none).

This is easy to demonstrate: dial a TCP server that accepts connections but never completes the TLS handshake. The manager reports "connecting" forever.

We've been chasing this family of hangs in production (see #1746 and #1747 for the session-level and call-level variants); this is the last phase of a connection's life without a bounded clock. gRPC core bounds connection establishment with a connect deadline; connect-node currently has no equivalent.

Summary

Add an opt-in connectTimeoutMs option to Http2SessionOptions. When the connection is not ready within the given time, it is destroyed, and pending requests reject with Code.Unavailable - the same code that ETIMEDOUT and ECONNREFUSED map to, so retry behavior is consistent with other connection failures. The next request opens a fresh connection.

By default no timeout is applied, so behavior is unchanged for existing users. Since the transport options intersect Http2SessionOptions, the option is available on createGrpcTransport() et al with a one-line change in validateNodeTransportOptions.

The new test uses a TCP server that accepts and stays silent: with connectTimeoutMs, the first request rejects within the timeout, the manager transitions to "error" instead of pinning, and a subsequent request dials again with the same bound.

Connection establishment in Http2SessionManager has no timeout. The
operating system's TCP timeouts bound an attempt that receives no
answer at all, but nothing bounds an attempt that dies after the TCP
handshake - for example when the network path is lost while the TLS
handshake is in flight. There is nothing left for TCP to retransmit,
TLS clients in Node.js have no handshake timeout, and http2.connect()
adds none, so the attempt stays pending indefinitely. The manager pins
in the "connecting" state, and every request queued on it fails.

Add an opt-in connectTimeoutMs option. When the connection is not
ready within the given time, it is destroyed, and pending requests
reject with Code.Unavailable - the same code that ETIMEDOUT and
ECONNREFUSED map to, so retry behavior is consistent with other
connection failures. The next request opens a fresh connection.

By default, no timeout is applied, preserving current behavior. The
option is also threaded through the transport options, which intersect
Http2SessionOptions.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Jacky Zhao <j.zhao2k19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant