From f09f5306c5cafa496fadeb057cdd3f777969fc1d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 18 Nov 2021 07:50:29 +0100 Subject: [PATCH] stream: don't emit finish after destroy PR-URL: https://github.com/nodejs/node/pull/40852 Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- lib/internal/streams/writable.js | 16 ++++++++++++---- test/parallel/test-http2-client-upload.js | 3 +-- test/parallel/test-http2-compat-socket-set.js | 10 ---------- test/parallel/test-stream-duplex-destroy.js | 2 +- test/parallel/test-stream-transform-destroy.js | 2 +- test/parallel/test-stream-writable-destroy.js | 2 -- .../test-stream-writable-finish-destroyed.js | 10 ++++++++++ test/parallel/test-stream-write-destroy.js | 6 ------ 8 files changed, 25 insertions(+), 26 deletions(-) diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index 7c082c075642eb..1720a2771365c1 100644 --- 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-http2-client-upload.js b/test/parallel/test-http2-client-upload.js index f7f170126c1557..d073cd94e6ee96 100644 --- a/test/parallel/test-http2-client-upload.js +++ b/test/parallel/test-http2-client-upload.js @@ -22,7 +22,7 @@ fs.readFile(loc, common.mustSucceed((data) => { const server = http2.createServer(); let client; - const countdown = new Countdown(3, () => { + const countdown = new Countdown(2, () => { server.close(); client.close(); }); @@ -50,7 +50,6 @@ fs.readFile(loc, common.mustSucceed((data) => { req.resume(); req.on('end', common.mustCall()); - req.on('finish', () => countdown.dec()); const str = fs.createReadStream(loc); str.on('end', common.mustCall()); str.on('close', () => countdown.dec()); diff --git a/test/parallel/test-http2-compat-socket-set.js b/test/parallel/test-http2-compat-socket-set.js index 30bddc6df59030..e8b804858d65bd 100644 --- a/test/parallel/test-http2-compat-socket-set.js +++ b/test/parallel/test-http2-compat-socket-set.js @@ -73,16 +73,6 @@ server.on('request', common.mustCall(function(request, response) { assert.throws(() => request.socket.pause = noop, errMsg); assert.throws(() => request.socket.resume = noop, errMsg); - request.stream.on('finish', common.mustCall(() => { - setImmediate(() => { - request.socket.setTimeout = noop; - assert.strictEqual(request.stream.setTimeout, noop); - - assert.strictEqual(request.stream._isProcessing, undefined); - request.socket._isProcessing = true; - assert.strictEqual(request.stream._isProcessing, true); - }); - })); response.stream.destroy(); })); diff --git a/test/parallel/test-stream-duplex-destroy.js b/test/parallel/test-stream-duplex-destroy.js index 1894bb7ad67617..ea6f6d42c90305 100644 --- 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 index c594d9989ae4de..8750c65164e5e0 100644 --- 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 index 910cca4adc21aa..768ad212b37e89 100644 --- 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 index 207b4aa11abfae..22657a170f0087 100644 --- 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 index 1acf45a9ab2781..d436b98f84d09b 100644 --- a/test/parallel/test-stream-write-destroy.js +++ b/test/parallel/test-stream-write-destroy.js @@ -20,9 +20,7 @@ for (const withPendingData of [ false, true ]) { let chunksWritten = 0; let drains = 0; - let finished = false; w.on('drain', () => drains++); - w.on('finish', () => finished = true); function onWrite(err) { if (err) { @@ -60,9 +58,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); } }