Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

events: Support prototype property name as an event type #4366

Closed
wants to merge 1 commit into from

6 participants

@tricknotes

The prototype property name (such as 'proto') as an event type does not work as my expectation.

> var events = require('events')
> var e = new events.EventEmitter();
> e.on('__proto__', function() {});
> e.emit('__proto__');
TypeError: Object #<Object> has no method 'apply'
    at EventEmitter.emit (events.js:126:20)
    at repl:1:3
    at REPLServer.self.eval (repl.js:109:21)
    at rli.on.self.bufferedCmd (repl.js:258:20)
    at REPLServer.self.eval (repl.js:116:5)
    at Interface.<anonymous> (repl.js:248:12)
    at Interface.EventEmitter.emit (events.js:96:17)
    at Interface._onLine (readline.js:200:10)
    at Interface._line (readline.js:518:8)
    at Interface._ttyWrite (readline.js:736:14)

To support these, use Object.create(null) instead of {}.

@tricknotes tricknotes events: Support prototype property name as an event type
The prototype name (such as '__proto__') as an event type
does not work as my expectation.

```
> var events = require('events')
> var e = new events.EventEmitter();
> e.on('__proto__', function() {});
> e.emit('__proto__');
TypeError: Object #<Object> has no method 'apply'
    at EventEmitter.emit (events.js:126:20)
    at repl:1:3
    at REPLServer.self.eval (repl.js:109:21)
    at rli.on.self.bufferedCmd (repl.js:258:20)
    at REPLServer.self.eval (repl.js:116:5)
    at Interface.<anonymous> (repl.js:248:12)
    at Interface.EventEmitter.emit (events.js:96:17)
    at Interface._onLine (readline.js:200:10)
    at Interface._line (readline.js:518:8)
    at Interface._ttyWrite (readline.js:736:14)
```

To support these, use `Object.create(null)` instead of `{}`.
440ce52
@bnoordhuis

While the PR in itself LGTM, I'm not sure if it's an acceptable change.

The event emitter code is considered a red hot code path and obj = Object.create(null) is 20-30 times slower than obj = {}.

@tricknotes

I see. It is true that I haven't look around about the performance.
So, what do you feel about the following code?:

obj = {};
obj.__proto__ = null;

This is not a cool code, but it is able you to use prototype property name as an event type without compromising the performance.

@bnoordhuis

Same issue as Object.create(null), it's about 20 times slower.

@tricknotes

Hmm, really?
In my local environment, obj.__proto__ = null has the same performance as obj = {}; on initialize.
And, faster on property lookup.

ref: https://gist.github.com/4214426

Please tell me another case that should be checked if it exist.

@AlexeyKupershtokh

There's a bug:

for (i = 0; i < count; i++)
  obj = {}; obj.__proto__ = null;
@tricknotes

Thanks @AlexeyKupershtokh.
That's right!

This code is really slower.

@AlexeyKupershtokh

lol :)
it's even faster to clone a created object using require('node-v8-clone').v8_clone:

clone(a);:
  16143 ms
{}:
  436 ms
Object.create(null):
  24133 ms
obj = {};obj.__proto__ = null:
  19603 ms
@isaacs
Owner

It's nice in theory, but in practice, we cannot do this for performance reasons. I'd be open to a doc patch explaining that __proto__ is not a valid event name.

@isaacs isaacs closed this
@isaacs
Owner

Another option worth trying would be to prefix all event names in the object with some character, like "e" or something. I used that approach to make node-lru-cache work with keys named __proto__

@TooTallNate
Owner

Another option worth trying would be to prefix all event names in the object with some character, like "e" or something. I used that approach to make node-lru-cache work with keys named __proto__

+1. This is the "standard" dictionary-in-JS practice as far as I know, and would solve all of these problems.

@isaacs
Owner

Whipped this up to test it out: https://github.com/isaacs/node/compare/ev_proto

Seems like http_simple doesn't get any slower with this. There's a bit of added string juggling, but not too bad.

@miksago

@isaacs might be nice to use "ev:" instead of just "ev", as to not end up with things like "evview", and instead "ev:view" (event name is "view")

@isaacs
Owner

Landed on b48e303.

might be nice to use "ev:" instead of just "ev", as to not end up with things like "evview", and instead "ev:view" (event name is "view")

Meh. It's internal anyway.

@isaacs
Owner

Turns out that this causes a significant regression with the emit() function. Users will just have to not use events named __proto__ or toString.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 5, 2012
  1. @tricknotes

    events: Support prototype property name as an event type

    tricknotes authored
    The prototype name (such as '__proto__') as an event type
    does not work as my expectation.
    
    ```
    > var events = require('events')
    > var e = new events.EventEmitter();
    > e.on('__proto__', function() {});
    > e.emit('__proto__');
    TypeError: Object #<Object> has no method 'apply'
        at EventEmitter.emit (events.js:126:20)
        at repl:1:3
        at REPLServer.self.eval (repl.js:109:21)
        at rli.on.self.bufferedCmd (repl.js:258:20)
        at REPLServer.self.eval (repl.js:116:5)
        at Interface.<anonymous> (repl.js:248:12)
        at Interface.EventEmitter.emit (events.js:96:17)
        at Interface._onLine (readline.js:200:10)
        at Interface._line (readline.js:518:8)
        at Interface._ttyWrite (readline.js:736:14)
    ```
    
    To support these, use `Object.create(null)` instead of `{}`.
This page is out of date. Refresh to see the latest.
View
8 lib/events.js
@@ -44,7 +44,7 @@ exports.EventEmitter = EventEmitter;
// that to be increased. Set to zero for unlimited.
var defaultMaxListeners = 10;
EventEmitter.prototype.setMaxListeners = function(n) {
- if (!this._events) this._events = {};
+ if (!this._events) this._events = Object.create(null);
this._maxListeners = n;
};
@@ -140,7 +140,7 @@ EventEmitter.prototype.addListener = function(type, listener) {
throw new Error('addListener only takes instances of Function');
}
- if (!this._events) this._events = {};
+ if (!this._events) this._events = Object.create(null);
// To avoid recursion in the case that type == "newListeners"! Before
// adding it to the listeners, first emit "newListeners".
@@ -234,7 +234,7 @@ EventEmitter.prototype.removeListener = function(type, listener) {
EventEmitter.prototype.removeAllListeners = function(type) {
if (arguments.length === 0) {
- this._events = {};
+ this._events = Object.create(null);
return this;
}
@@ -244,7 +244,7 @@ EventEmitter.prototype.removeAllListeners = function(type) {
};
EventEmitter.prototype.listeners = function(type) {
- if (!this._events) this._events = {};
+ if (!this._events) this._events = Object.create(null);
if (!this._events[type]) this._events[type] = [];
if (!isArray(this._events[type])) {
this._events[type] = [this._events[type]];
View
37 test/simple/test-event-emitter-prototype-property.js
@@ -0,0 +1,37 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var events = require('events');
+
+var e = new events.EventEmitter();
+
+var emited = false;
+e.on('__proto__', function() {
+ emited = true;
+});
+
+e.emit('__proto__');
+
+process.on('exit', function() {
+ assert.equal(true, emited);
+});
Something went wrong with that request. Please try again.