-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
https.request(): Agent name inconsistency when servername not supplied #6687
Comments
Are you using a custom agent? The default HTTPS agent includes servername iff |
@timotheeg ping |
Hey. Sorry I missed the message from @bnoordhuis. I'm using an agent indeed, but it's the one supplied by https, which I instantiate as: var secure_agent = new https.Agent({
keepAlive: true,
maxSockets: CONFIG.max_http_sockets || 1000,
maxFreeSockets: CONFIG.max_http_freeSockets || 256,
keepAliveMsecs: 15 * 1000
}); Sorry, with the workaround of supplying I think the issue can be seen by code inspection however. And I realized that I missed mentioning a piece of info about my requests, which helps shows the issue clearer. So further info are below. I am requesting for So to sum up: I hope that makes sense. I don't know when I'll have the time to make that test case, I'll try over the weekend :/ |
If calling `https.request()` with `options.headers.host` defined and `options.servername` undefined, `https.Agent.createSocket` mutates connection `options` after `https.Agent.addRequest` has created empty socket pool array with mismatching connection name. This results in two socket pool arrays being created and only the former gets eventually deleted by `removeSocket` - causing a memory leak. This commit fixes the leak by making sure that `addRequest` does the same modifications to `options` object as the `createSocket`. `createSocket` is intentionally left unmodified to prevent userland regressions. Test case included. Fixes: nodejs#6687
If calling `https.request()` with `options.headers.host` defined and `options.servername` undefined, `https.Agent.createSocket` mutates connection `options` after `https.Agent.addRequest` has created empty socket pool array with mismatching connection name. This results in two socket pool arrays being created and only the last one gets eventually deleted by `removeSocket` - causing a memory leak. This commit fixes the leak by making sure that `addRequest` does the same modifications to `options` object as the `createSocket`. `createSocket` is intentionally left unmodified to prevent userland regressions. Test case included. Fixes: nodejs#6687
@timotheeg Thank you for reporting this. The issue has been fixed in Node.js master branch and is likely to be included in the future releases. |
If calling `https.request()` with `options.headers.host` defined and `options.servername` undefined, `https.Agent.createSocket` mutates connection `options` after `https.Agent.addRequest` has created empty socket pool array with mismatching connection name. This results in two socket pool arrays being created and only the last one gets eventually deleted by `removeSocket` - causing a memory leak. This commit fixes the leak by making sure that `addRequest` does the same modifications to `options` object as the `createSocket`. `createSocket` is intentionally left unmodified to prevent userland regressions. Test case included. PR-URL: #8647 Fixes: #6687 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jackson Tian <shvyo1987@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
If calling `https.request()` with `options.headers.host` defined and `options.servername` undefined, `https.Agent.createSocket` mutates connection `options` after `https.Agent.addRequest` has created empty socket pool array with mismatching connection name. This results in two socket pool arrays being created and only the last one gets eventually deleted by `removeSocket` - causing a memory leak. This commit fixes the leak by making sure that `addRequest` does the same modifications to `options` object as the `createSocket`. `createSocket` is intentionally left unmodified to prevent userland regressions. Test case included. PR-URL: #8647 Fixes: #6687 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jackson Tian <shvyo1987@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
The PR #4389 introduced
servername
in the computation of the agent name.If the caller of
https.request()
does not supplyservername
, _http_agent may introduce it in its methodcreateSocket()
if the headerhost
is supplied. See here.That introduced an inconsistency issue in the method
addRequest()
, becausegetName()
is called beforecreateSocket()
is called, which means on first call, it will not containservername
, while it will on subsequent calls, in particular when handling destruction.getName()
:node/lib/_http_agent.js
Line 121 in 572e28e
createSocket()
in the same method:node/lib/_http_agent.js
Line 144 in 572e28e
That created an issue for us. We are a high traffic site, and node was leaving a lot of sockets in TIME_WAIT state behind.
Expected
agent name computation and management in
sockets
andfreeSockets
should be consistent.App-level workaround:
Always supply
servername
in the options ofhttps.request()
(Apologies, I haven't come up with a minimal test to highlight the problem, I figure it was more important to report the problem first, and I'll try to come up with a test after that.)
The text was updated successfully, but these errors were encountered: