RangeErrors caused by removeAllListeners change in v0.7

[reid]

A common code pattern I see in projects that need to attach to another http.Server is this:

// We want myAwesomeHandler to run before
// other event listeners. We also may hijack the
// event for our own purposes. This is useful for
// attaching to other servers (say, we want to
// only respond to requests starting with "/yeti")
// or for handling HTTP upgrades befor anything
// else (say, for Socket.io).

var listeners = server.listeners("request");

server.removeAllListeners("request");

server.on("request", function myAwesomeHandler(req, res) {
   if (req.url.indexOf("/awesome") !== -1) {
      // We're going to handle this one.
      handleAwesomeRequest(req, res);
      return;
   }
   // The original listeners should handle this one.
   listeners.forEach(function (fn) {
     // In Node.js v0.7, listeners now contains myAwesomeHandler!
     // myAwesomeHandler is now called recursively.
     fn.call(this, req, res);
   }
});

The problem is that 78dc13f causes removeAllListeners to maintain the array reference returned by the listeners call, which results in new side effects: since Node v0.7, the listeners array shown above will be updated with new event listeners attached.

In Node v0.6, the call to listeners() acted like a copy, so this code would not result in a loop.

If the listeners array is copied before being used, the v0.6-style behavior can be obtained. For exmaple, see: yui/yeti@51aa577#L1R25

This is currently a problem for the dnode project, which uses Socket.io 0.8.6 that uses this pattern for handling HTTP upgrades. This was also a problem with my own code as well.

For the sake of easy upgrades from v0.6, I request for the existing behavior to be maintained.

If this request is denied, the wiki about 6-to-8 changes should be updated to reflect this behavior change: https://github.com/joyent/node/wiki/API-changes-between-v0.6-and-v0.8

[isaacs]

I think this is a pretty strong argument for maintaining the v0.6 behavior.

@bnoordhuis Besides the doc inconsistency, are there examples in the wild of this causing issues for people? If not, do you have a strong objection if I revert 78dc13f and make it clear in the docs that removeAllListeners also removes the listeners reference?

[isaacs]

Or actually, @reid, wanna just send a patch?

[reid]

@isaacs Yeah, I'll send a patch in a few hours.

[bnoordhuis]

@isaacs Someone complained about it, that's why I fixed it. But if it causes more issues than it solves (which apparently it does), by all means revert it.

[TooTallNate]

Ughhh, I wish removeAllListeners() just went away :p

[defunctzombie]

@TooTallNate without removeAllListeners() what is a good way to cleanup event emitter objects so they can be free'd

[TooTallNate]

@shtylman

var listeners = server.listeners("request").splice(0);

^ That's backwards compatible and works with the new behavior as well.

In fact, it would be ideal if that's what removeAllListeners() actually did. I propose that we should rewrite it as:

EventEmitter.prototype.removeAllListeners = function(type) {
  return this.listeners(type).splice(0);
}

That would make everyone happy. It would keep the existing code working, and in fact you could do both in one shot now as well:

var listeners = server.removeAllListeners("request");

Thoughts people?

[piscisaureus]

Or we could just get rid of the fact that .listeners() returns a mutable array :-)

[piscisaureus]

I do not agree with the reverting to the 0.6 behavior. @TooTallNate's suggesting is kinda neat.

[reid]

@piscisaureus Unfortunately, @TooTallNate's proposal keeps the array reference intact after removeAllListeners("request").

Keeping the array reference alive breaks compatibility with a frozen API.

If we had the implementation by @bnoordhuis or @TooTallNate before the API was stable, I'd prefer it. I do not believe the benefit is significant enough to break compatibility that's guaranteed by the Stability Index.

The test inside pull request 3431 verifies the behavior I described in my original example above. The test fails when I change removeAllListeners to return this.listeners(type).splice(0);.