Skip to content

domain: fix error emit handling #17588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
apapirovski wants to merge 4 commits into from
Closed

domain: fix error emit handling #17588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e4e4e0a
domain: fix error emit handling
apapirovski Dec 10, 2017
d46e020
fixup: use listenerCount instead of _events
apapirovski Dec 10, 2017
f8361f7
fixup: rename doError variable
apapirovski Dec 11, 2017
f401c32
fixup: remove unnecessary null check
apapirovski Dec 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
domain: fix error emit handling 
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.
  • Loading branch information
@apapirovski
apapirovski committed Dec 10, 2017
commit e4e4e0ac5b2d65fad8f5298caaa2154ca6a31ffe
33 changes: 17 additions & 16 deletions lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,32 +399,33 @@ EventEmitter.init = function() {
const eventEmit = EventEmitter.prototype.emit;
EventEmitter.prototype.emit = function emit(...args) {
const domain = this.domain;
if (domain === null || domain === undefined || this === process) {
return Reflect.apply(eventEmit, this, args);
}

const type = args[0];
// edge case: if there is a domain and an existing non error object is given,
// it should not be errorized
// see test/parallel/test-event-emitter-no-error-provided-to-error-event.js
if (type === 'error' && args.length > 1 && args[1] &&
!(args[1] instanceof Error)) {
domain.emit('error', args[1]);
return false;
}
const doError = type === 'error' &&
Copy link
Copy Markdown
Member

@AndreasMadsen AndreasMadsen Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@AndreasMadsen AndreasMadsen Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this._events !== undefined &&
this._events.error !== undefined;

domain.enter();
try {
if (doError || domain === null || domain === undefined || this === process) {
return Reflect.apply(eventEmit, this, args);
} catch (er) {
}

if (type === 'error') {
const er = args.length > 1 && args[1] ?
Copy link
Copy Markdown
Member

@AndreasMadsen AndreasMadsen Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Dec 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

args[1] : new errors.Error('ERR_UNHANDLED_ERROR');

if (typeof er === 'object' && er !== null) {
Copy link
Copy Markdown
Member

@BridgeAR BridgeAR Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@BridgeAR BridgeAR Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@BridgeAR BridgeAR Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@apapirovski apapirovski Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

er.domainEmitter = this;
er.domain = domain;
er.domainThrown = false;
}

domain.emit('error', er);
return false;
} finally {
domain.exit();
}

domain.enter();
const ret = Reflect.apply(eventEmit, this, args);
domain.exit();

return ret;
};
20 changes: 20 additions & 0 deletions test/parallel/test-domain-ee-error-listener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain').create();
const EventEmitter = require('events');

domain.on('error', common.mustNotCall());

const ee = new EventEmitter();

const plainObject = { justAn: 'object' };
ee.once('error', common.mustCall((err) => {
assert.deepStrictEqual(err, plainObject);
}));
ee.emit('error', plainObject);

const err = new Error('test error');
ee.once('error', common.expectsError(err));
ee.emit('error', err);