http2: detect write-blocked PING frames

We start the PingTimeout timer before writing a PING frame.
However, writing the frame can block indefinitely (either
acquiring cc.wmu or writing to the conn), and the timer is
not checked until after the frame is written.

Move PING writes into a separate goroutine, so we can detect
write-blocked connections.

Fixes golang/go#48810.

Change-Id: Ifd67fa23799e750d02754e1fe5d32719f60faed4
Reviewed-on: https://go-review.googlesource.com/c/net/+/354389
Trust: Damien Neil <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: neild

http2/transport.go

@@ -2637,19 +2637,24 @@ func (cc *ClientConn) Ping(ctx context.Context) error {
 		}
 		cc.mu.Unlock()
 	}
-	cc.wmu.Lock()
-	if err := cc.fr.WritePing(false, p); err != nil {
-		cc.wmu.Unlock()
-		return err
-	}
-	if err := cc.bw.Flush(); err != nil {
-		cc.wmu.Unlock()
-		return err
-	}
-	cc.wmu.Unlock()
+	errc := make(chan error, 1)
+	go func() {
+		cc.wmu.Lock()
+		defer cc.wmu.Unlock()
+		if err := cc.fr.WritePing(false, p); err != nil {
+			errc <- err
+			return
+		}
+		if err := cc.bw.Flush(); err != nil {
+			errc <- err
+			return
+		}
+	}()
 	select {
 	case <-c:
 		return nil
+	case err := <-errc:
+		return err
 	case <-ctx.Done():
 		return ctx.Err()
 	case <-cc.readerDone:

http2/transport_test.go

@@ -3494,6 +3494,36 @@ func TestTransportCloseAfterLostPing(t *testing.T) {
 	ct.run()
 }
 
+func TestTransportPingWriteBlocks(t *testing.T) {
+	st := newServerTester(t,
+		func(w http.ResponseWriter, r *http.Request) {},
+		optOnlyServer,
+	)
+	defer st.Close()
+	tr := &Transport{
+		TLSClientConfig: tlsConfigInsecure,
+		DialTLS: func(network, addr string, cfg *tls.Config) (net.Conn, error) {
+			s, c := net.Pipe() // unbuffered, unlike a TCP conn
+			go func() {
+				// Read initial handshake frames.
+				// Without this, we block indefinitely in newClientConn,
+				// and never get to the point of sending a PING.
+				var buf [1024]byte
+				s.Read(buf[:])
+			}()
+			return c, nil
+		},
+		PingTimeout:     1 * time.Millisecond,
+		ReadIdleTimeout: 1 * time.Millisecond,
+	}
+	defer tr.CloseIdleConnections()
+	c := &http.Client{Transport: tr}
+	_, err := c.Get(st.ts.URL)
+	if err == nil {
+		t.Fatalf("Get = nil, want error")
+	}
+}
+
 func TestTransportPingWhenReading(t *testing.T) {
 	testCases := []struct {
 		name              string