Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

RangeErrors caused by removeAllListeners change in v0.7 #3425

Closed
reid opened this Issue Jun 13, 2012 · 13 comments

Comments

Projects
None yet
7 participants

reid commented Jun 13, 2012

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 commented Jun 13, 2012

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 commented Jun 13, 2012

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

@isaacs isaacs closed this Jun 13, 2012

@isaacs isaacs reopened this Jun 13, 2012

reid commented Jun 13, 2012

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

Owner

bnoordhuis commented Jun 13, 2012

@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.

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

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

@reid reid added a commit to reid/node that referenced this issue Jun 13, 2012

@reid reid Fix #3425: removeAllListeners should delete array
When removeAllListeners is called, the listeners array
is deleted to maintain compatibility with v0.6.

Reverts "events: don't delete the listeners array"

This reverts commit 78dc13f.

Conflicts:

	test/simple/test-event-emitter-remove-all-listeners.js
2fd00b8

@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?

Member

piscisaureus commented Jun 14, 2012

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

Member

piscisaureus commented Jun 14, 2012

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

reid commented Jun 14, 2012

@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);.

@indutny indutny was assigned Jun 14, 2012

isaacs commented Jun 14, 2012

I'm generally in agreement with @reid on this. We advertised that the behavior of require('events') wouldn't change, and here it is clearly changed. It is clear that a compromise will not work: either the listener array is a persistent reference after removeAllListeners, or it is not. It's easy for @reid (and others) to work around in their currently-existing-in-reality programs, but they shouldn't have to; it'll be even easier to work around the other direction in programs that don't yet exist.

That being said, what's the actual harm caused by not leaving the array intact? So far, the main arguments for not reverting are (a) "the docs say so", and (b) "oh, it'd be cool if you could...". For (a) we can change the docs, and for (b) we can shrug and say "Sorry, that's not how it works, do a different thing".

But not to be too hasty, are there other arguments for keeping the new behavior?

Well if we revert back to the previous behavior, then there's a chance of somebody calling process.stdin.removeAllListeners('keypress'), which would effectively detach the listeners variable used internally to generate said keypress events: https://github.com/joyent/node/blob/412c1ab5bc254906d8f68b22fdabef82dea1a15a/lib/readline.js#L789

That emitKeypressEvents was one of the main reasons Ben and I made these changes in the first place. Since if there's code in core relying on this broken behavior, then there's definitely other people replying on it too...

@isaacs isaacs closed this in c9a1b5d Jun 15, 2012

isaacs commented Jun 15, 2012

See also: #3442
joyent#3442

@isaacs isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Jun 15, 2012

@isaacs isaacs 2012.06.15, Version 0.7.11 (unstable)
* V8: Upgrade to v3.11.10

* npm: Upgrade to 1.1.26

* doc: Improve cross-linking in API docs markdown (Ben Kelly)

* Fix #3425: removeAllListeners should delete array (Reid Burke)

* cluster: don't silently drop messages when the write queue gets big (Bert Belder)

* Add Buffer.concat method (isaacs)

* windows: make symlinks tolerant to forward slashes (Bert Belder)

* build: Add node.d and node.1 to installer (isaacs)

* cluster: rename worker.unqiueID to worker.id (Andreas Madsen)

* Windows: Enable ETW events on Windows for existing DTrace probes. (Igor Zinkovsky)

* test: bundle node-weak in test/gc so that it doesn't need to be downloaded (Nathan Rajlich)

* Make many tests pass on Windows (Bert Belder)

* Fix #3388 Support listening on file descriptors (isaacs)

* Fix #3407 Add os.tmpDir() (isaacs)

* Unbreak the snapshotted build on Windows (Bert Belder)

* Clean up child_process.kill throws (Bert Belder)

* crypto: make cipher/decipher accept buffer args (Ben Noordhuis)
1f93aa5

@evangoer evangoer pushed a commit to evangoer/yeti that referenced this issue Nov 15, 2013

@reid reid Add Node.js 0.7.x to Travis CI testing.
Node.js 0.7.11 fixes a regression from 0.6.x
that broke the phantom module used in testing.

See: nodejs/node-v0.x-archive#3425
a0a2bdf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment