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

https.request(): Agent name inconsistency when servername not supplied #6687

Closed
timotheeg opened this issue May 11, 2016 · 4 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. https Issues or PRs related to the https subsystem.

Comments

@timotheeg
Copy link

timotheeg commented May 11, 2016

  • Version: v4.4.4
  • Platform: Linux XXXXXXXXXXX 3.13.0-62-generic url: improve parsing speed #102~precise1-Ubuntu SMP Wed Aug 12 14:09:54 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux

The PR #4389 introduced servername in the computation of the agent name.

If the caller of https.request() does not supply servername, _http_agent may introduce it in its method createSocket() if the header host is supplied. See here.

That introduced an inconsistency issue in the method addRequest(), because getName() is called before createSocket() is called, which means on first call, it will not contain servername, while it will on subsequent calls, in particular when handling destruction.

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 and freeSockets should be consistent.

App-level workaround:

Always supply servername in the options of https.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.)

@claudiorodriguez claudiorodriguez added the https Issues or PRs related to the https subsystem. label May 11, 2016
@bnoordhuis
Copy link
Member

Are you using a custom agent? The default HTTPS agent includes servername iff options.servername && options.servername !== options.host.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 5, 2016

@timotheeg ping

@timotheeg
Copy link
Author

timotheeg commented Jul 7, 2016

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 servername, things work for me, and I still haven't taken the time to supply a test case for the issue 😓 .

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 options.host as an ip but options.header.host as a domain. That caused servername and host to be out of sync based on this. So the second getName() call would have had ``options.servername && options.servername !== options.host` true, and compute the name inconsistently.

So to sum up: https.request() ends up calling agent.addRequest(), which calls getName(), if servername was not supplied in options, the test options.servername && options.servername !== options.host is false and servername is not taken into consideration. addRequest() calls createSocket() later, which then introduces servername. That snippet can cause options.servername and options.host to not be equal in a ip/host header setup, and causes the next call to getName() (that occurs immediately after) to use servername, and compute an agent name that is inconsistent with the previous one.

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 :/

imyller added a commit to imyller/node that referenced this issue Sep 18, 2016
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
@imyller imyller added the confirmed-bug Issues with confirmed bugs. label Sep 18, 2016
imyller added a commit to imyller/node that referenced this issue Sep 20, 2016
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
@imyller
Copy link
Member

imyller commented Sep 21, 2016

@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.

Fishrock123 pushed a commit that referenced this issue Oct 11, 2016
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>
MylesBorins pushed a commit that referenced this issue Nov 18, 2016
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants