Go: net/http: add Transport.Clone

[perillo]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

Describe the issue

In a recent topic on golang-nuts (https://groups.google.com/forum/#!topic/golang-nuts/JmpHoAd76aU) the user reported how hard is to set safe defaults on a custom Transport.

1. New fields may be added in future releases, and have custom (not zero) default value set for DefaultTransport.

2. Using a function to copy exported fields (like https://play.golang.org/p/cotZaihUxdi) may cause problems.

I think that the documentation of Transport should document this issue. Is it safe to copy the exported fields of a Transport?

If this cannot be guaranteed, the http package should export a new function, like NewTransportWithDefaults.

[bradfitz]

Well, it is safe to do this:

package main

import "net/http"

func main() {
        defCopy := *(http.DefaultTransport.(*http.Transport))
        var tr *http.Transport = &defCopy
        _ = tr

}

... but only early in the program before the DefaultTransport has been used.

And unfortunately that idiom causes vet failures:

./x.go:6: assignment copies lock value to defCopy: net/http.Transport contains sync.Mutex

So, yeah, maybe a new function to return a default is warranted, and/or we can fix vet.

[perillo]

The Copy function from https://play.golang.org/p/cotZaihUxdi does not cause vet to complain, and I'm pretty sure that all the exported fields of go 1.10 Transport are safe to copy. The problem is if this feature can be guarantee in the future.

On the other hand, a new function advantage is that it can also setup HTTP 2.

[rsc]

If code needs to make transports derived from others with changes, then it sounds like we should add a Clone method. The reflect blob in the original report is definitely not something we want to encourage.

[akavel]

@rsc As to the help wanted tag, I wouldn't be very sure what to start contributing yet as the Clone method; therefore, per NeedsDecision, would you intend further discussion (such as design questions/proposals) taking place here, or in a proposal in the proposals repo?

Specifically, I'm still having trouble understanding for Clone:

• would it mean the http.RoundTripper interface should get new Clone() http.RoundTripper method? wouldn't this be non-backwards-compatible? or should the Clone method be discovered dynamically with the likes of a clonable, ok := http.DefaultTransport.(interface{Clone() *http.Transport}) cast?
• how should one approach implementing the http.Transport.Clone method for cloning the DialContext field, which is a function? would it be deemed safe to just do a shallow copy of the field? alternatively, if NewDefaultTransport was used instead (see the proposal below), it should be fairly easy to construct a new DialContext.

For the record, and ease of discussion, an alternative proposal that I pondered in the original post on golang-nuts could be something like:

package http

var DefaultTransport = NewDefaultTransport()

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

[ianlancetaylor]

I'm moving this from NeedsDecision to NeedsFix.

Seems like copying a Transport is a general issue. Adding NewDefaultTransport only fixes the case of copying the default transport. It's a plausible fallback but before that let's see if we can fix the general issue.

I'm not sure why RoundTripper matters here; can you explain?

I would imagine that a Transport.Clone method would simply copy the DialContext field as it does the other simple fields. Is there any problem with doing that?

[akavel]

@ianlancetaylor Thanks! As to RoundTripper: the http.DefaultTransport is of RoundTripper interface type, that's why I was wondering where the Clone should be defined. (In my original use case, I need to clone the http.DefaultTransport variable.) I suppose adding the method on Transport and accessing it via a cast on DefaultTransport is the only reasonable/acceptable way to go. In the common case it would probably be OK to call it as http.DefaultTransport.(*Transport).Clone().

As to DialContext, I believe I didn't think it through well enough. The comment on DialContext already seems to mention that the func must be safe to run concurrently, so I suppose there shouldn't be any more problems with it.

edit: I tried to start writing the Clone, in order to submit a CL, but I'm becoming afraid to do that. Specifically, the comment on unexported field Transport.nextProtoOnce makes me feel uneasy:

    // nextProtoOnce guards initialization of TLSNextProto and
    // h2transport (via onceSetNextProtoDefaults)
    nextProtoOnce sync.Once

Notably, it seems to have a subtle interaction with initialization of HTTP2 support. IIUC, if Transport.TLSNextProto is initialized to nil (default), it allows for automatic initialization of HTTP2 support in the Transport. However, this results in nextProtoOnce.Do(t.onceSetNextProtoDefaults) being called. This then proceeds to set various unexported and exported fields of the Transport, including .TLSNextProto. This leads to two problems, IIUC:

1. Clone cannot be safely run on a Transport concurrently with other methods (which would invalidate it as a fix to the original issue), unless it also calls nextProtoOnce.Do(...) at the beginning. But it feels weird and suspicious to me that a Clone method would modify the original object.
2. If the original object started with .TLSNextProto=nil, this means "allow HTTP2". After the nextProtoOnce.Do(...) call however, .TLSNextProto is set to non-nil value. Calling Clone() on it would then create an object with non-nil .TLSNextProto, which would mean "disallow HTTP2" per the field's godoc (and per the implementation, which checks this field indeed). This would make the Clone method create an object, which is superficially and externally similar, but internally has significant differences and behaves differently, thus not being really a clone...

Or do I misunderstand? @bradfitz ?

[akavel]

@ianlancetaylor Could you please help find someone to give some guidance on how to proceed with this issue? It's marked as "help-needed", but then since I outlined my concerns as to how should I proceed with implementing it 5 months ago, I got no response, and I believe it's seriously non-trivial/tricky esp. for non-core contributors :/ Also, based on those concerns, would you still think that .Clone() is the way to go here?

edit: Please note, that the issue is particularly annoying to us, in that we need to remember to revisit a fragment of code related to it every time we upgrade Go to a newer release... and as you certainly understand, this can be easy to miss, which then can have some subtle, tricky, hard to detect and hard to track consequences for us :/

[ianlancetaylor]

Ping @bradfitz

[akavel]

@ianlancetaylor Could you please maybe at least remove the _NeedsFix_ tag and mark again with _NeedsDecision_ or something? Maybe the _NeedsFix_ or the _help-wanted_ hides it in Brad's filters or something? :( I don't believe "The path to resolution is known" in this case anymore, per my comment above.

[bradfitz]

I'm fine with Clone calling the receiver value's Once.Do func(s).

As for TLSNextProto, how about adding a new unexported bool like tlsNextProtoWasNil bool that'll control whether Clone clones that field as-is or sets it to nil?

[bradfitz]

@akavel, any update? You have 1.5 days to get this in to Go 1.13 before the freeze.

[akavel]

@bradfitz Nope, sorry, no chance. My life & work situation unexpectedly got somewhat dense in the meantime, so I'm not sure if and when I'll be able to proceed with this.

[bradfitz]

I sent https://go-review.googlesource.com/c/go/+/174597

[gopherbot]

Change https://golang.org/cl/174597 mentions this issue: net/http: add Transport.Clone