Skip to content

Issue #570: Disable keep-alive if no agent. #573

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

Closed
wants to merge 1 commit into from
Closed

Issue #570: Disable keep-alive if no agent. #573

wants to merge 1 commit into from

Conversation

jwalton
Copy link

@jwalton jwalton commented Feb 3, 2014

This is an alternative fix for #570 (see my other pr #572).

The idea here is that if no agent is specified, we explicitly set keepAlive to false.

If keepAlive is true, then every time we proxy a request we will create a new connection (because there is no agent managing a pool of connections), but with keepAlive set, node.js will not kill the connection after we make a request. The connection will stay open (potentially for several minutes) eating up a file descriptor.

With keepAlive set false, node.js will close the connection for us after the request is made, cleaning up the resources immediately. Under heavy load, this will result in far more connections than in PR #572, but may or may not result in better performance overall. When heavy load clears, however, the connection count will drop back down immediately.

Note that, with or without this fix, connections are not being reused - all this does is clean up connections after we're done with them. Also not that in node v0.11.11, this option is implicitly set for us when node assigns us an agent.

@jwalton
Copy link
Author

jwalton commented Feb 3, 2014

You would also need extremely high load to actually run out of FDs. I can hold down CTRL-R in my browser and constantly load a 404 page, and it climbs up into the ~170 range on my mac, but this is a far cry from the 10K range it will get to without this fix.

@jcrugzz
Copy link
Contributor

jcrugzz commented Feb 7, 2014

@jwalton This is something I can jive with. We haven't seen issues with this currently but it does make sense as we need to tell the operating system in SOME WAY to close the FD. Have you been able to test that this still works appropriately with 0.11.x? I cannot see why not, I'd just like to be sure :). Thanks for the contributions!

@jwalton
Copy link
Author

jwalton commented Feb 7, 2014

I can't say for sure that I did try it, so I'll try it again to be extra
sure. :)

-Jason
On Feb 7, 2014 5:28 PM, "Jarrett Cruger" notifications@github.com wrote:

@jwalton https://github.com/jwalton This is something I can jive with.
We haven't seen issues with this currently but it does make sense as we
need to tell the operating system in SOME WAY to close the FD. Have you
been able to test that this still works appropriately with 0.11.x? I
cannot see why not, I'd just like to be sure :). Thanks for the
contributions!

Reply to this email directly or view it on GitHubhttps://github.com//pull/573#issuecomment-34514464
.

@jwalton
Copy link
Author

jwalton commented Feb 8, 2014

Arg. Tonight I'm seeing this problem happen in node 0.10 and node 0.11,
both with and without my fix. :( This'll have to wait until Monday I'm
afraid.

On Fri, Feb 7, 2014 at 6:12 PM, Jason Walton jason@thedreaming.org wrote:

I can't say for sure that I did try it, so I'll try it again to be extra
sure. :)

-Jason
On Feb 7, 2014 5:28 PM, "Jarrett Cruger" notifications@github.com wrote:

@jwalton https://github.com/jwalton This is something I can jive with.
We haven't seen issues with this currently but it does make sense as we
need to tell the operating system in SOME WAY to close the FD. Have you
been able to test that this still works appropriately with 0.11.x? I
cannot see why not, I'd just like to be sure :). Thanks for the
contributions!

Reply to this email directly or view it on GitHubhttps://github.com//pull/573#issuecomment-34514464
.

@jcrugzz
Copy link
Contributor

jcrugzz commented Feb 11, 2014

@jwalton Looks like we should be fine in 0.10.25.

@jcrugzz
Copy link
Contributor

jcrugzz commented Mar 27, 2014

fixes in 89a22bc

@jcrugzz jcrugzz closed this Mar 27, 2014
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.

2 participants