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.