http: set async_id_symbol to any socket set #14389

mcollina · 2017-07-20T11:38:20Z

OutgoingMessage needs a async-hooks enabled socket to work, but
we support also basic streams. This PR init the async-hooks bits
for the passed stream if it is needed.

Fixes: #14386
Fixes: #14381

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

mcollina · 2017-07-20T11:38:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9266/

mcollina · 2017-07-20T11:45:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9267/

refack · 2017-07-20T11:52:11Z

lib/_http_outgoing.js

+  set: function(socket) {
+    this[kSocket] = socket;
+    if (socket && socket[async_id_symbol] === undefined) {
+      socket[async_id_symbol] = async_hooks.newUid();


So as we wrote in #14386 (comment)
This might not be enough. If the socket is being reused, then it might hold a socket[async_id_symbol] but it might be stale.
[I'm not completely awake yet, but] you might want to test socket[async_id_symbol] != socket._handle.getAsyncId()

I think that is a completely different problem, this is built to be a stop-gap fixes that avoid crashing node 8 everywhere during unit tests.
The line below sets the type to 'not a socket', which is probably not correct in that case.

This is an issue I've been wresting with for the last month while I was working on #14208. The problem with assigning an async_id to the OutgoingMessage is because OutgoingMessage isn't actually asynchronous in any way, so it will never emit a before()/after()/destroy(). So we're only assigning an async_id as a stop gap.

There are two long term solutions that I could implement quickly:

Assign the this[async_id_symbol] = newUid() in the OutgoingMessage constructor. Then use allow passing custom async_id to node::AsyncWrap::AsyncWrap #14208 to propagate that value to the socket when it's assigned. This will require detecting if the socket will call the destroy() callback, or if it needs to be queued to be called in the 'finish' event. For this to happen I'll need sign-off on said PR.

I've also considered and experimented with assigning error codes to the async_id_symbol/trigger_id_symbol that can be safely handled. In this case I'd assign UV_ENODATA in the constructor since the socket hasn't been created yet, then the async_id_symbol can be assigned when the socket has been created. This has the drawback of breaking async call graphs, but is easier to implement and doesn't require the additional PR.

I'm going to step out and clear my head. Give me feedback and I can get a fix posted.

@trevnorris I really like 1, and I do not really understand 2. I would prefer releasing something asap as a stop-gap solution (maybe today or tomorrow), and work on a proper fix. #14208 seems a semver-minor change, but maybe I am misreading it.

Do you think that will fix #14368 as well?

👍 on (2)
IMHO if we can placate all the assertion while async_hooks are disabled that would be best.

@refack I'll start working on (2). It's a quicker solution and should address a broader scope of issues.

@mcollina (1) is a more long term solution for more specific cases and will probably require more testing to make sure it's done correctly. I'll get on that once the referenced PR lands. (2) is a way to mitigate these errors and allow bad values to flow through without crashing the process while allowing users of async_hooks to know when bad values appear

addaleax

LGTM, I agree this can only be a stop-gap measure, but if it fixes the problems, fine by me

addaleax · 2017-07-20T13:51:34Z

test/parallel/test-http-outgoing-message-inheritance.js

+const { Writable } = require('stream');
+const assert = require('assert');
+
+// check that OutgoingMessage can be used without a proper Socket


Can you link to the issues in the test files? I think that would be helpful for people looking at these tests at a later point.

OutgoingMessage needs a async-hooks enabled socket to work, but we support also basic streams. This PR init the async-hooks bits for the passed stream if it is needed. PR-URL: nodejs#14389 Fixes: nodejs#14386 Fixes: nodejs#14381

Fishrock123 · 2017-07-20T15:02:04Z

Are we looking for a 8.2.1 today if this lands?

mcollina · 2017-07-20T15:05:44Z

@Fishrock123 considering that Hapi.js applications could not upgrade to Node 8.2 from 8.1, I would recommend either today/tomorrow/early next week.

refack · 2017-07-20T15:08:12Z

Are we looking for a 8.2.1 today if this lands?

I'm not sure, AFAICT shot (the root cause #14386) is a test mocking tool, so the issue is in an edge-case, not regular production flow.

refack · 2017-07-20T15:11:40Z

@Fishrock123 considering that Hapi.js applications could not upgrade to Node 8.2 from 8.1, I would recommend either today/tomorrow/early next week.

Even so, I think waiting for @trevnorris's PR will be prudent.

P.S. @mcollina regarding Hapi.js is running broken or only testing?

mcollina · 2017-07-20T15:18:27Z

@refack all CI systems are red with Node.js 8.2, including a lot of OSS modules. As they are red, no one will use 8.2 as of now.

I stress the fact that a) we should not have released 8.2 if we knew this was happening or b) we should release a fix asap.

I'm ok in reverting whatever commit caused this, but the best course of action seems a stop-gap like this one, as it is affecting only testing.

trevnorris · 2017-07-20T15:20:15Z

@addaleax Unless we plan on rolling a release in the next 8 hours, let's hold off on landing this right now. I proposed a solution that should address all the issues about crashes from async_hooks, and it's one I'm familiar with so writing it shouldn't take longer than today.

jasnell · 2017-07-20T15:37:14Z

Looking at this I think our options are quite limited... I'd much rather have a temporary fix that goes out in a release later today, followed by a better and more permanent fix in a proper release in the next week or two than delay getting a fix out at all. That would likely mean landing either or both this PR or #14387 now, then doing a partial revert (keeping the test) once @trevnorris' other fix is complete.

One thing I want to try to avoid is doing a Friday release, generally because those are horrible overall. This looks like a critical enough bug that we should prioritize getting a fix out today.

trevnorris · 2017-07-20T15:39:00Z

@jasnell is there not a chance the release can wait 8 hours?

jasnell · 2017-07-20T15:40:57Z

8 hours yeah, that's still Thursday (here in Fresno at least ;-) ...) I just really want to avoid having to do a release tomorrow.

refack · 2017-07-20T15:42:40Z

@refack all CI systems are red with Node.js 8.2, including a lot of OSS modules. As they are red, no one will use 8.2 as of now.

Was just asking.

I stress the fact that a) we should not have released 8.2 if we knew this was happening or b) we should release a fix asap.

We did not know. Our CI and CitGM were green.
@trevnorris's fix will close this hole, and provide even wider protection.

mcollina · 2017-07-20T15:48:02Z

How about you make a decision later today based on the status of the @trevnorris proposed fix? If we can get a release lined up for your end of the day, it's ok for me.

mcollina · 2017-07-20T22:58:24Z

Closing, as #14387 got released in 8.2.1

mcollina requested review from refack, trevnorris and AndreasMadsen July 20, 2017 11:38

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

mcollina force-pushed the fix-14386-bis branch from e5b6841 to f634ef0 Compare July 20, 2017 11:40

AndreasMadsen requested a review from addaleax July 20, 2017 11:41

refack reviewed Jul 20, 2017

View reviewed changes

addaleax approved these changes Jul 20, 2017

View reviewed changes

http: set async_id_symbol to any socket set

9977354

OutgoingMessage needs a async-hooks enabled socket to work, but we support also basic streams. This PR init the async-hooks bits for the passed stream if it is needed. PR-URL: nodejs#14389 Fixes: nodejs#14386 Fixes: nodejs#14381

mcollina force-pushed the fix-14386-bis branch from f634ef0 to 9977354 Compare July 20, 2017 14:07

mcollina added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 20, 2017

trevnorris mentioned this pull request Jul 20, 2017

async_hooks: triggerAsyncId can be undefined #14387

Closed

3 tasks

mcollina closed this Jul 20, 2017

mcollina deleted the fix-14386-bis branch July 20, 2017 22:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: set async_id_symbol to any socket set #14389

http: set async_id_symbol to any socket set #14389

mcollina commented Jul 20, 2017 •

edited

Loading

mcollina commented Jul 20, 2017

mcollina commented Jul 20, 2017

refack Jul 20, 2017 •

edited

Loading

mcollina Jul 20, 2017

trevnorris Jul 20, 2017

mcollina Jul 20, 2017

refack Jul 20, 2017

trevnorris Jul 20, 2017

addaleax left a comment

addaleax Jul 20, 2017

mcollina Jul 20, 2017

Fishrock123 commented Jul 20, 2017

mcollina commented Jul 20, 2017

refack commented Jul 20, 2017 •

edited

Loading

refack commented Jul 20, 2017

mcollina commented Jul 20, 2017

trevnorris commented Jul 20, 2017

jasnell commented Jul 20, 2017

trevnorris commented Jul 20, 2017

jasnell commented Jul 20, 2017

refack commented Jul 20, 2017

mcollina commented Jul 20, 2017

mcollina commented Jul 20, 2017 •

edited

Loading

http: set async_id_symbol to any socket set #14389

http: set async_id_symbol to any socket set #14389

Conversation

mcollina commented Jul 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mcollina commented Jul 20, 2017

mcollina commented Jul 20, 2017

refack Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

mcollina Jul 20, 2017

Choose a reason for hiding this comment

trevnorris Jul 20, 2017

Choose a reason for hiding this comment

mcollina Jul 20, 2017

Choose a reason for hiding this comment

refack Jul 20, 2017

Choose a reason for hiding this comment

trevnorris Jul 20, 2017

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

addaleax Jul 20, 2017

Choose a reason for hiding this comment

mcollina Jul 20, 2017

Choose a reason for hiding this comment

Fishrock123 commented Jul 20, 2017

mcollina commented Jul 20, 2017

refack commented Jul 20, 2017 • edited Loading

refack commented Jul 20, 2017

mcollina commented Jul 20, 2017

trevnorris commented Jul 20, 2017

jasnell commented Jul 20, 2017

trevnorris commented Jul 20, 2017

jasnell commented Jul 20, 2017

refack commented Jul 20, 2017

mcollina commented Jul 20, 2017

mcollina commented Jul 20, 2017 • edited Loading

mcollina commented Jul 20, 2017 •

edited

Loading

refack Jul 20, 2017 •

edited

Loading

refack commented Jul 20, 2017 •

edited

Loading

mcollina commented Jul 20, 2017 •

edited

Loading