transport: Invoke net.Conn.SetWriteDeadline in http2_client.Close

[jgold2-stripe]

Fixes: #8425

This PR adds a call to net.Conn.SetWriteDeadline, as discussed in #8425 (comment). Additionally, it updates the previous call to SetReadDeadline to log any non-nil error value (this doesn't affect behavior but proved helpful in some earlier debugging).

RELEASE NOTES:

• client: Set a read deadline when closing a transport to prevent it from blocking indefinitely on a broken connection.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.94%. Comparing base (9186ebd) to head (b9e53e2).
⚠️ Report is 41 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8534      +/-   ##
==========================================
- Coverage   82.47%   81.94%   -0.54%     
==========================================
  Files         413      412       -1     
  Lines       40513    40468      -45     
==========================================
- Hits        33415    33162     -253     
- Misses       5743     5920     +177     
- Partials     1355     1386      +31     

Files with missing lines	Coverage Δ
internal/transport/http2_client.go	92.36% <100.00%> (+0.24%)	⬆️

... and 253 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[arjan-bal]

Can you please add a test to verify the fix and catch regressions? You can refer to the test for the write deadline:

grpc-go/internal/transport/transport_test.go

Lines 2946 to 3009 in b0bc6dc

	// hangingConn is a net.Conn wrapper for testing, simulating hanging connections
	// after a GOAWAY frame is sent, of which Write operations pause until explicitly
	// signaled or a timeout occurs.
	type hangingConn struct {
	net.Conn
	hangConn chan struct{}
	startHanging *atomic.Bool
	}
	func (hc *hangingConn) Write(b []byte) (n int, err error) {
	n, err = hc.Conn.Write(b)
	if hc.startHanging.Load() {
	<-hc.hangConn
	}
	return n, err
	}
	// Tests the scenario where a client transport is closed and writing of the
	// GOAWAY frame as part of the close does not complete because of a network
	// hang. The test verifies that the client transport is closed without waiting
	// for too long.
	func (s) TestClientCloseReturnsEarlyWhenGoAwayWriteHangs(t *testing.T) {
	// Override timer for writing GOAWAY to 0 so that the connection write
	// always times out. It is equivalent of real network hang when conn
	// write for goaway doesn't finish in specified deadline
	origGoAwayLoopyTimeout := goAwayLoopyWriterTimeout
	goAwayLoopyWriterTimeout = time.Millisecond
	defer func() {
	goAwayLoopyWriterTimeout = origGoAwayLoopyTimeout
	}()
	// Create the server set up.
	connectCtx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
	defer cancel()
	server := setUpServerOnly(t, 0, &ServerConfig{}, normal)
	defer server.stop()
	addr := resolver.Address{Addr: "localhost:" + server.port}
	isGreetingDone := &atomic.Bool{}
	hangConn := make(chan struct{})
	defer close(hangConn)
	dialer := func(_ context.Context, addr string) (net.Conn, error) {
	conn, err := net.Dial("tcp", addr)
	if err != nil {
	return nil, err
	}
	return &hangingConn{Conn: conn, hangConn: hangConn, startHanging: isGreetingDone}, nil
	}
	copts := ConnectOptions{Dialer: dialer}
	copts.ChannelzParent = channelzSubChannel(t)
	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
	defer cancel()
	// Create client transport with custom dialer
	ct, connErr := NewHTTP2Client(connectCtx, ctx, addr, copts, func(GoAwayReason) {})
	if connErr != nil {
	t.Fatalf("failed to create transport: %v", connErr)
	}
	if _, err := ct.NewStream(ctx, &CallHdr{}); err != nil {
	t.Fatalf("Failed to open stream: %v", err)
	}
	isGreetingDone.Store(true)
	ct.Close(errors.New("manually closed by client"))
	}

[jgold2-stripe]

PTAL @arjan-bal

[arjan-bal]

These logs are too chatty. They're appearing almost every time a connection is closed, e.g:

tlogger.go:126: WARNING http2_client.go:993 [transport] Failed to set write deadline when closing connection: set tcp 127.0.0.1:59286: use of closed network connection  (t=+590.186033ms)

I don't think we should log here because we are calling conn.Close in another goroutine and expecting these calls to fail.

[jgold2-stripe]

Dropped the logging.

[arjan-bal]

LGTM. Adding a second reviewer.

[dfawley]

Thank you for the change. A couple small things to potentially change.

[dfawley]

I think it would be better/simpler to just use SetDeadline with the same 1 second deadline, if possible.

[dfawley]

If we were to set read/write deadlines during handshaking (we currently only block on the context here, but our server does set deadlines here), then this test would be invalid. I think we should either confirm that the values are still false before calling Close, or clear them unconditionally then.

[github-actions]

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.