async_hooks,http: fix socket reuse with Agent #13348

addaleax · 2017-05-31T21:20:27Z

Under very specific circumstances the http implementation could be brought to crash, because the Agent did not re-assign the async id field properly after setting up a socket for reuse.

Fixes: #13325

/cc @nodejs/async_hooks and @Trott because this is a somewhat complex test file

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http, async_hooks

Under very specific circumstances the `http` implementation could be brought to crash, because the Agent did not re-assign the async id field properly after setting up a socket for reuse. Fixes: nodejs#13325

addaleax · 2017-05-31T21:21:37Z

test/parallel/test-async-hooks-http-agent.js

+          }
+        }, common.mustCall((res) => {
+          const asyncId = res.socket[async_id_symbol];
+          assert.ok(asyncId > 0, `${asyncId} > 0`);


Commenting out this assertion will give you back the hard crash that we were seeing in the issue.

Commenting out and removing the fix?

refack · 2017-05-31T21:23:59Z

lib/_http_agent.js

@@ -168,6 +168,7 @@ Agent.prototype.addRequest = function addRequest(req, options, port/*legacy*/,
    var socket = this.freeSockets[name].shift();
    // Assign the handle a new asyncId and run any init() hooks.
    socket._handle.asyncReset();
+    socket[async_id_symbol] = socket._handle.getAsyncId();


👍 This is what I was referring to
"Race" was an inexact word

I don't know why but my intuition was to do this somewhere in a Socket method.
Maybe move both lines to a Socket method so other Agents could use it.
Something like Socket.prototype._prepareForReuse() or _recycle

Hmm … doing it this way doesn’t introduce any new methods that are accessible to the outside world. I am okay with moving it, but that should be based on more consideration than just fixing this bug; as you pointed out in the issue, we would want to consider what kind of API, if any, custom Agent implementations need.

So: I’m not disagreeing, but I think it’s better to postpone that after landing this.

edit: If we do this, we should probably also move the other part (marking the socket as free for reuse).

For the moment I'd say this is the correct way to handle the fix, and was an oversight on my part.

@refack

I don't know why but my intuition was to do this somewhere in a Socket method. Maybe move both lines to a Socket method so other Agents could use it. Something like Socket.prototype._prepareForReuse() or _recycle

Technically async_id_symbol was only added as a convenience to skip calling into native and retrieving AsyncWrap::async_id_, but in reality is unnecessary. The most sure way to handle this would be to remove all uses of async_id_symbol where there is a 1:1 pairing of native resource to JS resource and always call getAsyncId().

@addaleax

we would want to consider what kind of API, if any, custom Agent implementations need.

Node allows custom implementation of agents, timers and a few others. Which is the reason why getNewAsyncId() in lib/net.js checks if getAsyncId === 'function' and why insert() in lib/timers.js forcefully adds a new asyncId if none already exists. It has been a total pain during development, and the existing solution is the best I could come up with. Though if you have a better idea I would be happy to try it out.

addaleax · 2017-05-31T21:25:07Z

CI: https://ci.nodejs.org/job/node-test-commit/10258/

refack · 2017-05-31T21:31:41Z

test/parallel/test-async-hooks-http-agent.js

+    assert.strictEqual(r1.socket, socket);
+
+    res.on('data', common.mustCallAtLeast(() => {}));
+    res.on('end', common.mustCall(() => {


in 6bfdeed I used res.socket.once('free'
Might be simpler than on(end, setImmediate)

AndreasMadsen

Well done!

LGTM. I can confirm that this does fix a bug and that it corresponds to the ClientRequest._writeRaw (_http_outgoing.js:279:9) callsite from the stack trace provided in #13325 (comment)

The test looks sufficient, I haven't spend a lot of time reviewing it.

refack · 2017-05-31T22:47:35Z

Cross-ref: http, async_hooks: Consider async_hooks implications of the API that HTTP Agent uses

Trott · 2017-05-31T23:20:18Z

test/parallel/test-async-hooks-http-agent.js

+    res.on('data', common.mustCallAtLeast(() => {}));
+    res.on('end', common.mustCall(() => {
+      // setImmediate() to give the agent time to register the freed socket.
+      setImmediate(common.mustCall(() => {


Probably can leave out common.mustCall() on a setImmediate() but no harm either I suppose.

While this isn't an issue, and most likely will never be, there is actually a case that could cause some confusion:

require('async_hooks').createHook({ destroy() { process.exit() }}).enable(); process.on('exit', () => process._rawDebug('exiting...')); require('net').createServer().listen(0).close(); setImmediate(() => process._rawDebug('setImmediate'));

But that's just for reference. You're most likely correct that's it's unnecessary. But never hurts either. :)

Trott · 2017-05-31T23:20:44Z

/cc @nodejs/testing for reviews of the test file

addaleax · 2017-06-03T20:40:08Z

Landed in 3e02636

Under very specific circumstances the `http` implementation could be brought to crash, because the Agent did not re-assign the async id field properly after setting up a socket for reuse. Fixes: #13325 PR-URL: #13348 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

async_hooks,http: fix socket reuse with Agent

4d2eb5d

Under very specific circumstances the `http` implementation could be brought to crash, because the Agent did not re-assign the async id field properly after setting up a socket for reuse. Fixes: nodejs#13325

addaleax added async_hooks Issues and PRs related to the async hooks subsystem. http Issues or PRs related to the http subsystem. labels May 31, 2017

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label May 31, 2017

addaleax mentioned this pull request May 31, 2017

Strange exit after updated to node 8.0.0 #13325

Closed

addaleax commented May 31, 2017

View reviewed changes

refack approved these changes May 31, 2017

View reviewed changes

refack reviewed May 31, 2017

View reviewed changes

AndreasMadsen approved these changes May 31, 2017

View reviewed changes

Trott reviewed May 31, 2017

View reviewed changes

jasnell approved these changes Jun 1, 2017

View reviewed changes

japrescott mentioned this pull request Jun 3, 2017

passing socket from master to worker causes a timeout issue where socket is null. #13435

Closed

addaleax closed this Jun 3, 2017

addaleax deleted the fix-13325 branch June 3, 2017 20:40

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

bnoordhuis mentioned this pull request Jun 8, 2017

A C++ error with no always appear #13540

Closed

addaleax mentioned this pull request Jun 8, 2017

http request with a keep-alive agent leads to an assertion failed error #13545

Closed

refack mentioned this pull request Jun 8, 2017

Haraka test suite crashes with #13548

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

refack mentioned this pull request Jul 2, 2017

Cannot read property 'asyncReset' of null #13539

Closed

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks,http: fix socket reuse with Agent #13348

async_hooks,http: fix socket reuse with Agent #13348

addaleax commented May 31, 2017

addaleax May 31, 2017

refack May 31, 2017

addaleax May 31, 2017

refack May 31, 2017

refack May 31, 2017 •

edited

addaleax May 31, 2017 •

edited

refack May 31, 2017

trevnorris Jun 2, 2017

addaleax commented May 31, 2017

refack May 31, 2017

AndreasMadsen left a comment •

edited

refack commented May 31, 2017 •

edited

Trott May 31, 2017

trevnorris Jun 2, 2017

Trott commented May 31, 2017

addaleax commented Jun 3, 2017

async_hooks,http: fix socket reuse with Agent #13348

async_hooks,http: fix socket reuse with Agent #13348

Conversation

addaleax commented May 31, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack May 31, 2017 • edited

Choose a reason for hiding this comment

addaleax May 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented May 31, 2017

Choose a reason for hiding this comment

AndreasMadsen left a comment • edited

Choose a reason for hiding this comment

refack commented May 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented May 31, 2017

addaleax commented Jun 3, 2017

refack May 31, 2017 •

edited

addaleax May 31, 2017 •

edited

AndreasMadsen left a comment •

edited

refack commented May 31, 2017 •

edited