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

[timotheeg]

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

The PR https://github.com/nodejs/node/pull/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.

• Call to getName(): https://github.com/nodejs/node/blob/572e28efa2e3cde78b5e7984b7760fd4c270f619/lib/_http_agent.js#L121
• Call to createSocket() in the same method: https://github.com/nodejs/node/blob/572e28efa2e3cde78b5e7984b7760fd4c270f619/lib/_http_agent.js#L144

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

[bnoordhuis]

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

[cjihrig]

@timotheeg ping

[timotheeg]

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]

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