Skip to content

Commit

Permalink
stream: don't emit finish after destroy
Browse files Browse the repository at this point in the history
PR-URL: #40852
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
ronag committed Jan 3, 2022
1 parent f1658bd commit 48e7840
Show file tree
Hide file tree
Showing 8 changed files with 25 additions and 26 deletions.
16 changes: 12 additions & 4 deletions lib/internal/streams/writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down Expand Up @@ -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);
}
}
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-http2-client-upload.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down Expand Up @@ -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());
Expand Down
10 changes: 0 additions & 10 deletions test/parallel/test-http2-compat-socket-set.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}));

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-stream-duplex-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-stream-transform-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

{
Expand Down
2 changes: 0 additions & 2 deletions test/parallel/test-stream-writable-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,6 @@ const assert = require('assert');

write.destroy();

write.removeListener('finish', fail);
write.on('finish', common.mustCall());
assert.strictEqual(write.destroyed, true);
}

Expand Down
10 changes: 10 additions & 0 deletions test/parallel/test-stream-writable-finish-destroyed.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
6 changes: 0 additions & 6 deletions test/parallel/test-stream-write-destroy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
}

0 comments on commit 48e7840

Please sign in to comment.