Stream emitting same error twice under certain circomstances

[watson]

• Version: 12.18.1 and newer
• Platform: Darwin watson 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64
• Subsystem: stream

What steps will reproduce the bug?

Run this code on Node.js 12.18.1 or newer and notice how the error is emitted on b twice. On any older Node.js version it's only emitted once.

const { Readable, PassThrough, Writable, pipeline } = require('stream')

const a = new Readable({
  read () {
    this.destroy(new Error('foo'));
  }
})
const b = new PassThrough()
const c = new Writable()

a.once('error', (err) => {
  console.log('emitting error on b')
  b.emit('error', err) // Note: you're not supposed to emit error manually!
})

b.on('error', (err) => {
  console.log('b got error:', err.message)
})

pipeline(a.pipe(b), c, (err) => {
  console.log('end of pipeline, err:', err.message)
})

If I change b.emit('error', err) to b.destroy(err) the error goes away. Using destroy is best practise and the user isn't supposed to emit error events manually on streams like in the above code. However, this was how we told users to do it previously, so a lot of code most likely exists in the wild expecting this to be possible and emitting the same error twice can have bad consequences.

Thoughts:

1. It would be cool if we could warn users if they manually emitted an error event on a stream.
2. It would be nice if we could make this odd behaviour go away even if the user emits error manually.

This regression was most likely introduced in #30869.

[mcollina]

This is not a regression from my point of view, it actually fixed something that was broken before. The problem is that .emit('error', err) should not really be used as it bypass all the state management logic of streams. .destroy(err) has been the recommendation since Node v8.x, which is long in the past - It's time to update.

I would also note that this behave exactly as planned: emit('error', err) causes the error to be emitted on the stream. .pipeline() also does that via .destroy(err). I now agree that the change should likely have been semver-major, but I think that ship has long sailed as 12.x is heading into maintenance. Changing this also fixed some key bugs somewhere else, which is important for our users as well.

I do not think there is anything to do here :/.

[mafintosh]

@mcollina Ya agreed.

Watson and I discussed this issue offline and came to the same conclusion.
However I think there is something to be said here about the landscape being a bit confusing. I bet lots of people still think emit(error) is the way to go for streams. Any simple path where we could warn on that?

[mcollina]

Any simple path where we could warn on that?

Unfortunately not. The problem is that .emit('error', err) is also used by .destroy() and the rest of the internal machinery. We should really document this better.

[mafintosh]

@mcollina isn't it only emitted by destroy? Ie we can't just check if .destroyed = true in an overwrite of emit and throw a warning if not and the event is error?

[mcollina]

isn't it only emitted by destroy?

Yes

Ie we can't just check if .destroyed = true in an overwrite of emit and throw a warning if not and the event is error?

We could. The problem is that adding an override for emit() is likely going to slowdown things significantly as we emit a lot.

[mafintosh]

@mcollina We could just have an internal _emit that is just an alias to normal emit to avoid any slowdown

[mcollina]

@mcollina We could just have an internal _emit that is just an alias to normal emit to avoid any slowdown

Unfortunately that's not possible as some descendants override emit :/.

We should probably benchmark this and ship it anyway. I'm happy to review a PR.

[mmomtchev]

No statistically significant performance drop for node benchmark/run.js streams (ie variations across runs are inline with variations with or without the modification)

--- a/lib/internal/streams/legacy.js
+++ b/lib/internal/streams/legacy.js
@@ -93,6 +93,13 @@ Stream.prototype.pipe = function(dest, options) {
   return dest;
 };
 
+Stream.prototype.emit = function(event) {
+  if (event === 'error' && !this.destroyed)
+    process.emitWarning('Avoid emitting directly an \'error\' to Stream' +
+      ', prefer Stream.destroy()');
+  return EE.prototype.emit.apply(this, arguments);
+};
+
 function prependListener(emitter, event, fn) {
   // Sadly this is not cacheable as some libraries bundle their own
   // event emitter implementation with them.

Everything seems to work, but still an avalanche of warnings during make -j4 test - do as i say not as i do 😄

Do you really think it makes sense to add it?