events: shared object references are emitted to listeners

[samhmills]

When emitting to multiple listeners for the same event type, the for
loop iteration over these listeners shares the same arguments. This
means (for objects), the copy is only shallow, and changes made by one
listener are overwritten on the object for the next listener.

For example, if we pass an object with arrays: as below

EventEmitter.emit("event", {
  data: [{
      users: [9237895]
  }]
});

And the first listener sets the users array as null like so:

EventEmitter.on('event', function(packet) {
    var data = packet.data[0];
    data.users = null;
});

The second event listener will ALWAYS show the array as null. This is
because only a shallow copy of the arguments is made and not a deep
copy.

Here is the full code snippet that will reproduce the issue predictably:

var EventEmitter = require('events').EventEmitter;
var EE = new EventEmitter();

console.log("starting")

EE.on('event', function(packet) {
    //console.log(packet); //Log pre to see that it is not null
    var data = packet.data[0];
        data.users = null;  
});

EE.on('event', function(packet) {
    console.log(packet.data[0]); //packet.data[0].users will equal null.
});


setInterval(function(){
    console.log("emitting");
    EE.emit("event", {
        data: [
            {
                users: [9237895] //This outputs as null on the second event listener;
            }
        ]
    });
}, 5000)

[jasnell]

Yes, I've run into this a few times myself. It's definitely not ideal behavior. I'll schedule some time to review the PR but changing this behavior is, unfortunately, an API change that needs to be handled with a bit of care.

cc @joyent/node-coreteam

[Fishrock123]

Oh joy. I bet at least some people are using this in convoluted ways.

[cjihrig]

I'm -1 on this. While I sympathize with the current implementation's problems, performing deep copies can be expensive. Isn't it JavaScript convention to pass objects around by reference anyway?

[Fishrock123]

-1 for the same reasons as @cjihrig though.

[jasnell]

@cjihrig ... generally yes, but this behavior should, at the very least, be documented and as far as I can tell it is not. I'm just as concerned about the deep copying tho. Forcing a deep copy for every listener is going to be extremely expensive and likely completely unnecessary. However, it may be worthwhile providing a utility that performs a deep copy on a per listener basis... something along the lines of:

var listener = function(arg) { ... };
emitter.addListener('event', emitter.copyListener(listener));

This way not all listeners would pay the deep copy penalty. At the very least, this needs to be documented.

[cjihrig]

+1 for documenting

[jasnell]

@Hunchmun ... definitely appreciate the PR but given the discussion here, I think the better approach is going to be documenting the behavior in doc/api/events.markdown along with an example that illustrates the behavior. For your own listeners, implementing a simple wrapper that forces the deep copy on a listener-by-listener basis would likely be worthwhile if this is proving to be a problem for you. Would you be willing to update your PR to do the documentation change rather than the source change in events.js?

[samhmills]

@jasnell absolutely, I'll comment back when it has been updated!

[samhmills]

@jasnell please find above the updated PR (With the initial commit reverted).

[samhmills]

@jasnell @cjihrig Agreed with both of your points from the discussion, above you will find the appended and moved version, as per your request. Also, squashed back down to one commit.

[jasnell]

@Hunchmun ... Thank you for updating and being patient with us. I have this on my list of items to review either tomorrow or Friday (it's a bit of a crazy week here). If you don't see action on this by the end of the day Friday, give me another ping here.

[samhmills]

Hi @jasnell , just following up as requested,

Thanks,

[jasnell]

Thanks! I haven't forgotten, just on vacation at the moment. It's on my
list for as soon as I'm back.
On Jun 11, 2015 1:18 PM, "Sam Mills (Henchman)" notifications@github.com
wrote:

Hi @jasnell https://github.com/jasnell , just following up as requested,

Thanks,

—
Reply to this email directly or view it on GitHub
#25466 (comment).