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

Add MaxIdleConnsPerHost. Fixes too many open files error. #187

Merged
merged 1 commit into from
Feb 9, 2016

Conversation

emilevauge
Copy link
Member

This PR adds an option MaxIdleConnsPerHost to be used when too many open files error are encountered.
As explained here golang/go#6785, during high loads, it seems that:

when the number of requests/s to the proxy goes up, the small amount of bookkeeping that is required to recycle a connection (putIdleConn etc) starts taking just long enough that the next request in the pipeline gets in before a connection is idle. The Transport starts Dial:ing, adding more connections to take care of, and it explodes chaotically. I would prefer if it blocked and awaited an idle connection instead of Dial:ing at some point

Fixes #157 #127

@vdemeester
Copy link
Contributor

LGTM 🐸

vdemeester added a commit that referenced this pull request Feb 9, 2016
Add MaxIdleConnsPerHost. Fixes too many open files error.
@vdemeester vdemeester merged commit bb3b9f6 into master Feb 9, 2016
@vdemeester vdemeester deleted the add-max-idle-conns-per-host branch February 9, 2016 21:49
@Yggdrasil
Copy link
Contributor

The config file mentions "If zero, DefaultMaxIdleConnsPerHost is used", however I don't see the setting for DefaultMaxIdleConnsPerHost anywhere? In other words, what's the default value?

I just ran into the "Too many open files" problem on one of my servers. I have no idea what to set this to. Is setting MaxIdleConnsPerHost = 200 a good start? How can I tell, please?

@timoreimann
Copy link
Contributor

@Yggdrasil DefaultMaxIdleConnsPerHost is defined in the net/http package: https://golang.org/pkg/net/http/#pkg-constants . It's set to 2.

I will create a PR to add some clarity to the docs.

@timoreimann
Copy link
Contributor

Turns out we use a default of 200 already, but the docs aren't up to date.

#1239 is fixing this.

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

Successfully merging this pull request may close these issues.

None yet

4 participants