stream: ensure finish is emitted in next tick

When using end() it was possible for 'finish' to be emitted synchronously. PR-URL: nodejs#30733 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax · Dec 14, 2019 · e13a37e · e13a37e
1 parent 8dea6dc
commit e13a37e
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 17 deletions.
diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
@@ -698,30 +698,40 @@ function prefinish(stream, state) {
   }
 }
 
-function finishMaybe(stream, state) {
+function finishMaybe(stream, state, sync) {
   const need = needFinish(state);
   if (need) {
     prefinish(stream, state);
     if (state.pendingcb === 0) {
-      state.finished = true;
-      stream.emit('finish');
-
-      if (state.autoDestroy) {
-        // In case of duplex streams we need a way to detect
-        // if the readable side is ready for autoDestroy as well
-        const rState = stream._readableState;
-        if (!rState || (rState.autoDestroy && rState.endEmitted)) {
-          stream.destroy();
-        }
+      state.pendingcb++;
+      if (sync) {
+        process.nextTick(finish, stream, state);
+      } else {
+        finish(stream, state);
       }
     }
   }
   return need;
 }
 
+function finish(stream, state) {
+  state.pendingcb--;
+  state.finished = true;
+  stream.emit('finish');
+
+  if (state.autoDestroy) {
+    // In case of duplex streams we need a way to detect
+    // if the readable side is ready for autoDestroy as well
+    const rState = stream._readableState;
+    if (!rState || (rState.autoDestroy && rState.endEmitted)) {
+      stream.destroy();
+    }
+  }
+}
+
 function endWritable(stream, state, cb) {
   state.ending = true;
-  finishMaybe(stream, state);
+  finishMaybe(stream, state, true);
   if (cb) {
     if (state.finished)
       process.nextTick(cb);

diff --git a/test/parallel/test-internal-fs-syncwritestream.js b/test/parallel/test-internal-fs-syncwritestream.js
@@ -70,5 +70,7 @@ const filename = path.join(tmpdir.path, 'sync-write-stream.txt');
   assert.strictEqual(stream.fd, fd);
 
   stream.end();
-  assert.strictEqual(stream.fd, null);
+  stream.on('close', common.mustCall(() => {
+    assert.strictEqual(stream.fd, null);
+  }));
 }
diff --git a/test/parallel/test-stream-writable-destroy.js b/test/parallel/test-stream-writable-destroy.js
@@ -238,13 +238,15 @@ const assert = require('assert');
   // called again.
   const write = new Writable({
     write: common.mustNotCall(),
-    final: common.mustCall((cb) => cb(), 2)
+    final: common.mustCall((cb) => cb(), 2),
+    autoDestroy: true
   });
 
   write.end();
-  write.destroy();
-  write._undestroy();
-  write.end();
+  write.once('close', common.mustCall(() => {
+    write._undestroy();
+    write.end();
+  }));
 }
 
 {

diff --git a/test/parallel/test-stream-writable-finished.js b/test/parallel/test-stream-writable-finished.js
@@ -28,3 +28,16 @@ const assert = require('assert');
     assert.strictEqual(writable.writableFinished, true);
   }));
 }
+
+{
+  // Emit finish asynchronously
+
+  const w = new Writable({
+    write(chunk, encoding, cb) {
+      cb();
+    }
+  });
+
+  w.end();
+  w.on('finish', common.mustCall());
+}