Skip to content
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

Jump to bottom

Revert 9.x events changes #18942

Closed
wants to merge 3 commits into from
Closed

Revert 9.x events changes #18942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dc8893
Revert "domain: fix error emit handling"
MylesBorins Feb 22, 2018
22c7fe5
Revert "events: use Reflect.apply"
MylesBorins Feb 22, 2018
9bdd15d
Revert "events: move domain handling from events to domain"
MylesBorins Feb 22, 2018
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
54 changes: 5 additions & 49 deletions lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ const EventEmitter = require('events');
const errors = require('internal/errors');
const { createHook } = require('async_hooks');

// communicate with events module, but don't require that
// module to have to load this one, since this module has
// a few side effects.
EventEmitter.usingDomains = true;

// overwrite process.domain with a getter/setter that will allow for more
// effective optimizations
var _domain = [null];
Expand Down Expand Up @@ -392,52 +397,3 @@ Domain.prototype.bind = function(cb) {

return runBound;
};

// Override EventEmitter methods to make it domain-aware.
EventEmitter.usingDomains = true;

const eventInit = EventEmitter.init;
EventEmitter.init = function() {
this.domain = null;
if (exports.active && !(this instanceof exports.Domain)) {
this.domain = exports.active;
}

return eventInit.call(this);
};

const eventEmit = EventEmitter.prototype.emit;
EventEmitter.prototype.emit = function emit(...args) {
const domain = this.domain;

const type = args[0];
const shouldEmitError = type === 'error' &&
this.listenerCount(type) > 0;

// Just call original `emit` if current EE instance has `error`
// handler, there's no active domain or this is process
if (shouldEmitError || domain === null || domain === undefined ||
this === process) {
return Reflect.apply(eventEmit, this, args);
}

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

if (typeof er === 'object') {
er.domainEmitter = this;
er.domain = domain;
er.domainThrown = false;
}

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

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

return ret;
};
54 changes: 45 additions & 9 deletions lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

'use strict';

var domain;
var spliceOne;

function EventEmitter() {
Expand All @@ -31,6 +32,9 @@ module.exports = EventEmitter;
// Backwards-compat with node 0.10.x
EventEmitter.EventEmitter = EventEmitter;

EventEmitter.usingDomains = false;
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I’m basically suggesting only taking this line of the diff, and leaving the rest alone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to that as well, wasn't sure the context or why that line was removed. Should we add that line back to master instead?

Would doing another release next tuesday make sense then or should we get this out asap

Copy link
Member

Choose a reason for hiding this comment

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

@MylesBorins I guess it seems fair to remove the line because it would be false-y by default anyway, so explicitly setting it here and thereby leaking domains logic into this module seems undesirable.

Would doing another release next tuesday make sense then or should we get this out asap

It’s not really all that clear to me just how breaking this change is or how many people it would affect – but if we’re only adding this line back, I don’t see a reason not to do a release as soon as we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok opening a PR


EventEmitter.prototype.domain = undefined;
EventEmitter.prototype._events = undefined;
EventEmitter.prototype._eventsCount = 0;
EventEmitter.prototype._maxListeners = undefined;
Expand Down Expand Up @@ -61,6 +65,14 @@ Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
});

EventEmitter.init = function() {
this.domain = null;
if (EventEmitter.usingDomains) {
// if there is an active domain, then attach to it.
domain = domain || require('domain');
if (domain.active && !(this instanceof domain.Domain)) {
this.domain = domain.active;
}
}

if (this._events === undefined ||
this._events === Object.getPrototypeOf(this)._events) {
Expand Down Expand Up @@ -101,35 +113,59 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
else if (!doError)
return false;

const domain = this.domain;

// If there is no 'error' event listener then throw.
if (doError) {
let er;
if (args.length > 0)
er = args[0];
if (er instanceof Error) {
if (domain !== null && domain !== undefined) {
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;
}
domain.emit('error', er);
} else if (er instanceof Error) {
throw er; // Unhandled 'error' event
} else {
// At least give some kind of context to the user
const errors = lazyErrors();
const err = new errors.Error('ERR_UNHANDLED_ERROR', er);
err.context = er;
throw err;
}
// At least give some kind of context to the user
const errors = lazyErrors();
const err = new errors.Error('ERR_UNHANDLED_ERROR', er);
err.context = er;
throw err;
return false;
}

const handler = events[type];

if (handler === undefined)
return false;

let needDomainExit = false;
if (domain !== null && domain !== undefined && this !== process) {
domain.enter();
needDomainExit = true;
}

if (typeof handler === 'function') {
Reflect.apply(handler, this, args);
handler.apply(this, args);
} else {
const len = handler.length;
const listeners = arrayClone(handler, len);
for (var i = 0; i < len; ++i)
Reflect.apply(listeners[i], this, args);
listeners[i].apply(this, args);
}

if (needDomainExit)
domain.exit();

return true;
};

Expand Down Expand Up @@ -214,7 +250,7 @@ function onceWrapper(...args) {
if (!this.fired) {
this.target.removeListener(this.type, this.wrapFn);
this.fired = true;
Reflect.apply(this.listener, this.target, args);
this.listener.apply(this.target, args);
}
}

Expand Down
20 changes: 0 additions & 20 deletions test/parallel/test-domain-ee-error-listener.js
View file
Open in desktop

This file was deleted.