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

Difference between http.request({keepAlive}) and http.request({agent: {keepAlive}}) #1300

Closed
vvo opened this issue Mar 31, 2015 · 4 comments
Closed
Assignees
Labels
http Issues or PRs related to the http subsystem.

Comments

@vvo
Copy link

vvo commented Mar 31, 2015

Hi,

I have been playing with keepAlive in io.js and node 0.12. Something strange is that the documentation states that one could use:

http.request({keepAlive: true})

But this line did not trigger keepAlive on my requests. I could not find a code path using it in the nodejs codebase.

Is this a mistake or am I missing something? Maybe agent keepAlive and request keepAlive are not the same thing, then maybe we could clarify things.

Using:

http.request({agent: http.Agent({keepAlive: true})})

works, as shown in the tests https://github.com/joyent/node/blob/857975d5e7e0d7bf38577db0478d9e5ede79922e/test/simple/test-http-agent-keepalive.js
https://github.com/iojs/io.js/blob/v1.x/test/parallel/test-http-agent-keepalive.js

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Mar 31, 2015
@Fishrock123
Copy link
Contributor

keepAlive and keepAliveMsecs are wrongly indented in the docs. They should be indented one more level, since they are for http.Agent.

@Fishrock123 Fishrock123 self-assigned this Apr 9, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this issue Apr 9, 2015
Fixes: nodejs#1300
PR-URL: nodejs#1384
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Fishrock123
Copy link
Contributor

Fixed in 69bc138, thanks for reporting! :)

@vvo
Copy link
Author

vvo commented Apr 10, 2015

awwww now it makes sense.. the nodejs docs as the same issue.

@vvo
Copy link
Author

vvo commented Apr 10, 2015

@Fishrock123 You sure this is the right fix?

2015-04-10-094203_1142x357_scrot

Reading this is confusing, it explains that agent: can take value keepAliveMsecs which is not true.

I think we should just remove keepAlive and keepAliveMsecs from here and link to the Agent constructor where they are explained.

Fishrock123 added a commit to Fishrock123/node that referenced this issue Apr 11, 2015
These can only be specified in the options for http.Agent

Fixes: nodejs#1300
PR-URL: nodejs#1392
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants