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

feat(transport): configure HTTP/2 ReadIdleTimeout #882

Merged
merged 6 commits into from Apr 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Expand Up @@ -8,6 +8,7 @@ require (
github.com/googleapis/gax-go/v2 v2.0.5
go.opencensus.io v0.23.0
golang.org/x/lint v0.0.0-20201208152925-83fdc39ff7b5
golang.org/x/net v0.0.0-20210316092652-d523dce5a7f4
golang.org/x/oauth2 v0.0.0-20210413134643-5e61552d6c78
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
golang.org/x/sys v0.0.0-20210412220455-f1c623a9e750
Expand Down
25 changes: 25 additions & 0 deletions transport/http/configure_http2_go116.go
@@ -0,0 +1,25 @@
// Copyright 2021 Google LLC.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

// +build go1.16

package http

import (
"net/http"
"time"

"golang.org/x/net/http2"
)

// configureHTTP2 configures the ReadIdleTimeout HTTP/2 option for the
// transport. This allows broken idle connections to be pruned more quickly,
// preventing the client from attempting to re-use connections that will no
// longer work.
func configureHTTP2(trans *http.Transport) {
http2Trans, err := http2.ConfigureTransports(trans)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but should we be using ConfigureTransport? Or is http2Trans magically connecting some pointers under the hood. I guess I don't understand how the setting on line 23 will do anything to trans, am I missing something?

Copy link
Contributor Author

@tritone tritone Feb 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked Damien the same question and he confirmed that this is how it's supposed to be done. Unfortunately ConfigureTransport does not actually give you access to the underlying surface. @neild can you comment on how this works under the hood?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http2.ConfigureTransport just calls http2.ConfigureTransports and discards the *Transport.

ConfigureTransports is a newer function added to allow modifying the http2.Transport configuration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neild Right, I do understand this provides some toggles to configure settings. What I was trying to ask is should this method, configureHTTP2, should be returning http2Trans as a RoundTripper? But it look like this transport is embedded into trans via RegisterProtocol now that I look a little closer. Would there be any benefit returning http2Trans though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified via local testing with http2debug=1 that a storage client created using this does, in fact, send a PING every 15s.

if err == nil {
http2Trans.ReadIdleTimeout = time.Second * 15
}
}
16 changes: 16 additions & 0 deletions transport/http/configure_http2_not_go116.go
@@ -0,0 +1,16 @@
// Copyright 2021 Google LLC.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

// +build !go1.16

package http

import (
"net/http"
)

// configureHTTP2 configures the ReadIdleTimeout HTTP/2 option for the
// transport. The interface to do this is only available in Go 1.16 and up, so
// this performs a no-op.
func configureHTTP2(trans *http.Transport) {}
4 changes: 4 additions & 0 deletions transport/http/dial.go
Expand Up @@ -175,6 +175,10 @@ func defaultBaseTransport(ctx context.Context, clientCertSource cert.Source) htt
}
}

// If possible, configure http2 transport in order to use ReadIdleTimeout
// setting. This can only be done in Go 1.16 and up.
configureHTTP2(trans)

return trans
}

Expand Down