From 2b83ff1dfb6169eb344f4a229aa469f836013db2 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 19 Apr 2020 23:28:58 +0200 Subject: [PATCH 1/6] stream: don't emit finish after close Writable stream could emit 'finish' after 'close'. --- lib/_stream_readable.js | 4 +++ lib/_stream_writable.js | 7 +++- lib/internal/streams/destroy.js | 17 +++++++--- .../test-stream-writable-finish-destroyed.js | 33 +++++++++++++++++++ 4 files changed, 56 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-stream-writable-finish-destroyed.js diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 9a5b4c3c1b35c6..6e6481316a1134 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -153,6 +153,10 @@ function ReadableState(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; + // Crypto is kind of old and crusty. Historically, its default string // encoding is 'binary' so we have to make this configurable. // Everything else in the universe uses 'utf8', though. diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 48050f776c7552..d221b413931dd0 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -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) { @@ -653,7 +657,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; // TODO(ronag): This could occur after 'close' is emitted. diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index 207074a0c2c134..58dbc4ccd8914e 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -73,6 +73,13 @@ function emitCloseNT(self) { const r = self._readableState; const w = self._writableState; + if (w) { + w.closeEmitted = true; + } + if (r) { + r.closeEmitted = true; + } + if ((w && w.emitClose) || (r && r.emitClose)) { self.emit('close'); } @@ -101,25 +108,27 @@ function undestroy() { const w = this._writableState; if (r) { - r.closed = false; r.destroyed = false; + r.closed = false; + r.closeEmitted = false; r.errored = false; + r.errorEmitted = false; r.reading = false; r.ended = false; r.endEmitted = false; - r.errorEmitted = false; } 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; } } diff --git a/test/parallel/test-stream-writable-finish-destroyed.js b/test/parallel/test-stream-writable-finish-destroyed.js new file mode 100644 index 00000000000000..013155cb9aba62 --- /dev/null +++ b/test/parallel/test-stream-writable-finish-destroyed.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const { Writable } = require('stream'); + +{ + const w = new Writable({ + write(chunk, encoding, cb) { + w.on('close', () => { + cb(); + }); + } + }); + + w.end('asd'); + w.destroy(); + w.on('finish', common.mustNotCall()); +} + +{ + const w = new Writable({ + write(chunk, encoding, cb) { + w.on('close', () => { + cb(); + w.end(); + }); + } + }); + + w.write('asd'); + w.destroy(); + w.on('finish', common.mustNotCall()); +} From 4f1e2bce93d9d856fc2e3cd186de430d96033adc Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 22 Apr 2020 23:10:31 +0200 Subject: [PATCH 2/6] fixup: move on before destroy --- test/parallel/test-stream-writable-finish-destroyed.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-stream-writable-finish-destroyed.js b/test/parallel/test-stream-writable-finish-destroyed.js index 013155cb9aba62..fff202325fd0cd 100644 --- a/test/parallel/test-stream-writable-finish-destroyed.js +++ b/test/parallel/test-stream-writable-finish-destroyed.js @@ -27,7 +27,7 @@ const { Writable } = require('stream'); } }); + w.on('finish', common.mustNotCall()); w.write('asd'); w.destroy(); - w.on('finish', common.mustNotCall()); } From 7da4656e2880eb4a702f2c213a015771c76c03ce Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 25 Apr 2020 19:35:08 +0200 Subject: [PATCH 3/6] fixup: mustCall --- .../test-stream-writable-finish-destroyed.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/parallel/test-stream-writable-finish-destroyed.js b/test/parallel/test-stream-writable-finish-destroyed.js index fff202325fd0cd..138e5d68613854 100644 --- a/test/parallel/test-stream-writable-finish-destroyed.js +++ b/test/parallel/test-stream-writable-finish-destroyed.js @@ -5,11 +5,11 @@ const { Writable } = require('stream'); { const w = new Writable({ - write(chunk, encoding, cb) { - w.on('close', () => { + write: common.mustCall((chunk, encoding, cb) => { + w.on('close', common.mustCall(() => { cb(); - }); - } + })); + }) }); w.end('asd'); @@ -19,12 +19,12 @@ const { Writable } = require('stream'); { const w = new Writable({ - write(chunk, encoding, cb) { - w.on('close', () => { + write: common.mustCall((chunk, encoding, cb) => { + w.on('close', common.mustCall(() => { cb(); w.end(); - }); - } + })); + }) }); w.on('finish', common.mustNotCall()); From b544f97d6b5dfc50c284587a6bfabde0009f19db Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 25 Apr 2020 23:03:59 +0200 Subject: [PATCH 4/6] fixup: move on 'finish' --- test/parallel/test-stream-writable-finish-destroyed.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-stream-writable-finish-destroyed.js b/test/parallel/test-stream-writable-finish-destroyed.js index 138e5d68613854..207b4aa11abfae 100644 --- a/test/parallel/test-stream-writable-finish-destroyed.js +++ b/test/parallel/test-stream-writable-finish-destroyed.js @@ -12,9 +12,9 @@ const { Writable } = require('stream'); }) }); + w.on('finish', common.mustNotCall()); w.end('asd'); w.destroy(); - w.on('finish', common.mustNotCall()); } { From 706e1897e5d102d59f53379e375659064b6ca4cb Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 26 Apr 2020 11:05:19 +0200 Subject: [PATCH 5/6] fixup: closeEmitted not needed on readable, yet --- lib/_stream_readable.js | 4 ---- lib/internal/streams/destroy.js | 8 ++------ 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 6e6481316a1134..9a5b4c3c1b35c6 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -153,10 +153,6 @@ function ReadableState(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; - // Crypto is kind of old and crusty. Historically, its default string // encoding is 'binary' so we have to make this configurable. // Everything else in the universe uses 'utf8', though. diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index 58dbc4ccd8914e..f17034c3b7aac5 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -76,9 +76,6 @@ function emitCloseNT(self) { if (w) { w.closeEmitted = true; } - if (r) { - r.closeEmitted = true; - } if ((w && w.emitClose) || (r && r.emitClose)) { self.emit('close'); @@ -108,14 +105,13 @@ function undestroy() { const w = this._writableState; if (r) { - r.destroyed = false; r.closed = false; - r.closeEmitted = false; + r.destroyed = false; r.errored = false; - r.errorEmitted = false; r.reading = false; r.ended = false; r.endEmitted = false; + r.errorEmitted = false; } if (w) { From d8b3d590ad6136c6986fc471cd5bd69b8119c39c Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 26 Apr 2020 11:10:26 +0200 Subject: [PATCH 6/6] fixup: remove fixed TODO --- lib/_stream_writable.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index d221b413931dd0..ad125aa334edc3 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -661,8 +661,6 @@ function finish(stream, state) { if (state.errorEmitted || state.closeEmitted) return; - // TODO(ronag): This could occur after 'close' is emitted. - state.finished = true; stream.emit('finish');