From 4f5ac7fa84ec7ebb0d2b096b324cc5d3cdb1ebc0 Mon Sep 17 00:00:00 2001 From: vdeturckheim Date: Thu, 30 Nov 2017 16:40:05 +0100 Subject: [PATCH 1/3] events: move domain handling from events to domain PR-URL: https://github.com/nodejs/node/pull/17403 Reviewed-By: Anna Henningsen Reviewed-By: Benjamin Gruenbaum Reviewed-By: Andreas Madsen Reviewed-By: Timothy Gu Reviewed-By: Anatoli Papirovski --- lib/domain.js | 51 ++++++++++++++++++++++++++++++++++++++++++++++----- lib/events.js | 48 ++++++------------------------------------------ 2 files changed, 52 insertions(+), 47 deletions(-) diff --git a/lib/domain.js b/lib/domain.js index 6c85ca2b17277b..cef09dca1c8857 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -31,11 +31,6 @@ 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]; @@ -387,3 +382,49 @@ 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; + if (domain === null || domain === undefined || this === process) { + return eventEmit.apply(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; + } + + domain.enter(); + try { + return eventEmit.apply(this, args); + } catch (er) { + if (typeof er === 'object' && er !== null) { + er.domainEmitter = this; + er.domain = domain; + er.domainThrown = false; + } + domain.emit('error', er); + return false; + } finally { + domain.exit(); + } +}; diff --git a/lib/events.js b/lib/events.js index d8c542a01264ef..ed2c9343d9b445 100644 --- a/lib/events.js +++ b/lib/events.js @@ -21,7 +21,6 @@ 'use strict'; -var domain; var spliceOne; function EventEmitter() { @@ -32,9 +31,6 @@ module.exports = EventEmitter; // Backwards-compat with node 0.10.x EventEmitter.EventEmitter = EventEmitter; -EventEmitter.usingDomains = false; - -EventEmitter.prototype.domain = undefined; EventEmitter.prototype._events = undefined; EventEmitter.prototype._eventsCount = 0; EventEmitter.prototype._maxListeners = undefined; @@ -67,14 +63,6 @@ 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) { @@ -115,34 +103,19 @@ 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 (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) { + 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; } - return false; + // 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; } const handler = events[type]; @@ -150,12 +123,6 @@ EventEmitter.prototype.emit = function emit(type, ...args) { if (handler === undefined) return false; - let needDomainExit = false; - if (domain !== null && domain !== undefined && this !== process) { - domain.enter(); - needDomainExit = true; - } - if (typeof handler === 'function') { handler.apply(this, args); } else { @@ -165,9 +132,6 @@ EventEmitter.prototype.emit = function emit(type, ...args) { listeners[i].apply(this, args); } - if (needDomainExit) - domain.exit(); - return true; }; From a3417fed424935480e9cc743cf55e139dc62f9f7 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 4 Dec 2017 11:45:15 -0500 Subject: [PATCH 2/3] events: use Reflect.apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of callback bound apply, instead use the standard Reflect.apply. This is both safer and appears to offer a slight performance benefit. PR-URL: https://github.com/nodejs/node/pull/17456 Refs: https://github.com/nodejs/node/issues/12956 Reviewed-By: Timothy Gu Reviewed-By: Colin Ihrig Reviewed-By: Michaël Zasso Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- lib/domain.js | 4 ++-- lib/events.js | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/domain.js b/lib/domain.js index cef09dca1c8857..b68e642c490cc3 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -400,7 +400,7 @@ const eventEmit = EventEmitter.prototype.emit; EventEmitter.prototype.emit = function emit(...args) { const domain = this.domain; if (domain === null || domain === undefined || this === process) { - return eventEmit.apply(this, args); + return Reflect.apply(eventEmit, this, args); } const type = args[0]; @@ -415,7 +415,7 @@ EventEmitter.prototype.emit = function emit(...args) { domain.enter(); try { - return eventEmit.apply(this, args); + return Reflect.apply(eventEmit, this, args); } catch (er) { if (typeof er === 'object' && er !== null) { er.domainEmitter = this; diff --git a/lib/events.js b/lib/events.js index ed2c9343d9b445..b9149d2b9b51a7 100644 --- a/lib/events.js +++ b/lib/events.js @@ -124,12 +124,12 @@ EventEmitter.prototype.emit = function emit(type, ...args) { return false; if (typeof handler === 'function') { - handler.apply(this, args); + Reflect.apply(handler, this, args); } else { const len = handler.length; const listeners = arrayClone(handler, len); for (var i = 0; i < len; ++i) - listeners[i].apply(this, args); + Reflect.apply(listeners[i], this, args); } return true; @@ -216,7 +216,7 @@ function onceWrapper(...args) { if (!this.fired) { this.target.removeListener(this.type, this.wrapFn); this.fired = true; - this.listener.apply(this.target, args); + Reflect.apply(this.listener, this.target, args); } } From 9b987428c08f589dec7edc8a8c1aa005b83ebf92 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sun, 10 Dec 2017 13:11:18 -0500 Subject: [PATCH 3/3] 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. PR-URL: https://github.com/nodejs/node/pull/17588 Refs: https://github.com/nodejs/node/pull/17403 Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- lib/domain.js | 37 ++++++++++--------- .../parallel/test-domain-ee-error-listener.js | 20 ++++++++++ 2 files changed, 40 insertions(+), 17 deletions(-) create mode 100644 test/parallel/test-domain-ee-error-listener.js diff --git a/lib/domain.js b/lib/domain.js index b68e642c490cc3..9670a53629e16b 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -399,32 +399,35 @@ 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 shouldEmitError = type === 'error' && + this.listenerCount(type) > 0; - domain.enter(); - try { + // 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); - } catch (er) { - if (typeof er === 'object' && er !== null) { + } + + 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; - } finally { - domain.exit(); } + + domain.enter(); + const ret = Reflect.apply(eventEmit, this, args); + domain.exit(); + + return ret; }; diff --git a/test/parallel/test-domain-ee-error-listener.js b/test/parallel/test-domain-ee-error-listener.js new file mode 100644 index 00000000000000..69041c7523b142 --- /dev/null +++ b/test/parallel/test-domain-ee-error-listener.js @@ -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);