From 353acfef459fa87f96ba4f8ad3f0950bce74b860 Mon Sep 17 00:00:00 2001 From: Bram Date: Fri, 24 Jul 2026 15:41:33 -0400 Subject: [PATCH] Fix flaky TLS test by waiting for dial outcome before closing connection (#572) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix flaky TLS test by waiting for dial outcome before closing connection PRs #564 and #567 introduced errSignalingConn to surface TLS errors (especially TLS 1.3 post-handshake alerts) to BlockingDial callers. This caused a flaky test failure (reported in: https://github.com/fullstorydev/grpcurl/pull/564#issuecomment-4916344087): ``` TestBrokenTLS_RequireClientCertButNonePresented: expecting a TLS certificate error, got: read tcp ...: use of closed network connection ``` The race works as follows: 1. With TLS 1.3, the server rejects the connection *after* the handshake completes (e.g. missing client cert) by sending a TLS alert, then closing the connection. 2. Simultaneously, the client's Write fails (broken pipe) because the server already closed its side. 3. grpc-go's `NewHTTP2Client` returns the Write error and calls `t.Close()` to tear down the transport, which calls `errSignalingConn.Close()` -> `c.Conn.Close()`, closing the connection. 4. The reader goroutine's pending Read (which should have returned the TLS alert (e.g. "certificate required")) instead returns `use of closed network connection` because the connection was closed underneath it. 5. `writeResult` surfaces the "closed network connection" error to the caller instead of the meaningful TLS alert. The fix: delay Close on the errSignalingConn until the outcome of the dial is known, with a 50ms timeout. This gives the reader a chance to surface the TLS alert. Demonstrating this by stressing the test: **Without the fix** ``` alasia :: github/fullstorydev/grpcurl ‹master› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS' /var/folders/7l/0tdjxwg912j9gqh446yk3ll40000gn/T/go-stress-20260724T113423-1710148011 --- FAIL: TestBrokenTLS_RequireClientCertButNonePresented (0.00s) tls_settings_test.go:332: expecting a TLS certificate error, got: read tcp 127.0.0.1:52850->127.0.0.1:52848: use of closed network connection FAIL ERROR: exit status 1 [...] 5s: 1580 runs so far, 5 failures (0.32%), 13 active ``` **With the fix** ``` alasia :: github/fullstorydev/grpcurl ‹surface-post-dial-conn-errors› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS' 5s: 1594 runs so far, 0 failures, 16 active 10s: 3386 runs so far, 0 failures, 16 active 15s: 4999 runs so far, 0 failures, 1 active 15s: 5000 runs total, 0 failures ``` * Implement review comment --- grpcurl.go | 47 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/grpcurl.go b/grpcurl.go index c5625b1..c970d3d 100644 --- a/grpcurl.go +++ b/grpcurl.go @@ -20,6 +20,8 @@ import ( "slices" "sort" "strings" + "sync" + "time" "github.com/golang/protobuf/proto" //lint:ignore SA1019 we have to import these because some of their types appear in exported API "github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above @@ -629,7 +631,16 @@ func BlockingDial(ctx context.Context, network, address string, creds credential // custom dialer that can provide that info. That means we manage the TLS handshake. result := make(chan interface{}, 1) + // dialCompleted is closed once the outcome of the dial (ready connection + // or error) is known, so that connection teardown no longer needs to wait + // for a pending TLS alert to be read. + dialCompleted := make(chan struct{}) + var dialCompletedOnce sync.Once + completeDial := func() { dialCompletedOnce.Do(func() { close(dialCompleted) }) } + defer completeDial() + writeResult := func(res interface{}) { + completeDial() // non-blocking write: we only need the first result select { case result <- res: @@ -642,6 +653,7 @@ func BlockingDial(ctx context.Context, network, address string, creds credential creds = &errSignalingCreds{ TransportCredentials: creds, writeResult: writeResult, + dialCompleted: dialCompleted, } switch network { @@ -724,7 +736,8 @@ func BlockingDial(ctx context.Context, network, address string, creds credential // it will use the writeResult function to notify on error. type errSignalingCreds struct { credentials.TransportCredentials - writeResult func(res interface{}) + writeResult func(res interface{}) + dialCompleted <-chan struct{} } func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { @@ -736,14 +749,23 @@ func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, ra // Wrap the connection to capture post-handshake errors, e.g.: // - TLS 1.3 client cert rejection (server sends alert after handshake) // - Plaintext client to TLS server (server closes conn immediately) - return &errSignalingConn{Conn: conn, writeResult: c.writeResult}, auth, nil + return &errSignalingConn{Conn: conn, writeResult: c.writeResult, dialCompleted: c.dialCompleted}, auth, nil } -// errSignalingConn wraps a net.Conn to capture the first read error and -// report it via writeResult. +// maxTLSAlertWait is an upper bound on how long a failing dial's connection +// teardown waits for a pending TLS alert to be read. It is a heuristic: long +// enough for the reader goroutine to be scheduled even on a loaded machine, +// short enough to not noticeably delay a failing dial. +const maxTLSAlertWait = 50 * time.Millisecond + +// errSignalingConn wraps a net.Conn to capture read errors and report +// them via writeResult. Close is delayed until the dial has completed to +// give the reader a chance to read a pending TLS alert before the +// connection is closed. type errSignalingConn struct { net.Conn - writeResult func(res interface{}) + writeResult func(res interface{}) + dialCompleted <-chan struct{} } func (c *errSignalingConn) Read(b []byte) (int, error) { @@ -754,6 +776,21 @@ func (c *errSignalingConn) Read(b []byte) (int, error) { return n, err } +func (c *errSignalingConn) Close() error { + // With TLS 1.3, the server may reject the connection *after* the + // handshake completes (e.g. a missing client cert), by sending an alert. + // If the transport closes the connection before that alert is read, the + // caller sees a less useful error (e.g. "use of closed network + // connection") instead of the TLS alert. So while the outcome of the + // dial is still undecided, give the reader a brief window to read a + // pending alert before closing the connection. + select { + case <-c.dialCompleted: + case <-time.After(maxTLSAlertWait): + } + return c.Conn.Close() +} + // UsesXDS forwards the optional UsesXDS marker of the wrapped credentials. The // xDS credentials returned for "xds://" targets implement this method, and // grpc-go's cds balancer relies on a type assertion for it to decide whether to