Regression in stream.Readable

[kanongil]

The following code snippet no longer throws after 23d92ec from #5996. I suggest replacing it with my actual fix from #4978.

var R = require('stream').Readable;
var W = require('stream').Writable;

var r = new R;
r._read = function(n) {
  r.push(new Buffer('!'));
}

var w = new W;

function onerror(err) {
  console.error('got error', err);
}

w.on('error', onerror);
r.pipe(w);
w.removeListener('error', onerror);

[isaacs]

Why would this code snippet throw?

The first error event (caused by calling the not-implemented w._write()) removes the pipe connection, and then is caught by the onerror function. There are no more calls to w._write(), so the fact that you remove the error listener is irrelevant.

Consider this code snippet, which WILL throw:

var EE = require('events').EventEmitter;
var R = require('stream').Readable;
var W = require('stream').Writable;

var r = new R;
r._read = function(n) {
  r.push(new Buffer('!'));
}

var w = new W;
w.emit = function(orig) { return function(ev, arg) {
  if (ev === 'error')
    console.error('emit error', EE.listenerCount(w, ev) === 0 ? 'throw': 'catch', arg);
  return orig.apply(w, arguments);
}}(w.emit);

function onerror(err) {
  console.error('got error', err);
}

w.on('error', onerror);
r.pipe(w);
w.removeListener('error', onerror);

// but this will throw now!
r.pipe(w);

// EDIT: Removed the setImmediate, as it's not actually relevant.

[kanongil]

I tested it with the current 0.10.15 release, and a checkout from master. One throws, the other doesn't (and the error listener is never called).

[kanongil]

Expected output (from node v0.10.15):

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: not implemented
    at Writable._write (_stream_writable.js:317:6)
    at doWrite (_stream_writable.js:219:10)
    at writeOrBuffer (_stream_writable.js:209:5)
    at Writable.write (_stream_writable.js:180:11)
    at write (_stream_readable.js:573:24)
    at flow (_stream_readable.js:582:7)
    at _stream_readable.js:550:7
    at process._tickCallback (node.js:415:13)
    at Function.Module.runMain (module.js:499:11)
    at startup (node.js:119:16)

Output after patch 23d92ec:

[kanongil]

@isaacs To answer your question directly, the code would throw because the flow start is deferred to nextTick.

Anyway, the why is not relevant, rather the missed error handling caused by the recent patch. I can see that it is also in the pipeline for the current stable branch, and I would appreciate some further feedback on this issue.

Feel free to close this if I have misunderstood things, and it is now OK for node to silently ignore errors with no attached listeners.

[isaacs]

Ah, I see. Yes. I was getting confused by the example code. Here's a simpler case:

writable.on('error', handler);
readable.pipe(writable);
writable.removeListener('error', handler);
writable.emit('error', new Error('this should throw'));

Fixed by pull req #6075