-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
x/net/http2: add IdleConnTimeout, DisableKeepAlives, ExpectContinueTimeout, and ResponseHeaderTimeout to http2.Transport #57893
Comments
Exposes an IdleConnTimeout on http2.Transport directly, rather than rely on configuring it through the underlying http1 transport. Fixes golang/go#57893
Change https://go.dev/cl/461641 mentions this issue: |
There are a few other knobs that the
We should have a consistent policy here; either we should add all those fields to |
I think we should go with adding them, but maintaining fallback behavior for existing and new fields. Reading through the current code my reckoning is that the current behavior is so the result of I could see an argument for making the configuration transfer static by defining these fields on A compromise solution that preserves behavior could be
though honestly that just seems more complex than having consistent fallback behavior for the fields. |
@neild any thoughts on this? |
I support this. I always have to do stuff like: // Create the HTTP/2 Transport using a net/http.Transport
// (which only does HTTP/1) because it's the only way to
// configure certain properties on the http2.Transport. But we
// never actually use the net/http.Transport for any HTTP/1
// requests.
h2Transport, err := http2.ConfigureTransports(&http.Transport{
IdleConnTimeout: time.Minute,
})
if err != nil {
return nil, err
}
np.h2t = h2Transport
np.Client = &http.Client{Transport: np} |
@bradfitz and I talked this over, and we support making it so that an That would mean adding four new fields to Keep alives will be disabled if For timeouts, we'll prefer the value on the HTTP/2 transport if non-zero, falling back to the HTTP/1 transport's value. |
Sounds good!
For Once I get another spare night this week I can come back through and wire through the other fields. |
This proposal has been added to the active column of the proposals project |
Have all concerns with this proposal been addressed? |
No concerns here. |
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. 🎉 |
Exposes an IdleConnTimeout on http2.Transport directly, rather than rely on configuring it through the underlying http1 transport. Fixes golang/go#57893
Exposes an IdleConnTimeout on http2.Transport directly, rather than rely on configuring it through the underlying http1 transport. For golang/go#57893
Change https://go.dev/cl/497195 mentions this issue: |
Would it be possible to consider exposing |
Exposes an IdleConnTimeout on http2.Transport directly, rather than rely on configuring it through the underlying http1 transport. For golang/go#57893 Change-Id: Ibe506da39e314aebec1cd6df64937982182a37ca GitHub-Last-Rev: cc8f171 GitHub-Pull-Request: #173 Reviewed-on: https://go-review.googlesource.com/c/net/+/497195 Reviewed-by: Damien Neil <dneil@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
If I understand correctly, the accepted proposal is to add four new fields to http2.Transport as described in #57893 (comment). I updated the title accordingly, otherwise it's hard to understand why this is open after CL 497195 is submitted. Please correct if needed. |
While it is possible to configure the IdleConnTimeout using the
ConfigureTransports
function, doing so is comparatively clunky to configuring it directly on the http2.Transport.ConfigureTransports
sets up a different connPool implementation than if it were left nil before the first usageThe proposal then is to add the following API
which follows the existing http.Transport API. To preserve backwards compatibility, idleConnTimeout in http2.Transport will use the following rules
The text was updated successfully, but these errors were encountered: