x/net/http2: add IdleConnTimeout, DisableKeepAlives, ExpectContinueTimeout, and ResponseHeaderTimeout to http2.Transport

[dastbe]

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.

• It requires you to setup an http1 Transport which you may not otherwise need. you also need to guard against behavior differences elsewhere if the underlying transport is non-nil
• It requires you to reset the connPool as ConfigureTransports sets up a different connPool implementation than if it were left nil before the first usage

The proposal then is to add the following API

    // IdleConnTimeout is the maximum amount of time an idle
    // (keep-alive) connection will remain idle before closing
    // itself.
    // Zero means no limit.
    IdleConnTimeout time.Duration

which follows the existing http.Transport API. To preserve backwards compatibility, idleConnTimeout in http2.Transport will use the following rules

1. if the transport IdleConnTimeout is non-zero, return that
2. if the underlying transport is non-nil, return the underly transport's IdleConnTimeout
3. otherwise, return 0 (no timeout)

[gopherbot]

Change https://go.dev/cl/461641 mentions this issue: http2: add IdleConnTimeout to http2.Transport

[ianlancetaylor]

CC @neild @bradfitz

[neild]

There are a few other knobs that the http2.Transport will read from the http.Transport when available.

DisableCompression can be set on either the http.Transport or the http2.Transport.

DisableKeepAlives, ExpectContinueTimeout, IdleConnTimeout, and ResponseHeaderTimeout can only be set on the http.Transport.

We should have a consistent policy here; either we should add all those fields to http2.Transport or none of them.

[dastbe]

We should have a consistent policy here; either we should add all those fields to http2.Transport or none of them.

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 ConfigureTransports is the http1 and http2 transports operate in tandem, i.e. a change to the http1 transport automatically propagates to the http2 transport. This does simplify setup since given a correctly configured http.Transport I also get a correctly configured http2.Transport, and it does ensure they stay in sync (at the expense of some spooky action at distance when updating the http.Transport). However, it precludes users from customizing the http2.Transport differently from the http.Transport and limits the utility of using the http2.Transport directly.

I could see an argument for making the configuration transfer static by defining these fields on http2.Transport and modifying ConfigureTransports to copy them over rather than pull at runtime. However, that would break code that relies on changes to propagate after the call to ConfigureTransports.

A compromise solution that preserves behavior could be

1. Keep the existing fallback behavior for existing fields but also explicitly add to http2.Transport. Have new fields be copy on configure
2. Keep ConfigureTransports behavior as it currently is for existing fields, and copy over new fields
3. Add a new WireTransports function that takes an http2.Transport and an http.Transport and does the upgrade wiring but does not embed the http.Transport into the http2.Transport.

though honestly that just seems more complex than having consistent fallback behavior for the fields.

[dastbe]

@neild any thoughts on this?

[bradfitz]

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}

[neild]

@bradfitz and I talked this over, and we support making it so that an http2.Transport can be configured without needing to bring in an HTTP/1 transport as well.

That would mean adding four new fields to http2.Transport: DisableKeepAlives, ExpectContinueTimeout, IdleConnTimeout, and ResponseHeaderTimeout.

Keep alives will be disabled if DisableKeepAlives is set on either the HTTP/1 or HTTP/2 transports. (Consistent with the existing DisableCompression knob.)

For timeouts, we'll prefer the value on the HTTP/2 transport if non-zero, falling back to the HTTP/1 transport's value.

[dastbe]

Sounds good!

For timeouts, we'll prefer the value on the HTTP/2 transport if non-zero, falling back to the HTTP/1 transport's value.

For IdleConnTimeout I've followed that pattern here: https://go.dev/cl/461641

Once I get another spare night this week I can come back through and wire through the other fields.

[rsc]

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

[rsc]

Have all concerns with this proposal been addressed?

[neild]

No concerns here.

[rsc]

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

[rsc]

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

[gopherbot]

Change https://go.dev/cl/497195 mentions this issue: http2: add IdleConnTimeout to http2.Transport

[bbassingthwaite]

Would it be possible to consider exposing ReadIdleTimeout on the http2Transport as well? From what I can tell it's impossible to set this field from the public API. This controls whether HTTP/2 keep-alives are sent from the client to server and is not enabled today.

[dmitshur]

If I understand correctly, the accepted proposal is to add four new fields to http2.Transport as described in https://github.com/golang/go/issues/57893#issuecomment-1406834509. I updated the title accordingly, otherwise it's hard to understand why this is open after CL 497195 is submitted. Please correct if needed.