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

Default agent is limited with 5 sockets #22

Closed
floatdrop opened this issue Dec 15, 2014 · 7 comments · Fixed by #30
Closed

Default agent is limited with 5 sockets #22

floatdrop opened this issue Dec 15, 2014 · 7 comments · Fixed by #30

Comments

@floatdrop
Copy link
Contributor

It seems like there an open socket left after got has redirect reply from server. Setting {agent: false} fixes test, but feels like it should be working by default.

Default http agent in node is limited by 5 open sockets. Read more in hyperquest readme.

@floatdrop floatdrop changed the title Got is not closing sockets on redirects Default agent is limited with 5 sockets Dec 30, 2014
@floatdrop
Copy link
Contributor Author

Should we set agent to false by default as well? This seems like not obvious behaviour, but 5 sockets limit bothers me a lot.

@julien-f
Copy link
Contributor

At first thought I would be against but if @substack does it… :p

@sindresorhus
Copy link
Owner

Yes, as long as we document it. I think it's fixed in Node 0.11 though.

@floatdrop
Copy link
Contributor Author

@sindresorhus yes - this is fixed in 0.11.x. I would wait for release for some time, instead of setting default agent to false (it also breaks https tests) now and revert it back.

@sindresorhus
Copy link
Owner

Node 0.10 will still be have to supported for a long time after (or if ever) node 0.12 is released. I think we should do it.

@julien-f
Copy link
Contributor

Having the same behavior on Node 0.10 & 0.12 is reason enough to do it.

@floatdrop
Copy link
Contributor Author

To track progress on this:

Setting agent: false breaks https requests - https://github.com/substack/hyperquest/issues/19

There is another approach to this (see here) - setting maxSockets to Infinity (like 0.11 node does - nodejs/node-v0.x-archive@9fc9b87#diff-5f7fb0850412c6be189faeddea6c5359R100):

opts.maxSockets = opts.maxSockets || Infinity;
// ...
var fn;
if (parsedUrl.protocol === 'https:') {
    fn = https;
    opts.agent = new https.Agent(opts);
} else {
    fn = http;
    opts.agent = new http.Agent(opts);
}

But this will leave connections in pool - so redirect test will not be completed.

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 a pull request may close this issue.

3 participants