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

transport/http: update default transport settings #496

Merged
merged 9 commits into from
May 28, 2020

Conversation

tritone
Copy link
Contributor

@tritone tritone commented May 21, 2020

Updates the default HTTP transport to use a larger value for
MaxIdleConnsPerHost. This improves performance under high load
for the GCS client.

Copied from gerrit CL https://code-review.googlesource.com/c/google-api-go-client/+/55470

Updates the default HTTP transport to use a larger value for
MaxIdleConnsPerHost. This improves performance under high load
for the GCS client.

Change-Id: I11a7cdb30e867b60df4cc0280cf7d7c56a750513
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 21, 2020
@broady
Copy link
Contributor

broady commented May 21, 2020

This actually has another effect: the transport (http.DefaultTransport) would now no longer be shared between clients.

I don't know if that matters, but if it does, we would want the tweaked transport stored as a package-level variable rather than being copied every time a new client is created.

Otherwise, this looks good. I would tweak the build tags a bit:

//====dial.go===

// defaultBaseTransport returns the base HTTP transport.
// On App Engine, this is urlfetch.Transport.
// Otherwise, use a default transport, taking most defaults from
// http.DefaultTransport.
// If TLSCertificate is available, set TLSClientConfig as well.
func defaultBaseTransport(ctx context.Context, clientCertSource cert.Source) http.RoundTripper {
	if appengineUrlfetchHook != nil {
		return appengineUrlfetchHook(ctx)
	}

	trans := clonedTransport(http.DefaultTransport)
	if trans == nil {
		trans = fallbackBaseTransport()
	}
	trans.MaxIdleConnsPerHost = 100

	if clientCertSource != nil {
		trans.TLSClientConfig = &tls.Config{
			GetClientCertificate: clientCertSource,
		}
	}

	return trans
}

// fallbackBaseTransport is used in <go1.13 as well as in the rare case if http.DefaultTransport has been reassigned something that's not a *http.Transport.
func fallbackBaseTransport() *http.Transport {
	return &http.Transport{
		Proxy: http.ProxyFromEnvironment,
		DialContext: (&net.Dialer{
			Timeout:   30 * time.Second,
			KeepAlive: 30 * time.Second,
			DualStack: true,
		}).DialContext,
		MaxIdleConns:          100,
		MaxIdleConnsPerHost:   100,
		IdleConnTimeout:       90 * time.Second,
		TLSHandshakeTimeout:   10 * time.Second,
		ExpectContinueTimeout: 1 * time.Second,
	}
}


//====default_transport_go113.go===

// clonedTransport returns the given RoundTripper as a cloned *http.Transport.
// It returns nil if the RoundTripper can't be cloned or coerced to *http.Transport.
func clonedTransport(rt http.RoundTripper) *http.Transport {
	t, ok := rt.(*http.Transport)
	if !ok {
		return nil
	}
	return t.Clone()
}


//====default_transport_not_go113.go===

func clonedTransport(rt http.RoundTripper) *http.Transport { return nil }

@codyoss codyoss added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 21, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 21, 2020
broady
broady previously approved these changes May 27, 2020
Change-Id: I64bc2e9d13a218cb370620beea8e7b696916fa42
Change-Id: I90a310a0ad609ae0c813d4d4c03918568dd81dea
Change-Id: I5c38f25e3ced41adbc518f1247acc5b57427046e
@tritone
Copy link
Contributor Author

tritone commented May 27, 2020

Thanks for your comments, @broady! I fixed the cloning and build tag factoring per your suggestion.

About the fact that the default transport is no longer shared between clients-- do you think I should address that in this PR? Let me know what you think.

Change-Id: If63f83cee877aa945f370899425661c509001032
transport/http/dial.go Show resolved Hide resolved
transport/http/dial.go Show resolved Hide resolved
Change-Id: I8a4df55c62e62028fcc9728bbc8e4e77bafcc617
@broady
Copy link
Contributor

broady commented May 28, 2020

About the fact that the default transport is no longer shared between clients-- do you think I should address that in this PR? Let me know what you think.

Yeah, probably worth doing. Sent some code suggestion.

@broady broady self-requested a review May 28, 2020 19:17
Change-Id: I4c7a8b139727e3991f8d217a96c3c4d3ac9c9d11
Change-Id: Ia8f4736ae5e3d7187abefc95b6aec7752055758d
Change-Id: Ia618ed032d9595596a2cd45a484acd45f05b1cff
@tritone tritone merged commit a83bacb into googleapis:master May 28, 2020
@tritone tritone deleted the http-trans branch May 28, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants