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

Conversation

tritone
Copy link
Contributor

@tritone tritone commented Feb 19, 2021

@neild recommended this setting so that the transport will
do better at pruning dead HTTP/2 connections from the pool.

I ran storage integration tests with this change and verified that they work. Need to confirm that this will work for other HTTP-based clients.

@neild recommended this setting so that the transport will
do better at pruning dead HTTP/2 connections from the pool.

WIP; need to:
* Put under a build tag
* Confirm this will work for other HTTP clients
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 19, 2021
@tritone tritone marked this pull request as ready for review February 19, 2021 19:32
@tritone tritone requested review from yoshi-approver and a team as code owners February 19, 2021 19:32
@tritone
Copy link
Contributor Author

tritone commented Feb 19, 2021

Looks like we also need to add a Go 1.16 build to this library?

@codyoss
Copy link
Member

codyoss commented Feb 19, 2021

@tritone yeah you are beating me to the punch! I have the image created but I still need to create the jobs. Will try to get this done on Monday.

// 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.

@tritone tritone merged commit c2ff762 into googleapis:master Apr 16, 2021
@tritone tritone deleted the http2-readidletimeout branch April 16, 2021 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants