Skip to content

Commit

Permalink
[internal-branch.go1.17-vendor] http2: detect write-blocked PING frames
Browse files Browse the repository at this point in the history
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.

Updates golang/go#49077

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>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357687
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
  • Loading branch information
neild authored and dmitshur committed Oct 29, 2021
1 parent ef3b794 commit 2187e36
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 10 deletions.
25 changes: 15 additions & 10 deletions transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -2590,19 +2590,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:
Expand Down
30 changes: 30 additions & 0 deletions transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 2187e36

Please sign in to comment.