transport: Invoke `net.Conn.SetWriteDeadline` in `http2_client.Close` (#8534)

Fixes: #8425
This PR adds a call to `net.Conn.SetWriteDeadline`, as discussed in
https://github.com/grpc/grpc-go/issues/8425#issuecomment-3057938248.
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.

diff --git a/internal/transport/http2_client.go b/internal/transport/http2_client.go
index 7cb238794fb530dedfedfa2541e9a1e3d0a32336..e06771c3397f86447516363823c462eb1daa9119 100644 (file)
--- a/internal/transport/http2_client.go
+++ b/internal/transport/http2_client.go
@@ -1002,6 +1002,9 @@ func (t *http2Client) closeStream(s *ClientStream, err error, rst bool, rstCode
 // accessed anymore.
 func (t *http2Client) Close(err error) {
        t.conn.SetWriteDeadline(time.Now().Add(time.Second * 10))
+       // For background on the deadline value chosen here, see
+       // https://github.com/grpc/grpc-go/issues/8425#issuecomment-3057938248 .
+       t.conn.SetReadDeadline(time.Now().Add(time.Second))
        t.mu.Lock()
        // Make sure we only close once.
        if t.state == closing {

diff --git a/internal/transport/transport_test.go b/internal/transport/transport_test.go
index d0c8a88d6abfd4dce56d1efefbc2175f656768ff..0b0396b0ab37bc421a534aee91aa6537838fce76 100644 (file)
--- a/internal/transport/transport_test.go
+++ b/internal/transport/transport_test.go
@@ -3008,6 +3008,64 @@ func (s) TestClientCloseReturnsEarlyWhenGoAwayWriteHangs(t *testing.T) {
        ct.Close(errors.New("manually closed by client"))
 }
 
+// deadlineTestConn is a net.Conn wrapper used to assert that deadlines are set
+// during http2Client.Close().
+type deadlineTestConn struct {
+       net.Conn
+       // We use atomic.Bool here since there may be more than one call to
+       // http2Client.Close -- which sets these deadlines -- and not all of them
+       // from the same goroutine as our test. In fact we only care about the first
+       // such invocation, which *does* come from the main goroutine of our test,
+       // but the race detector can't know that and complains (understandably)
+       // about writes from those successive calls when these variables are not
+       // atomic.Bool.
+       //
+       // For more detailed background, see
+       // https://github.com/grpc/grpc-go/pull/8534#discussion_r2297717445 .
+       observedReadDeadline  atomic.Bool
+       observedWriteDeadline atomic.Bool
+}
+
+func (c *deadlineTestConn) SetReadDeadline(t time.Time) error {
+       c.observedReadDeadline.Store(true)
+       return c.Conn.SetReadDeadline(t)
+}
+
+func (c *deadlineTestConn) SetWriteDeadline(t time.Time) error {
+       c.observedWriteDeadline.Store(true)
+       return c.Conn.SetWriteDeadline(t)
+}
+
+// Tests that connection read and write deadlines are set as expected during
+// Close().
+func (s) TestCloseSetsConnectionDeadlines(t *testing.T) {
+       dialer := func(_ context.Context, addr string) (net.Conn, error) {
+               conn, err := net.Dial("tcp", addr)
+               if err != nil {
+                       return nil, err
+               }
+               return &deadlineTestConn{Conn: conn}, nil
+       }
+       co := ConnectOptions{
+               Dialer: dialer,
+       }
+       server, client, cancel := setUpWithOptions(t, 0, &ServerConfig{}, normal, co)
+       defer cancel()
+       defer server.stop()
+       dConn := client.conn.(*deadlineTestConn)
+       // Set both to false before invoking Close() in case some other code set a
+       // deadline above.
+       dConn.observedReadDeadline.Store(false)
+       dConn.observedWriteDeadline.Store(false)
+       client.Close(fmt.Errorf("closed manually by test"))
+       if !dConn.observedReadDeadline.Load() {
+               t.Errorf("Connection read deadline was never set")
+       }
+       if !dConn.observedWriteDeadline.Load() {
+               t.Errorf("Connection write deadline was never set")
+       }
+}
+
 // TestReadHeaderMultipleBuffers tests the stream when the gRPC headers are
 // split across multiple buffers. It verifies that the reporting of the
 // number of bytes read for flow control is correct.