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

CC @neild @bradfitz

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.

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.

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

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.

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

Have all concerns with this proposal been addressed?

No concerns here.

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

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

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

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.