Skip to content

Commit

Permalink
stream: don't emit finish after close
Browse files Browse the repository at this point in the history
Writable stream could emit 'finish' after 'close'.

PR-URL: #32933
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
ronag authored and BethGriggs committed Apr 28, 2020
1 parent fad188f commit 3c07b17
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 3 deletions.
7 changes: 6 additions & 1 deletion lib/_stream_writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,10 @@ function WritableState(options, stream, isDuplex) {

// Indicates whether the stream has finished destroying.
this.closed = false;

// True if close has been emitted or would have been emitted
// depending on emitClose.
this.closeEmitted = false;
}

function resetBuffer(state) {
Expand Down Expand Up @@ -639,7 +643,8 @@ function finishMaybe(stream, state, sync) {

function finish(stream, state) {
state.pendingcb--;
if (state.errorEmitted)
// TODO (ronag): Unify with needFinish.
if (state.errorEmitted || state.closeEmitted)
return;

state.finished = true;
Expand Down
9 changes: 7 additions & 2 deletions lib/internal/streams/destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ function emitCloseNT(self) {
const r = self._readableState;
const w = self._writableState;

if (w) {
w.closeEmitted = true;
}

if ((w && w.emitClose) || (r && r.emitClose)) {
self.emit('close');
}
Expand Down Expand Up @@ -111,15 +115,16 @@ function undestroy() {
}

if (w) {
w.closed = false;
w.destroyed = false;
w.closed = false;
w.closeEmitted = false;
w.errored = false;
w.errorEmitted = false;
w.ended = false;
w.ending = false;
w.finalCalled = false;
w.prefinished = false;
w.finished = false;
w.errorEmitted = false;
}
}

Expand Down
33 changes: 33 additions & 0 deletions test/parallel/test-stream-writable-finish-destroyed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';

const common = require('../common');
const { Writable } = require('stream');

{
const w = new Writable({
write: common.mustCall((chunk, encoding, cb) => {
w.on('close', common.mustCall(() => {
cb();
}));
})
});

w.on('finish', common.mustNotCall());
w.end('asd');
w.destroy();
}

{
const w = new Writable({
write: common.mustCall((chunk, encoding, cb) => {
w.on('close', common.mustCall(() => {
cb();
w.end();
}));
})
});

w.on('finish', common.mustNotCall());
w.write('asd');
w.destroy();
}

1 comment on commit 3c07b17

@ronag
Copy link
Member Author

@ronag ronag commented on 3c07b17 Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpinca @mcollina @nodejs/streams

This fails ws on v14 since it is missing #31806 which fixes a bug in net.Socket where the socket is destroyed before 'finish' has been emitted.

With this change and without #31806, 'finish' will never be emitted even though it should. I guess being emitted with the wrong timing is better than not at all.

I think we can fix net.Socket in way that is landable on v14.

Please sign in to comment.