Domain feature must not clobber EventEmitter#domain field.

[ashtuchkin]

// Constructor of some model class.
function WebSite(domain) {
    this.domain = domain;
}

// Inheriting from EventEmitter.
var EventEmitter = require('events').EventEmitter;
WebSite.prototype = new EventEmitter();

// Create instance. 
var website = new WebSite("google.com");

// Add event handler.
website.on("ping", function() { console.log("pong"); });

// Try to emit event.
website.emit("ping");

> TypeError: Object google.com has no method 'enter'
    at EventEmitter.emit (events.js:80:19)
    ...
// What? How? Hmm.. 

What happens here is that EventEmitter uses a field named 'domain' to store its domain, and tries to call this.domain.enter(), which is neither documented, nor expected.

Solution: At least rename it to '_domain' (like '_events', internal by convention).

Background: I have a Mongoose model with field 'domain' and cannot rename it easily as the database depends on it. Therefore I have no choice but to get back to Node v0.6, until this bug is fixed. Please, help!

[tellnes]

+1

I had the same problem earlier today

[ashtuchkin]

I could make a pull request if you have no time to fix it right now.. Because it's really stopping me from doing work on my project.

[piscisaureus]

/cc @isaacs

[jaredhanson]

+1

[isaacs]

This causes a regression on several benchmarks: https://gist.github.com/4747944

At least for now, I'm afraid the name domain is just a part of the EventEmitter API.

[jaredhanson]

Ugh... This breaks any Mongoose schema with a "domain" property. See reports on the mailing list.

I'm sure fingers can point both ways between the projects, but this is common enough and a particularly nasty way to fail. Have you had any discussions with them about the issue?

[isaacs]

Why are your Mongoose schema event emitters? It will break much worse if you have schema that have an "emit" or "on" property.

[jaredhanson]

It's probably not the schemas that are event emitters, but the models/documents that are constructed from them (which will naturally have a "domain" property too). I haven't dug too deep into Mongoose to comment with much confidence past that.

I'm the messenger here, having just been bitten hard. I have a lot of Schemas with "domain" properties, and any attempt to create and save one back to MongoDB fails with the following stack trace:

events.js:84
        this.domain.enter();
                    ^
TypeError: Object github.com has no method 'enter'
    at model.EventEmitter.emit (events.js:84:21)
    at model.save (/Users/jaredhanson/Projects/website/node_modules/mongoose/lib/model.js:383:10)
    at model.module.exports.hook.proto.(anonymous function)._done (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:59:24)
    at module.exports.hook.proto.(anonymous function)._next (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:52:28)
    at fnWrapper (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:159:8)
    at model.module.exports (/Users/jaredhanson/Projects/node/mongoose-timestamps/lib/timestamps.js:22:5)
    at module.exports.hook.proto.(anonymous function)._next (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:50:30)
    at fnWrapper (/Users/jaredhanson/Projects/website/node_modules/mongoose/node_modules/hooks/hooks.js:159:8)
    at complete (/Users/jaredhanson/Projects/website/node_modules/mongoose/lib/document.js:920:5)
    at Document.validate.err (/Users/jaredhanson/Projects/website/node_modules/mongoose/lib/document.js:911:20)

In this case, "github.com" is the value I assigned to my domain property (on an Account model).

The appropriate fix/workaround may indeed be in the Mongoose project, although they've fingered Node core in their discussions. This seems to me likely to be a common scenario and source of frustration, and it doesn't seem good for both projects to consider the issue "won't fix". I don't think the workaround for this should be "change your schemas."

[ashtuchkin]

+1
Is there any good use of EventEmitter#domain property from a user perspective?
IMHO it should be an implementation detail, hidden from users, just like _events.

[isaacs]

It's probably not the schemas that are event emitters, but the models/documents that are constructed from them (which will naturally have a "domain" property too).

So... Why are the "models/documents that are constructed from them" EventEmitters? Something is calling EventEmitter.emit() on that object. That is highly unsafe for random bag-o-data objects like you'd expect to be backed by a database. Whatever program is doing this has a hideous and awful bug, which is likely a serious security issue (depending on how the _events and emit fields are guarded, perhaps even one that allows arbitrary code injection). Please go fix or report it there, asap!

[jaredhanson]

These are just standard mongoose.Model instances, which inherit from mongoose.Document:

https://github.com/LearnBoost/mongoose/blob/master/lib/model.js
https://github.com/LearnBoost/mongoose/blob/master/lib/document.js

mongoose.Document inherits from EventEmitter, and emits events relating to the object lifecycle, such as when it is validated, saved, or removed. See:

https://github.com/LearnBoost/mongoose/blob/master/lib/document.js#L919
https://github.com/LearnBoost/mongoose/blob/master/lib/model.js#L334
https://github.com/LearnBoost/mongoose/blob/master/lib/model.js#L689

I see nothing "highly unsafe" about this, and it is, in fact, pretty standard usage of EventEmitter and not a "hideous and awful bug".

The problem is, that any given mongoose.Model instance may have a domain property as part of its schema. And then, when the Model calls emit('save'), Node core calls this.domain.enter() here:

https://github.com/joyent/node/blob/master/lib/events.js#L81

As noted in the bug, anything with a domain property that is also an EventEmitter is going to have this conflict. There's a pretty strong argument to be made that EventEmitter is not properly hiding what should be an internal implementation details.

Please take a moment to investigate this further, rather than dismissing it as "likely a serious security issue" (which I doubt). If, in fact, it is a security issue, then it impacts not only every Mongoose deployment, but likely many other places that EventEmitter is being used. I will report to the Mongoose maintainers, but I don't consider Node core to be exempt from this discussion.

[jaredhanson]

Cross-referencing the issue I just filed with Mongooose:

Automattic/mongoose#1338

[isaacs]

The unsafe thing is that you're using an EventEmitter as an arbitrary data store. It's not for that. It has a specific API. It also doesn't "hide" the emit function, or removeAllListeners functions.

If Document inherits from EventEmitter, then you should not be putting arbitrary keys on it. You should create a specific property like data, and put your arbitrary keys and values on there.

As noted in the bug, anything with a domain property that is also an EventEmitter is going to have this conflict.

As noted in my response, anything with an emit property that is also an EventEmitter is going to have an even worse conflict. EventEmitters are not data bags. Don't use them for that purpose.

[jaredhanson]

I'm afraid I can't agree with the point you are making.

EventEmitter is used as an "interface" to inherit in so many Node and JavaScript projects that it is too numerous to count. That interface is detailed in the public API. Naturally, as part of inheriting, subclasses take care not to conflict with emit etc. However, in no place is domain even mentioned in that interface.

Every Node project that inherits EventEmitter is extending it with additional properties, and none of them (that I've seen) take any precaution to "namespace" any of them in data (or whatever). Is your suggestion that all of these projects are unsafe and violating some sacred principle of EventEmitter inheritance (which isn't known to anyone)? If so, I suggest that this be explicitly documented, and some set of "safe" properties (such as "data") be reserved for this purpose.

If this is your guidance, it'd would also set a good example if Node core would make Stream not add readable and writable properties, or any additional functions, to EventEmitter. I have full faith in your pragmatism, and am sure you'd see that suggestion as quite unreasonable. But, this is exactly illustrative of why this bug report deserves further consideration, and not reactionary comments.

[bnoordhuis]

It's not reactionary, it's just common sense. Same reason why you shouldn't let __proto__ get set on an object. Bad things will happen.

Though I agree that the name 'domain' is unfortunate.

[jaredhanson]

I agree, in JavaScript there are certain properties of "prime" importance, and you need to code carefully (as is the case for any highly dynamic language). For Node, EventEmitter and its properties reach close to that level, since it is probably one of the most widely inherited classes.

If we are at the point where we are going to start canonizing classes and their properties, that is fine. But, let's be explicit about the pros and cons, and what the recommended patterns are. My personal solution would be:

1. EventEmitter's "sacred" interface is its public API. Mess with that at your peril.
2. Domains are an internal implementation detail and should be hidden.

If @issacs solution, as Node's maintainer, is that EventEmitter should not be extended with additional properties, I'll reluctantly follow suit (while raising hell until the hammer falls). However, some things should be done in that case:

1. The domain property should be added to the public API
2. Some set of "safe" properties for "namespacing" extensions should be reserved.
3. Node core should start following suit with respect to its own inheritance of EventEmitter.

The clearly drastic downside of this latter path would be that inheriting "properly" from EventEmitter would be really fugly, looking like:

function MyClass(name) {
    events.EventEmitter.call(this);
    this.data.name = name;

    // sadly, can't put functions on the prototype of classes inherited
    // from EventEmitter
    this.data.sayHello = function() {
      console.log('Goodbye Cruel World!');
    }
}

util.inherits(MyClass, events.EventEmitter);

In my opinion, there's nothing pragmatic or rational in declaring that EventEmitter should be inherited in such a manner.

[aheckmann]

Mongoose is at fault here. We need to add domain and friends to our reserved properties list. As for the security concerns, we already store all data in an internal _doc object. Properties are just getters/setters to this store. In 4x we plan on revisiting the document model. If anyone would like to discuss the Mongoose related details further feel free to comment on the Mongoose issue mentioned above.

[isaacs]

I've been burned a lot of times by using class instance objects as bag-o-data stores. It's always a mess. Even using {} is problematic.

You have a few options:

1. Use an Object.create(null) to put your data things onto. This is slower for all gets and sets, so that sucks. Also, it's weird, because hasOwnProperty, toString, etc, are all missing.
2. Prefix all keys with some known string.
3. Put the data somewhere else (like how EventEmitter does with the _events object), and perhaps accept that using __proto__ as a key will cause weirdness.

[cstockton]

I would like to chime in as someone who just had to track this issue down. I think that assuming that anyone who tries to define a "domain" property is using objects as "bag-o-data" stores is a bit off.

In my case, I have a "XyzClient" instance that inherits from EventEmitter. This "XyzClient" reaches into a endpoint, where a set of 5-10 "services" will be made available. In my case one of these services was named "domain". The client then assigned a XyzClient.domain property, for the user to have easy access to this service. Is there a way around this? Sure I can make it XyzClient.foobar('domain') for example. However, at this point how is XyzClient any more wrong then the EventEmitter?

Lets just say I made a bad design decision.. which is subjective but I can accept that, regardless the NodeJS team has to EXPECT bad code to be written at times. I mean javascript is not exactly the paragon platform of best coding practices.

This is the first issue I've needed to track down on github and found a discussion for, but as a new advocate for NodeJS I am a bit disappointed to see a rather elitist position here of "YOUR CODE WRONG" as opposed to looking at the bigger picture and understanding your users will not write code how you think they should.

If you guys wont fix this, I have to think what happens if in version 1.0 of node you guys decide to add a property to EventEmitter or another core class I inherit and depend upon called "CorePublicPropertyOfMyLibraries", in the process of which you break all of my code? Knowing that it's my problem, because well, I shouldn't of added the property to begin with!

Sorry this came a little lengthy, but I just wanted to voice my opinion on the importance of a humble foresight into the horrible things your users will do with your libraries.

[d-oliveros]

@cstockton then don't inherit your prototype from EventEmitter. Create an EventEmitter instance in xyzClient.emitter in your prototype's constructor, or instanciate a static EventEmitter when you define your prototype instead. Unless you are legitimately extending EventEmitter, I don't see why you would inherit your whole core library as an event emitter.

[gritzko]

That domain field certainly violates the principle of least surprise.
IMO, EventEmitter adds more clutter than it should.