Skip to content

Commit

Permalink
[internal-branch.go1.16-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#49076

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]>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357092
Reviewed-by: Dmitri Shuralyov <[email protected]>
TryBot-Result: Go Bot <[email protected]>
  • Loading branch information
neild authored and dmitshur committed Oct 29, 2021
1 parent 20ed279 commit 198b78c
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 10 deletions.
25 changes: 15 additions & 10 deletions http2/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -2594,19 +2594,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 http2/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 198b78c

Please sign in to comment.