Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
neild opened this issue Oct 6, 2021 · 1 comment
Closed

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

neild opened this issue Oct 6, 2021 · 1 comment
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@neild
Copy link
Contributor

neild commented Oct 6, 2021

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.

@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 6, 2021
@mknyszek mknyszek added this to the Unreleased milestone Oct 6, 2021
@gopherbot
Copy link

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

dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
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 <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Oct 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants