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

proposal: net/http: Add NewDefaultTransport function #39299

Open
rittneje opened this issue May 28, 2020 · 8 comments
Open

proposal: net/http: Add NewDefaultTransport function #39299

rittneje opened this issue May 28, 2020 · 8 comments
Labels
Projects
Milestone

Comments

@rittneje
Copy link

@rittneje rittneje commented May 28, 2020

We would like to be able to use the default options for http.Transport without modifying the global, or worrying about whether anyone has modified the transport, or having to typecast it to call Clone. I propose adding a NewDefaultTransport() *Transport function to the http package. It should return a new Transport with the default options filled out. The global definition then becomes var DefaultTransport RoundTripper = NewDefaultTransport(). Note that the function returns *Transport not RoundTripper so we don't have to typecast it.

func NewDefaultTransport() *Transport {
    return &Transport{
        Proxy: ProxyFromEnvironment,
        DialContext: (&net.Dialer{
            Timeout:   30 * time.Second,
            KeepAlive: 30 * time.Second,
            DualStack: true,
        }).DialContext,
        ForceAttemptHTTP2:     true,
        MaxIdleConns:          100,
        IdleConnTimeout:       90 * time.Second,
        TLSHandshakeTimeout:   10 * time.Second,
        ExpectContinueTimeout: 1 * time.Second,
    }
}

This was previously discussed in #26013, but in that case people wanted to pull in modifications to the global that had been made, so the Clone() method was introduced.

@gopherbot gopherbot added this to the Proposal milestone May 28, 2020
@gopherbot gopherbot added the Proposal label May 28, 2020
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented May 28, 2020

how is this different from tr := http.DefaultTransport.Clone() besides being 2 characters shorter?

@rittneje
Copy link
Author

@rittneje rittneje commented May 28, 2020

@seankhliao As I mentioned, Clone() will also pull in any modifications that have been made to the global, and requires a typecast that can fail if the global itself was replaced. We want a clean default Transport to work with.

@eandre
Copy link

@eandre eandre commented Jun 4, 2020

I'm not sure I understand the use case of "I will happily accept whatever defaults the Go project deems appropriate (from NewDefaultTransport) but reject any defaults decided by the author of the running program and its specific environment".

Why is one set of defaults fine and not the other?

@rittneje
Copy link
Author

@rittneje rittneje commented Jun 4, 2020

@eandre I'm not sure what you mean by "the author of the running program". Any external library can just modify the global variable. So the issues are:

  1. We trust the Go authors to use reasonable defaults. If the zero value of http.Transport used these defaults, we would be happy with that, but since it doesn't, this is the next best thing. I do not want to have to worry about what some third-party dependency (direct or indirect) happens to do with that global in an init function.
  2. If anything changes that global to be something other than an *http.Transport (which is possible since it is actually of type http.RoundTripper), calling the Clone method does not work.

In my opinion, the Clone method was a misfeature for the original use case described in #26013. I cannot think of any situation where I would want to copy the potentially modified global. The whole reason for not just using the global directly in my code is that I want to be isolated from such changes, and I want my changes to be isolated from other sections of code.

@eandre
Copy link

@eandre eandre commented Jun 5, 2020

I don't have strong feelings for or against your proposal, but you seem to be taking an adversarial stance against these dependencies ("who knows what they might do?"). My point of view is that as an author of a binary, you are responsible for what libraries you pull in and what they do.

Rather than viewing any change to the http.DefaultTransport configuration as hostile, you could also view them from the perspective of "the author of the binary should know what they're doing, and if they are using code that reconfigures the default transport, I should follow along with their wishes". I agree with you that in most environments there is no need to reconfigure these settings. But perhaps they want to set strict timeout settings, or perhaps they have a tricky networking setup that requires custom proxying.

Ignoring these settings is only "correct" in a sense if you take the stance that any changes to http.DefaultTransport are being done by library authors who don't know better, and the author of the binary is unaware that it's happening. I think that's not a particularly charitable perspective. That's why I argued that if you truly need a specific configuration, it's easy to construct your own *http.Transport with those settings. I just can't see any use case for "I don't care about the specifics of the configuration as long as it was chosen by the authors of the Go Project [as opposed to the author of the binary]". But maybe I'm missing something – If you could explain a concrete use case that would be great.

@rittneje
Copy link
Author

@rittneje rittneje commented Jun 5, 2020

@eandre I am confused by your argument. You say both "as an author of a binary, you are responsible for what libraries you pull in and what they do" and "the author of the binary should know what they're doing, and if they are using code that reconfigures the default transport, I should follow along with their wishes." Those sound like mutually exclusive scenarios to me.

If I am authoring a binary, then I should have control over the creation of the transport. If I have requirements concerning the timing of the network operations, then I should be creating a transport with such settings. But I don't see a reason I would want to clone the default transport (as in the global) to do so. At best, it would be the same thing as constructing a new default transport, and at worst it could actually cause an unintentional issue. However, I could see why somebody would want to modify/replace the global variable itself in their top-level code.

If I am authoring a library, then I should allow the client to pass in an http.RoundTripper and let them have full control over its configuration. Consequently, the Clone method does not seem useful, because the round tripper need not be an http.Transport, and even if it were, I'm not sure why I would want to clone it.

My concrete use case is we needed to disable HTTP/2 on the transport, but wanted to leverage the rest of the default settings. Consequently, we ended up with something like:

transport := http.DefaultTransport.(*http.Transport).Clone()
transport.TLSNextProto = make(map[string]func(authority string, c *tls.Conn) http.RoundTripper)
// use transport

This did not actually work as expected due to #39302. However, even if it did, the fact that that requires a typecast, and there is no way to handle the typecast failing without just aborting the binary, is disconcerting. Additionally, we do not like the implication that we are forced to accept any changes to the global that could have been made elsewhere. (This is also a concern with just using the default transport global variable normally, but it became more apparent here.) If someone really wants those changes, they are free to use the Clone method, but it should not be the only option.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Jun 9, 2020

It would be helpful if you could cite a real-world example that drove this proposal. Was there a case where a third-party library modified some values of the DefaultTransport or swapped it with something else without documenting it ? The same argument could be theoretically made for any global variable in the standard library.

IIUC, this is primarily about not having to copy the default timeouts to the codebase separately to create a separate transport. Would that be accurate ?

@rittneje
Copy link
Author

@rittneje rittneje commented Jun 10, 2020

Was there a case where a third-party library modified some values of the DefaultTransport or swapped it with something else without documenting it ?

Thankfully, this has not yet happened with the libraries that we have used. But the issues I see are (1) this leads to fragile code that fundamentally relies on nothing else in the entire binary (including some transitive dependency of a dependency) changing that global; and (2) it prevents us from writing properly robust code due to the following no-win situation.

t, ok := http.DefaultTransport.(*http.Transport)
if !ok {
    // now what?
}
t2 := t.Clone()
...

The same argument could be theoretically made for any global variable in the standard library.

Indeed, which is partially why, in my opinion, properly robust code should never be using global variables.

IIUC, this is primarily about not having to copy the default timeouts to the codebase separately to create a separate transport. Would that be accurate ?

Yes, especially because we would have to manually maintain those default settings with each new Go release. As I mentioned above, if the zero value for http.Transport had those semantics it would have been fine, but unfortunately it does not.

Also, one thing I would like some clarification on is what specifically instigated the introduction of the Clone() method. In #26013 it was mentioned that "copying a Transport is a general issue." But that means the code in question is specifically looking for *http.Transport and not http.RoundTripper. Consequently, any code relying on the Clone() method either isn't actually generalized, or inevitably runs into that same no-win situation with the typecast failing. In other words, what use cases actually necessitated the Clone() method over NewDefaultTransport()?

@rsc rsc added this to Incoming in Proposals Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.