x/net/http2: pings ineffective when connection writes block

[neild]

There are two http2.Transport parameters that control detection of stalled connections: ReadIdleTimeout and PingTimeout. If no data is read from a connection after ReadIdleTimeout, a PING frame is sent. If no response is received to the ping within PingTimeout, the connection is closed.

However, the ping timeout starts after the ping is sent. If a connection is write-blocked so that the PING frame cannot be written, the PingTimeout timer never starts and the connection is never closed.

Sample test case:

  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()
                          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.Second,
                  ReadIdleTimeout: 1 * time.Second,
          }
          defer tr.CloseIdleConnections()
          c := &http.Client{Transport: tr}

          _, err := c.Get(st.ts.URL)
          if err == nil {
                  t.Fatalf("Get = nil, want err")
          }
  }

(Another issue I just noticed while writing this test case is that if a connection becomes write-blocked while sending the initial handshake frames, we never even reach the point of sending a PING. This is unlikely in practice, because the initial handshake will almost certainly fit in the socket's send buffer, but still.)

The PingTimeout timer should probably start when we start trying to write the PING frame.

In addition, I think we might want an additional knob on http2.Transport to catch write-blocked connections in general:

// WriteTimeout is the timeout after which the connection will be closed
// if no data can be written to it. The timeout begins when data is written
// to the connection, and is extended if any bytes can be written.
// If zero, no write timeout is set.
WriteTimeout time.Duration

In particular, a write timeout would detect stalled connections in cases where the user doesn't want to constantly ping the connection for responsiveness.

[gopherbot]

Change https://golang.org/cl/354389 mentions this issue: http2: detect write-blocked PING frames