stream: don't emit finish after destroy

nodejs · Nov 18, 2021 · e256132 · e256132
1 parent 415726b
commit e256132
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 12 deletions.
diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js
@@ -650,6 +650,7 @@ Writable.prototype.end = function(chunk, encoding, cb) {
 
 function needFinish(state) {
   return (state.ending &&
+          !state.destroyed &&
           state.constructed &&
           state.length === 0 &&
           !state.errored &&
@@ -732,11 +733,18 @@ function prefinish(stream, state) {
 function finishMaybe(stream, state, sync) {
   if (needFinish(state)) {
     prefinish(stream, state);
-    if (state.pendingcb === 0 && needFinish(state)) {
-      state.pendingcb++;
+    if (state.pendingcb === 0) {
       if (sync) {
-        process.nextTick(finish, stream, state);
-      } else {
+        state.pendingcb++;
+        process.nextTick((stream, state) => {
+          if (needFinish(state)) {
+            finish(stream, state);
+          } else {
+            state.pendingcb--;
+          }
+        }, stream, state);
+      } else if (needFinish(state)) {
+        state.pendingcb++;
         finish(stream, state);
       }
     }

diff --git a/test/parallel/test-stream-duplex-destroy.js b/test/parallel/test-stream-duplex-destroy.js
@@ -125,7 +125,7 @@ const assert = require('assert');
   duplex.removeListener('end', fail);
   duplex.removeListener('finish', fail);
   duplex.on('end', common.mustNotCall());
-  duplex.on('finish', common.mustCall());
+  duplex.on('finish', common.mustNotCall());
   assert.strictEqual(duplex.destroyed, true);
 }
 

diff --git a/test/parallel/test-stream-transform-destroy.js b/test/parallel/test-stream-transform-destroy.js
@@ -117,7 +117,7 @@ const assert = require('assert');
   transform.removeListener('end', fail);
   transform.removeListener('finish', fail);
   transform.on('end', common.mustCall());
-  transform.on('finish', common.mustCall());
+  transform.on('finish', common.mustNotCall());
 }
 
 {

diff --git a/test/parallel/test-stream-writable-destroy.js b/test/parallel/test-stream-writable-destroy.js
@@ -124,8 +124,6 @@ const assert = require('assert');
 
   write.destroy();
 
-  write.removeListener('finish', fail);
-  write.on('finish', common.mustCall());
   assert.strictEqual(write.destroyed, true);
 }
 

diff --git a/test/parallel/test-stream-writable-finish-destroyed.js b/test/parallel/test-stream-writable-finish-destroyed.js
@@ -31,3 +31,13 @@ const { Writable } = require('stream');
   w.write('asd');
   w.destroy();
 }
+
+{
+  const w = new Writable({
+    write() {
+    }
+  });
+  w.on('finish', common.mustNotCall());
+  w.end();
+  w.destroy();
+}
diff --git a/test/parallel/test-stream-write-destroy.js b/test/parallel/test-stream-write-destroy.js
@@ -60,9 +60,5 @@ for (const withPendingData of [ false, true ]) {
     assert.strictEqual(chunksWritten, useEnd && !withPendingData ? 1 : 2);
     assert.strictEqual(callbacks.length, 0);
     assert.strictEqual(drains, 1);
-
-    // When we used `.end()`, we see the 'finished' event if and only if
-    // we actually finished processing the write queue.
-    assert.strictEqual(finished, !withPendingData && useEnd);
   }
 }