The 'error' event can be emitted more than once when using writable.destroy()

[lpinca]

• Version: v11.9.0
• Platform: macOS
• Subsystem: stream

The 'error' event can be emitted multiple times when using writable.destroy() if the _destroy() callback is called asynchronously. Here is a test case:

const { Writable } = require('stream');

const writable = new Writable({
  destroy(err, callback) {
    process.nextTick(callback, new Error('oops'));
  }
});

writable.on('error', console.error);

writable.destroy();

// Assume an internal resource is closed in the `_destroy()` implementation.
// The resource fails to be closed cleanly causing `writable.destroy()` to be
// called again with an error.
writable.destroy(new Error('error'));

Actual result:

The 'error' event is emitted twice.

Expected result:

The 'error' event is emitted only once.

This is because the _writableState.errorEmitted guard is set to true when the callback is called.

[lpinca]

This seems to fix the issue without breaking any existing tests

diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js
index 0c652be9dd..200c75459a 100644
--- a/lib/internal/streams/destroy.js
+++ b/lib/internal/streams/destroy.js
@@ -10,10 +10,15 @@ function destroy(err, cb) {
   if (readableDestroyed || writableDestroyed) {
     if (cb) {
       cb(err);
-    } else if (err &&
-               (!this._writableState || !this._writableState.errorEmitted)) {
-      process.nextTick(emitErrorNT, this, err);
+    } else if (err) {
+      if (!this._writableState) {
+        process.nextTick(emitErrorNT, this, err);
+      } else if (!this._writableState.errorEmitted) {
+        this._writableState.errorEmitted = true;
+        process.nextTick(emitErrorNT, this, err);
+      }
     }
+
     return this;
   }
 
@@ -31,9 +36,13 @@ function destroy(err, cb) {
 
   this._destroy(err || null, (err) => {
     if (!cb && err) {
-      process.nextTick(emitErrorAndCloseNT, this, err);
-      if (this._writableState) {
+      if (!this._writableState) {
+        process.nextTick(emitErrorAndCloseNT, this, err);
+      } else if (!this._writableState.errorEmitted) {
         this._writableState.errorEmitted = true;
+        process.nextTick(emitErrorAndCloseNT, this, err);
+      } else {
+        process.nextTick(emitCloseNT, this);
       }
     } else if (cb) {
       process.nextTick(emitCloseNT, this);

but is there a reason for having no guard at all for readable only streams?

[lpinca]

cc: @nodejs/streams

[mcollina]

but is there a reason for having no guard at all for readable only streams?

The time needed for making it happen and dealing with the potential ecosystem breakage. If you got some stretch of time, send a PR!

[mcollina]

@lpinca can you PR the changes in #26015 (comment)?

[lpinca]

Yes, will do.