Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
events: _events to object and undefined not null
Browse files Browse the repository at this point in the history
By making sure the _events is always an object there is one less check
that needs to be performed by emit.

Use undefined instead of null. typeof checks are a lot faster than
isArray.

There are a few places where the this._events check cannot be removed
because it is possible for the user to call those methods after using
utils.extend to create their own EventEmitter, but before it has
actually been instantiated.
  • Loading branch information
trevnorris authored and isaacs committed Mar 2, 2013
1 parent b3ea844 commit 4f7f8bb
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 18 deletions.
29 changes: 14 additions & 15 deletions lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

var isArray = Array.isArray;
var domain;

exports.usingDomains = false;
Expand All @@ -33,7 +32,7 @@ function EventEmitter() {
this.domain = domain.active;
}
}
this._events = this._events || null;
this._events = this._events || {};
this._maxListeners = this._maxListeners || defaultMaxListeners;
}
exports.EventEmitter = EventEmitter;
Expand All @@ -57,8 +56,9 @@ var PROCESS;
EventEmitter.prototype.emit = function(type) {
// If there is no 'error' event listener then throw.
if (type === 'error') {
if (!this._events || !this._events.error ||
(isArray(this._events.error) && !this._events.error.length)) {
if (!this._events.error ||
(typeof this._events.error === 'object' &&
!this._events.error.length)) {
if (this.domain) {
var er = arguments[1];
er.domainEmitter = this;
Expand All @@ -77,7 +77,6 @@ EventEmitter.prototype.emit = function(type) {
}
}

if (!this._events) return false;

This comment has been minimized.

Copy link
@wanderview

wanderview Mar 2, 2013

Suggest adding this line back.

var handler = this._events[type];
if (!handler) return false;

Expand Down Expand Up @@ -111,7 +110,7 @@ EventEmitter.prototype.emit = function(type) {
}
return true;

} else if (isArray(handler)) {
} else if (typeof handler === 'object') {
if (this.domain) {
PROCESS = PROCESS || process;
if (this !== PROCESS) {
Expand Down Expand Up @@ -153,7 +152,7 @@ EventEmitter.prototype.addListener = function(type, listener) {
if (!this._events[type]) {
// Optimize the case of one listener. Don't need the extra array object.
this._events[type] = listener;
} else if (isArray(this._events[type])) {
} else if (typeof this._events[type] === 'object') {

// If we've already got an array, just append.
this._events[type].push(listener);
Expand All @@ -165,7 +164,7 @@ EventEmitter.prototype.addListener = function(type, listener) {
}

// Check for listener leak
if (isArray(this._events[type]) && !this._events[type].warned) {
if (typeof this._events[type] === 'object' && !this._events[type].warned) {
var m;
m = this._maxListeners;

Expand Down Expand Up @@ -219,11 +218,11 @@ EventEmitter.prototype.removeListener = function(type, listener) {

if (list === listener ||
(typeof list.listener === 'function' && list.listener === listener)) {
this._events[type] = null;
this._events[type] = undefined;
if (this._events.removeListener)
this.emit('removeListener', type, listener);

} else if (isArray(list)) {
} else if (typeof list === 'object') {
for (i = 0; i < length; i++) {
if (list[i] === listener ||
(list[i].listener && list[i].listener === listener)) {
Expand All @@ -237,7 +236,7 @@ EventEmitter.prototype.removeListener = function(type, listener) {

if (list.length === 1) {
list.length = 0;
this._events[type] = null;
this._events[type] = undefined;
} else {
list.splice(position, 1);
}
Expand All @@ -261,9 +260,9 @@ EventEmitter.prototype.removeAllListeners = function(type) {
// not listening for removeListener, no need to emit
if (!this._events.removeListener) {
if (arguments.length === 0)
this._events = null;
this._events = {};
else if (this._events[type])
this._events[type] = null;
this._events[type] = undefined;
return this;
}

Expand All @@ -274,7 +273,7 @@ EventEmitter.prototype.removeAllListeners = function(type) {
this.removeAllListeners(key);
}
this.removeAllListeners('removeListener');
this._events = null;
this._events = {};
return this;
}

Expand All @@ -287,7 +286,7 @@ EventEmitter.prototype.removeAllListeners = function(type) {
while (listeners.length)
this.removeListener(type, listeners[listeners.length - 1]);
}
this._events[type] = null;
this._events[type] = undefined;

return this;
};
Expand Down
3 changes: 3 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2477,6 +2477,9 @@ Handle<Object> SetupProcessObject(int argc, char *argv[]) {
3);
process->Set(String::NewSymbol("_tickInfoBox"), info_box);

// pre-set _events object for faster emit checks
process->Set(String::NewSymbol("_events"), Object::New());

return process;
}

Expand Down
2 changes: 1 addition & 1 deletion test/simple/test-event-emitter-listeners-side-effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ var fl; // foo listeners
fl = e.listeners('foo');
assert(Array.isArray(fl));
assert(fl.length === 0);
assert.equal(e._events, null);
assert.deepEqual(e._events, {});

e.on('foo', assert.fail);
fl = e.listeners('foo');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ var events = require('events');

var e = new events.EventEmitter;

assert.strictEqual(e._events, null);
assert.deepEqual(e._events, {});
e.setMaxListeners(5);
assert.strictEqual(e._events, null);
assert.deepEqual(e._events, {});

4 comments on commit 4f7f8bb

@wanderview
Copy link

Choose a reason for hiding this comment

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

This commit triggers an error in the current nodeunit module:

events.js:80
  var handler = this._events[type];
                            ^
TypeError: Cannot read property 'set' of undefined
    at Results.EventEmitter.emit (events.js:80:29)
    at Results.addSet (/Users/bkelly/Dropbox/devel/test/node_modules/nodeunit/node_modules/tap/lib/tap-results.js:38:8)
    at new Results (/Users/bkelly/Dropbox/devel/test/node_modules/nodeunit/node_modules/tap/lib/tap-results.js:13:8)
    at GlobalHarness.Harness (/Users/bkelly/Dropbox/devel/test/node_modules/nodeunit/node_modules/tap/lib/tap-harness.js:31:18)
    at new GlobalHarness (/Users/bkelly/Dropbox/devel/test/node_modules/nodeunit/node_modules/tap/lib/tap-global-harness.js:20:23)
    at Object.<anonymous> (/Users/bkelly/Dropbox/devel/test/node_modules/nodeunit/node_modules/tap/lib/main.js:8:28)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)

To reproduce all you have to do is:

[xykon-2:~] bkelly% mkdir hmm
[xykon-2:~] bkelly% cd hmm
[xykon-2:~/hmm] bkelly% npm install nodeunit
[xykon-2:~/hmm] bkelly% ./node_modules/nodeunit/bin/nodeunit foo

It doesn't seem to matter if you run nodeunit on a real set of tests or not, the error is the same.

@wanderview
Copy link

Choose a reason for hiding this comment

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

It appears this is occurring because tap Results constructor is not chaining to its parent EventEmitter constructor.

This is probably a bug in Results, but could inflict widespread damage in many modules. Might want to consider lazy initializing since EventEmitter has been around for a while without requiring its constructor called.

@wanderview
Copy link

Choose a reason for hiding this comment

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

Sorry, spoke too soon. It does still lazy initialize, but deleting line 80 in events.js causes it to trigger this error if no one has called addListener() or on() yet.

@wanderview
Copy link

Choose a reason for hiding this comment

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

See #4891

Please sign in to comment.