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

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.

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?

cc: @nodejs/streams

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!

@lpinca

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

Yes, will do.

stream: ensure writable.destroy() emits error once

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

lpinca commented on Feb 9, 2019

lpinca commented on Feb 12, 2019

mcollina commented on Feb 12, 2019

mcollina commented on Feb 12, 2019

lpinca commented on Feb 12, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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