drpcserver: bound the ServeOne TLS handshake with an optional timeout#83
drpcserver: bound the ServeOne TLS handshake with an optional timeout#83rafiss wants to merge 1 commit into
Conversation
48f4752 to
202df95
Compare
| handshakeTimeout := s.opts.TLSHandshakeTimeout | ||
| if handshakeTimeout > 0 { | ||
| _ = tlsConn.SetDeadline(time.Now().Add(handshakeTimeout)) | ||
| } | ||
| err := tlsConn.HandshakeContext(ctx) | ||
| if handshakeTimeout > 0 { | ||
| _ = tlsConn.SetDeadline(time.Time{}) | ||
| } |
There was a problem hiding this comment.
@rafiss Could we use context.WithTimeout and pass the child context to HandshakeContext instead of changing the connection deadline? This preserves any existing connection deadline and respects an earlier parent context deadline. Go's TLS implementation closes the underlying connection when the handshake context is canceled, so stalled I/O is still interrupted without resetting connection state afterward.
There was a problem hiding this comment.
Good call. I switched to context.WithTimeout(ctx, timeout), instead of setting/clearing the connection deadline. I had used the deadline approach since that was what was required in the first call-site where this got fixed (https://github.com/cockroachlabs/cockroach/pull/2482), since there was no context available there.
Claude agreed on all three points: since ServeOne already passes a cancelable context to HandshakeContext, the interrupt-on-cancel machinery is already in play, so this adds no extra complexity; it respects an earlier parent deadline/cancellation (e.g. server drain); and it touches no connection-deadline state, so the reset-after-handshake dance goes away entirely. Updated the test to cover both the timeout firing and an unset timeout still being interrupted by parent-context cancellation.
ServeOne performs the TLS handshake explicitly so it can populate the peer connection info before serving requests. That handshake was bounded only by the context passed to ServeOne, which typically carries no deadline, so a peer that completes the TCP connection but then stalls mid-handshake (never sending its ClientHello flight) would pin the serving goroutine and its file descriptor indefinitely. Add an Options.TLSHandshakeTimeout field. When positive, ServeOne derives a child context with that timeout from the context it was given and passes it to HandshakeContext; Go closes the underlying connection when the handshake context is done, so a stalled handshake is interrupted without touching the connection's own deadlines, and serving continues to use the original context. Deriving the timeout from the parent context also means an earlier parent deadline or cancellation still applies. A zero value preserves the existing behavior (bounded only by the parent context), keeping the change backward compatible. Informs: cockroachdb/cockroach#144754 Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
202df95 to
59cc501
Compare
2 participants
ServeOneperforms the TLS handshake explicitly (so it can populate peer connection info before serving requests), but that handshake had no deadline. A peer that completes the TCP connection and then stalls mid-handshake — never sending its ClientHello flight — would pin the serving goroutine and its file descriptor indefinitely.This adds an
Options.TLSHandshakeTimeoutfield. When positive,ServeOnesets a deadline on the connection for the duration of the handshake and clears it once the handshake returns, so it does not affect subsequent reads and writes on the established connection. A zero value preserves the existing behavior (no deadline), keeping the change backward compatible.Tested with a stalled
net.Pipepeer (handshake times out promptly) and a zero-timeout case (no deadline is set).Informs: cockroachdb/cockroach#144754