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]);
-returnfalse;
-}
+constdoError=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'){
+conster=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){consterrors=lazyErrors();er=newerrors.Error('ERR_UNHANDLED_ERROR');}if(typeofer==='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

+conster=args.length>1&&args[1] ?
+args[1] : newerrors.Error('ERR_UNHANDLED_ERROR');
+
 if(typeofer==='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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 d46e020Compare 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

Uh oh!

domain: fix error emit handling#17588

domain: fix error emit handling #17588

Uh oh!

Conversation

apapirovski commented Dec 10, 2017

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski commented Dec 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndreasMadsen commented Dec 10, 2017

Uh oh!

apapirovski commented Dec 10, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndreasMadsen commented Dec 10, 2017

Uh oh!

apapirovski commented Dec 10, 2017

Uh oh!

AndreasMadsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovskiDec 11, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovskiDec 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeARDec 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovskiDec 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Dec 13, 2017

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

apapirovski commented Dec 10, 2017•
edited
Loading

apapirovski commented Dec 10, 2017•
edited
Loading

apapirovskiDec 11, 2017•
edited
Loading

apapirovskiDec 12, 2017•
edited
Loading

BridgeARDec 12, 2017•
edited
Loading

apapirovskiDec 12, 2017•
edited
Loading