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: make nil *Client equivalent to zero Client #26036

Closed
bontibon opened this issue Jun 25, 2018 · 3 comments

Comments

Projects
None yet
5 participants
@bontibon
Copy link
Contributor

commented Jun 25, 2018

net.Resolver in currently allows the following (ignoring #24330):

var r *net.Resolver
fmt.Println(r.LookupHost(context.Background(), "example.com"))

Note that nil *Resolver is equivalent to the zero Resolver.

I propose that using the nil net/http.*Client also be permitted. This is useful when allowing callers to specify a *Client to use for HTTP operations:

type Config struct {
	Client *http.Client
}

func (c *Config) Get() {
	resp, err := c.Client.Get("https://example.com")
	// Rather than:
	client := c.Client
	if client == nil {
		client = http.DefaultClient
	}
	resp, err := client.Get("https://example.com")
}

@gopherbot gopherbot added this to the Proposal milestone Jun 25, 2018

@gopherbot gopherbot added the Proposal label Jun 25, 2018

@meirf

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

One concern imo is that this reduces to the problem with DefaultTransport. Specifically, DefaultClient uses DefaultTransport which uses DefaultMaxIdleConnsPerHost which frequently bites/surprises people due to its small size (2) especially relative to DefaultTransport's MaxIdleConns (100). This proposal would add another layer of obscurity from that unfortunate default. (If we decided to increase DefaultMaxIdleConnsPerHost in Go2, this wouldn't be an issue.)

You could argue that nil Resolver has a similar issue, but for stdlib's clients/transports you have the alternate option to create your own with better parameters. Also, dns is much more lightweight than http so regular usage shouldn't run into the resolver's file descriptor limitation as much as DefaultTransport runs into DefaultMaxIdleConnsPerHost limitation.

Anyway, maybe the convenience is worth it.

/cc @bradfitz @odeke-em

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

I think this would've been fine (and probably good) had we started with this, but doing it at this point would cause a bunch of previously-correct code in the wild to now be crash-prone.

Anything that currently checks, say, an *http.Client's Transport or Jar fields now also needs to check whether the client's nil also.

Per discussion with the proposal review group, I think we should decline this.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

I wonder if proposals of the type "we wish we had done this from the start" could be reconsidered for Go2 in bulk.

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