streams.js line 81 (v0.8.11) emits error incorrectly

[mcavage]

Hello,

In some basic network server testing, I found that using .pipe and .once('error') correctly is crazy for consumers. The order in which you call .pipe and .once('error') dictates whether your program will exit with an uncaughtException or not. I found this with EPIPE and long-running server "things" sending data back to a client, but here's a trivial repro case:

var net = require('net');

var stream = net.connect({ 'host': 'www.googleasdfalkjsdf.com', 'port': '80' });
stream.end('GET / HTTP/1.0\n\n');

// This is ok
// stream.pipe(process.stdout);
// stream.once('error', function (err) {});

// This is not ok
stream.once('error', function (err) {});
stream.pipe(process.stdout);

The streams.js implementation of pipe does a check for listeners.length on error, and rethrows an error if there's nothing listening. And, it is common to use .once on 'error' and 'end', since those are expected to indicate "cleanup", and it's fairly easy to cause memory leaks by leaving listeners around with .on. So, with the illustration above, if the listener call is attached before pipe, the .once logic removes the error, and pipe rethrows, even though the program correctly caught the error.

Is this intended behavior?

[langpavel]

Desired or not this is error prone.
Should be this fixed by removing events in process.nextTick?
Not sure if on EventEmitter or somewhere else..

[bnoordhuis]

Is this intended behavior?

No, but it falls out that way because of how .once() is implemented. I can acknowledge the bug but I don't see a good way to fix it without very hacky additions to Stream or EventEmitter.

[bnoordhuis]

/cc @isaacs - maybe his streams2 work can address this.

[spion]

Hi. I got bitten by this issue recently and tried to think about what could be done to fix it. I see two ways:

Change the semantics of once slightly

Adding a scheduledForDeletion object to EventEmitter that for each event keeps an array where once listeners will be added after they are called.

This will make once listeners aware of each other. Note how the once listener still checks if its scheduledForDeletion before firing the original function - thus keeping behavior almost completely the same (it will not under any circumstances fire twice)

EventEmitter.prototype.once = function(type, listener) {
  ...
  function g() {
    if (self.scheduledForDeletion[type].indexOf(g) !== -1) return;
    self.scheduledForDeletion[type].push(g);
    listener.apply(this, arguments);
  };
  ...
}

Then at the end of emit remove all listeners scheduledForDeletion - something like this:

var deletions = self.scheduledForDeletion[type];
if (deletions.length > 0) {
  for (i = 0, l = deletions.length; i < l; ++i) {
    self.removeListener(type, deletions[i]);
  }
  self.scheduledForDeletion[type] = [];
}

The result is that once listeners are removed only after all the listeners for that event are fired. This may:

• break existing code that assumes that the listeners array is updated immediately when a once listener is fired, not after the whole emit completes (I think this is never the case, in fact even node core assumes the opposite as it can be seen here)
• make every .emit slightly slower (probably negligible, basically only checks self.scheduledForDeletion[type].length > 0 after every emit)

But it won't break the essential meaning of once - that the listener will only fire once in all possible cases.

Change .pipe() to alter the listeners list

Will first fetch listeners with listeners('error'), removeAllListeners('error'), add its own error listener, then re-add all other listeners. This does the bare minimum and has virtually no impact on other code, it would only remove the bug. However

• it seems hacky (emulates an insertListener API)
• the same issue will re-surface in other modules that inherit from EventEmitter and use a similar error handling scheme...

[kanongil]

@spion a modified once() won't do much good, since the error will also be there if a regular on listener is applied and the callback uses removeListener() to clean up after itself.

[spion]

@kanogil yeah, we could use that to get the old broken behavior, if we really need it. The immediate removal will be explicit, inside the listener code.

On the other hand, to get the new and improved behavior (one-time listeners that can see eachother) we can just use "once".

Finally if we want to use the underlying scheduledForDeletion mechanism, it can be exposed through the EventEmitter API. Maybe disableAndRemoveListenerLater(fn) (I am terrible with names). How does that sound? (not the name, I know its horrible)

Edit: Or how about making removeListener schedule removals for later if called in the middle of the event?