domain: fix error emit handling #17588

apapirovski · 2017-12-10T18:13:26Z

There are a few errors in the logic of the new domain-specific emit and a bit too much fanciness with the falling through try-catch logic. Fixed it all up and created a regression test.

Refs: #17403

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)

domain, events, test

apapirovski · 2017-12-10T18:17:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12027/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1141/

Edit: Both appear to be green all around.

AndreasMadsen · 2017-12-10T22:38:25Z

@apapirovski Thanks. Could you elaborate on what the bugs are?

apapirovski · 2017-12-10T22:39:19Z

@AndreasMadsen see the added test case which fails with the old implementation. Basically, error isn't emitted on the original EventEmitter under any circumstances.

AndreasMadsen · 2017-12-10T22:43:50Z

@apapirovski Could you describe that in the commit message?

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block.

apapirovski · 2017-12-10T22:56:36Z

@AndreasMadsen Done.

AndreasMadsen

This is a good start. But let's try to stay closer to the original implementation.

AndreasMadsen · 2017-12-11T13:50:51Z

lib/domain.js

-    domain.emit('error', args[1]);
-    return false;
-  }
+  const doError = type === 'error' &&


This doError is opposite of the doError in the old events.js implementation. Logically it is not wrong but it makes it hard to compare the implementations.

That seems more like a naming issue. Maybe just name it shouldEmitError instead of doError?

I'm fine with a renaming. But you should make it clear if the emit is on the original EventEmitter or the domain instance.

AndreasMadsen · 2017-12-11T13:56:26Z

lib/domain.js

+  }
+
+  if (type === 'error') {
+    const er = args.length > 1 && args[1] ?


args[1] || new errors.Error('ERR_UNHANDLED_ERROR') should be enough. But I think it would be better to stay grammatically closer to the old implementation.

Something like:

er = args[1] if (!er) { const errors = lazyErrors(); er = new errors.Error('ERR_UNHANDLED_ERROR'); } if (typeof er === 'object' && er !== null) { er.domainEmitter = this; er.domain = domain; er.domainThrown = false; }

That triggers a slow path in V8. We need the if check or a ternary. This version does the same in less lines and we don't need lazyErrors, which is the main reason we didn't just use a ternary before.

BridgeAR

LGTM but it would be nice if my comment would be addressed (even though that code was not touched in this PR).

BridgeAR · 2017-12-12T20:53:01Z

lib/domain.js

+  if (type === 'error') {
+    const er = args.length > 1 && args[1] ?
+      args[1] : new errors.Error('ERR_UNHANDLED_ERROR');
+
    if (typeof er === 'object' && er !== null) {


er can not be null anymore. It is either truthy and in that case it is not null or it is an error and in that case it is also not null. I wonder if the object check is actually necessary either. Since er is always a truthy value, assigning properties to it will never fail (even though it would of course be weird to try to add properties to e.g. a number, but trying would not hurt either and the average case would be faster).

@BridgeAR Great job noticing that! Seems like we've had that pointless check in this code for a while. Guessing it was useful at some point but clearly left over and unnecessary now.

The only reason I can think of leaving the object check is in case someone is testing for those properties and assuming that they're dealing with an object afterwards... ? But that seems obscure enough that it shouldn't be a real issue.

That could be solved with a comment as well, right?

I just mean for the purposes of third-party code... but that seems like a pretty obscure thing to me. We can run CitGM to double-check.

Although, something to think about: typeof isn't very expensive in V8 these days and setting 3 props on numbers or strings is, I think, a lot more expensive. So from a perf perspective, it might be better to keep this check.

I am not sure what you mean that third-party code relies on that? Trying to set a property on a primitive (not function) is a noop. The outcome will be the same as it is right now.

About the performance implications - I am aware that a typeof is cheap but do we want to optimize for the average case or for obscure cases? Because passing in a non object as error sounds really weird to me...

Does it? I've definitely seen people use strings. It's tricky... we've allowed it till now so we're kind of stuck with supporting it. Anyway, if we're optimizing then it should be done in another PR. I don't think domain related code is that performance sensitive anyway... there are much worse offenders.

I'll remove the null check tho.

apapirovski · 2017-12-13T14:38:13Z

Landed in 04ae486

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block. PR-URL: #17588 Refs: #17403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-01-09T22:40:03Z

I'm setting the dont-land label for 9.x as #17403 didn't land.

Please feel free to backport

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block. PR-URL: nodejs#17588 Refs: nodejs#17403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block. Backport-PR-URL: #18487 PR-URL: #17588 Refs: #17403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

apapirovski added domain Issues and PRs related to the domain subsystem. events Issues and PRs related to the events subsystem / EventEmitter. test Issues and PRs related to the tests. labels Dec 10, 2017

apapirovski requested review from AndreasMadsen and addaleax December 10, 2017 18:13

nodejs-github-bot added the domain Issues and PRs related to the domain subsystem. label Dec 10, 2017

AndreasMadsen mentioned this pull request Dec 10, 2017

domain migration to async_hooks #17143

Open

7 tasks

apapirovski added 2 commits December 10, 2017 17:54

domain: fix error emit handling

e4e4e0a

Fix an issue where error is never emitted on the original EventEmitter in situations where a listener for error does exist. Refactor to eliminate unnecessary try/catch/finally block.

fixup: use listenerCount instead of _events

d46e020

apapirovski force-pushed the fix-domain-emit-error branch from e1037dd to d46e020 Compare December 10, 2017 22:56

AndreasMadsen reviewed Dec 11, 2017

View reviewed changes

fixup: rename doError variable

f8361f7

apapirovski mentioned this pull request Dec 12, 2017

streams: remove all references to readableState/writableState from docs #12855

Closed

4 tasks

BridgeAR approved these changes Dec 12, 2017

View reviewed changes

fixup: remove unnecessary null check

f401c32

jasnell approved these changes Dec 12, 2017

View reviewed changes

apapirovski closed this Dec 13, 2017

apapirovski deleted the fix-domain-emit-error branch December 13, 2017 14:38

MylesBorins added dont-land-on-v4.x labels Jan 9, 2018

apapirovski removed the dont-land-on-v9.x label Jan 31, 2018

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

MylesBorins added backported-to-v9.x and removed backport-requested-v9.x labels Feb 20, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

domain: fix error emit handling #17588

domain: fix error emit handling #17588

apapirovski commented Dec 10, 2017

apapirovski commented Dec 10, 2017 •

edited

Loading

AndreasMadsen commented Dec 10, 2017

apapirovski commented Dec 10, 2017 •

edited

Loading

AndreasMadsen commented Dec 10, 2017

apapirovski commented Dec 10, 2017

AndreasMadsen left a comment

AndreasMadsen Dec 11, 2017

apapirovski Dec 11, 2017

AndreasMadsen Dec 11, 2017

AndreasMadsen Dec 11, 2017

apapirovski Dec 11, 2017 •

edited

Loading

BridgeAR left a comment

BridgeAR Dec 12, 2017

apapirovski Dec 12, 2017

BridgeAR Dec 12, 2017

apapirovski Dec 12, 2017

apapirovski Dec 12, 2017 •

edited

Loading

BridgeAR Dec 12, 2017 •

edited

Loading

apapirovski Dec 12, 2017 •

edited

Loading

apapirovski commented Dec 13, 2017

MylesBorins commented Jan 9, 2018

domain: fix error emit handling #17588

domain: fix error emit handling #17588

Conversation

apapirovski commented Dec 10, 2017

Checklist

Affected core subsystem(s)

apapirovski commented Dec 10, 2017 • edited Loading

AndreasMadsen commented Dec 10, 2017

apapirovski commented Dec 10, 2017 • edited Loading

AndreasMadsen commented Dec 10, 2017

apapirovski commented Dec 10, 2017

AndreasMadsen left a comment

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

Choose a reason for hiding this comment

apapirovski Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

BridgeAR left a comment

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

Choose a reason for hiding this comment

apapirovski Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

BridgeAR Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski commented Dec 13, 2017

MylesBorins commented Jan 9, 2018

apapirovski commented Dec 10, 2017 •

edited

Loading

apapirovski commented Dec 10, 2017 •

edited

Loading

apapirovski Dec 11, 2017 •

edited

Loading

apapirovski Dec 12, 2017 •

edited

Loading

BridgeAR Dec 12, 2017 •

edited

Loading

apapirovski Dec 12, 2017 •

edited

Loading