Skip to content

Commit e13a37e

Browse files
ronagTrott
authored andcommitted
stream: ensure finish is emitted in next tick
When using end() it was possible for 'finish' to be emitted synchronously. PR-URL: #30733 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>
1 parent 8dea6dc commit e13a37e

File tree

4 files changed

+44
-17
lines changed

4 files changed

+44
-17
lines changed

lib/_stream_writable.js

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -698,30 +698,40 @@ function prefinish(stream, state) {
698698
}
699699
}
700700

701-
function finishMaybe(stream, state) {
701+
function finishMaybe(stream, state, sync) {
702702
const need = needFinish(state);
703703
if (need) {
704704
prefinish(stream, state);
705705
if (state.pendingcb === 0) {
706-
state.finished = true;
707-
stream.emit('finish');
708-
709-
if (state.autoDestroy) {
710-
// In case of duplex streams we need a way to detect
711-
// if the readable side is ready for autoDestroy as well
712-
const rState = stream._readableState;
713-
if (!rState || (rState.autoDestroy && rState.endEmitted)) {
714-
stream.destroy();
715-
}
706+
state.pendingcb++;
707+
if (sync) {
708+
process.nextTick(finish, stream, state);
709+
} else {
710+
finish(stream, state);
716711
}
717712
}
718713
}
719714
return need;
720715
}
721716

717+
function finish(stream, state) {
718+
state.pendingcb--;
719+
state.finished = true;
720+
stream.emit('finish');
721+
722+
if (state.autoDestroy) {
723+
// In case of duplex streams we need a way to detect
724+
// if the readable side is ready for autoDestroy as well
725+
const rState = stream._readableState;
726+
if (!rState || (rState.autoDestroy && rState.endEmitted)) {
727+
stream.destroy();
728+
}
729+
}
730+
}
731+
722732
function endWritable(stream, state, cb) {
723733
state.ending = true;
724-
finishMaybe(stream, state);
734+
finishMaybe(stream, state, true);
725735
if (cb) {
726736
if (state.finished)
727737
process.nextTick(cb);

test/parallel/test-internal-fs-syncwritestream.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,5 +70,7 @@ const filename = path.join(tmpdir.path, 'sync-write-stream.txt');
7070
assert.strictEqual(stream.fd, fd);
7171

7272
stream.end();
73-
assert.strictEqual(stream.fd, null);
73+
stream.on('close', common.mustCall(() => {
74+
assert.strictEqual(stream.fd, null);
75+
}));
7476
}

test/parallel/test-stream-writable-destroy.js

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -238,13 +238,15 @@ const assert = require('assert');
238238
// called again.
239239
const write = new Writable({
240240
write: common.mustNotCall(),
241-
final: common.mustCall((cb) => cb(), 2)
241+
final: common.mustCall((cb) => cb(), 2),
242+
autoDestroy: true
242243
});
243244

244245
write.end();
245-
write.destroy();
246-
write._undestroy();
247-
write.end();
246+
write.once('close', common.mustCall(() => {
247+
write._undestroy();
248+
write.end();
249+
}));
248250
}
249251

250252
{

test/parallel/test-stream-writable-finished.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,3 +28,16 @@ const assert = require('assert');
2828
assert.strictEqual(writable.writableFinished, true);
2929
}));
3030
}
31+
32+
{
33+
// Emit finish asynchronously
34+
35+
const w = new Writable({
36+
write(chunk, encoding, cb) {
37+
cb();
38+
}
39+
});
40+
41+
w.end();
42+
w.on('finish', common.mustCall());
43+
}

0 commit comments

Comments
 (0)