Skip to content

Commit

Permalink
Add a workaround for HTTP/2 connection reuse
Browse files Browse the repository at this point in the history
Sometimes we're seeing HTTP/2 connections be reused for up to ~16
minutes despite being completely defunct -- i.e. we're receiving no
response from the remote end.

We believe that this is in part a result of Go bug

  golang/go#59690

in which failed connections are not correctly discarded under certain
circumstances.

This commit enables the http2 transport's `ReadIdleTimeout` function,
which will send an h2 "ping" frame on any connection that's been idle
longer than the value of `ReadIdleTimeout`. If it doesn't hear back in
`PingTimeout`, it will throw away the connection. This seems like a
reasonable thing to leave on in general.
  • Loading branch information
nickstenning committed Nov 27, 2023
1 parent e251d3c commit 01d10ad
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 1 deletion.
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ require (
go.opentelemetry.io/otel/sdk v1.19.0
go.opentelemetry.io/otel/trace v1.19.0
go.uber.org/zap v1.26.0
golang.org/x/net v0.17.0
golang.org/x/tools v0.14.0
gotest.tools/gotestsum v1.11.0
)
Expand Down Expand Up @@ -217,7 +218,6 @@ require (
golang.org/x/exp v0.0.0-20230510235704-dd950f8aeaea // indirect
golang.org/x/exp/typeparams v0.0.0-20230307190834-24139beb5833 // indirect
golang.org/x/mod v0.13.0 // indirect
golang.org/x/net v0.17.0 // indirect
golang.org/x/sync v0.4.0 // indirect
golang.org/x/sys v0.13.0 // indirect
golang.org/x/term v0.13.0 // indirect
Expand Down
28 changes: 28 additions & 0 deletions httpclient/httpclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
"go.opentelemetry.io/otel/propagation"
"golang.org/x/net/http2"
)

const ConnectTimeout = 5 * time.Second
Expand Down Expand Up @@ -72,6 +73,7 @@ func DefaultPooledTransport() *http.Transport {
ExpectContinueTimeout: 1 * time.Second,
MaxIdleConnsPerHost: runtime.GOMAXPROCS(0) + 1,
}
configureHTTP2(transport)
return transport
}

Expand All @@ -93,3 +95,29 @@ func DefaultPooledClient() *http.Client {
Transport: DefaultPooledRoundTripper(),
}
}

func configureHTTP2(t *http.Transport) {
h2t, err := http2.ConfigureTransports(t)
if err != nil {
// ConfigureTransports should only ever return an error if the transport
// passed in has already been configured for http2, which shouldn't be
// possible for us.
panic(err)
}

// Send a ping frame on any connection that's been idle for more than 10
// seconds.
//
// The default is to never do this. We set it primarily as a workaround for
//
// https://github.com/golang/go/issues/59690
//
// where a connection that goes AWOL will not be correctly terminated and
// removed from the connection pool under certain circumstances. Together
// `ReadIdleTimeout` and `PingTimeout` should ensure that we remove defunct
// connections in ~20 seconds.
h2t.ReadIdleTimeout = 10 * time.Second
// Give the other end 10 seconds to respond. If we don't hear back, we'll
// close the connection.
h2t.PingTimeout = 10 * time.Second
}

0 comments on commit 01d10ad

Please sign in to comment.