feat(transport): configure HTTP/2 ReadIdleTimeout #882
Conversation
@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
bot
added
the
cla: yes
|
Looks like we also need to add a Go 1.16 build to this library? |
|
@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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
http2.ConfigureTransport just calls http2.ConfigureTransports and discards the *Transport.
ConfigureTransports is a newer function added to allow modifying the http2.Transport configuration.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
I verified via local testing with http2debug=1 that a storage client created using this does, in fact, send a PING every 15s.
tritone
force-pushed
the
http2-readidletimeout
branch
from
80d8ec1
to
fcf5b81
Compare
@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.