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

net/http: add Transport.Clone #26013

Closed
perillo opened this issue Jun 22, 2018 · 14 comments

Comments

@perillo
Copy link

commented Jun 22, 2018

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.

@perillo perillo changed the title net/http net/http: document if it is safe to copy exported fields of Transport Jun 22, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

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.

@bradfitz bradfitz added this to the Go1.12 milestone Jun 22, 2018

@perillo

This comment has been minimized.

Copy link
Author

commented Jun 22, 2018

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 rsc changed the title net/http: document if it is safe to copy exported fields of Transport net/http: add Transport.Clone Sep 26, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

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.

@rsc rsc added the NeedsFix label Sep 26, 2018

@gopherbot gopherbot removed the NeedsFix label Sep 26, 2018

@akavel

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Ping @bradfitz

@akavel

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

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

@akavel

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@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 bradfitz modified the milestones: Go1.13, Unplanned Apr 30, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Apr 30, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.