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

GOMAXPROCS underestimates potential concurrency for low CPU counts #15

Open
MaerF0x0 opened this issue Aug 7, 2020 · 2 comments
Open

Comments

@MaerF0x0
Copy link

MaerF0x0 commented Aug 7, 2020

I would suggest we increase the minimum MaxIdleConnsPerHost to something like 5-10 range.
I suggest this because a network connection isn't going to Pin a CPU and resource restriction should probably be done externally via a sync/semaphore?

Here is a potential solution I arbitrarily made up

// Conns calculates a good number of idle connections assuming a GOPROC can handle more than 1 network connection at a time . Algo is Max(GOMAXPROCS*2, 8) 
func Conns() int {
  if  max := runtime.GOMAXPROCS(0) * 2 ; max > 8 {
        return max
  }
  return 8
}
// DefaultPooledTransport returns a new http.Transport with similar default
// values to http.DefaultTransport. Do not use this for transient transports as
// it can leak file descriptors over time. Only use this for transports that
// will be re-used for the same host(s).
func DefaultPooledTransport() *http.Transport {
	transport := &http.Transport{
		Proxy: http.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,
		MaxIdleConnsPerHost:   runtime.GOMAXPROCS(0) + 1, // <-- Probably could be much larger on the low end
	}
	return transport
}
@jefferai
Copy link
Member

I think it depends what you're going for. Do you want to keep the number of fds as low as possible when connecting to lots of different hosts, or do you want to use it as a more general purpose connection pool?

It may be worth introducing an option pattern to allow runtime tweaks of some of these parameters.

@mitar
Copy link

mitar commented Oct 31, 2023

I think it is easy to modify all of those on returned http.Transport struct? This is package is really about sane defaults. If you want to change those defaults, you can do it yourself. They are all public fields. I do not think we should be adding options to the package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants