Skip to content

Commit ba3be57

Browse files
ronagmcollina
ronag
authored and
mcollina
committed
stream: don't emit finish on error
After 'error' only 'close' should be emitted. PR-URL: #28979 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent dc7c7b8 commit ba3be57

File tree

6 files changed

+49
-78
lines changed

6 files changed

+49
-78
lines changed

‎doc/api/stream.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2425,7 +2425,8 @@ and `stream.Readable` classes, respectively. The `'finish'` event is emitted
24252425
after [`stream.end()`][stream-end] is called and all chunks have been processed
24262426
by [`stream._transform()`][stream-_transform]. The `'end'` event is emitted
24272427
after all data has been output, which occurs after the callback in
2428-
[`transform._flush()`][stream-_flush] has been called.
2428+
[`transform._flush()`][stream-_flush] has been called. In the case of an error,
2429+
neither `'finish'` nor `'end'` should be emitted.
24292430

24302431
#### transform.\_flush(callback)
24312432

‎lib/_stream_writable.js‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -437,19 +437,12 @@ function onwriteError(stream, state, sync, er, cb) {
437437
// Defer the callback if we are being called synchronously
438438
// to avoid piling up things on the stack
439439
process.nextTick(cb, er);
440-
// This can emit finish, and it will always happen
441-
// after error
442-
process.nextTick(finishMaybe, stream, state);
443-
errorOrDestroy(stream, er);
444440
} else {
445441
// The caller expect this to happen before if
446442
// it is async
447443
cb(er);
448-
errorOrDestroy(stream, er);
449-
// This can emit finish, but finish must
450-
// always follow error
451-
finishMaybe(stream, state);
452444
}
445+
errorOrDestroy(stream, er);
453446
}
454447

455448
function onwrite(stream, er) {
@@ -618,6 +611,7 @@ Object.defineProperty(Writable.prototype, 'writableLength', {
618611
function needFinish(state) {
619612
return (state.ending &&
620613
state.length === 0 &&
614+
!state.errorEmitted &&
621615
state.bufferedRequest === null &&
622616
!state.finished &&
623617
!state.writing);

‎test/parallel/test-net-connect-buffer.js‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -51,34 +51,6 @@ tcp.listen(0, common.mustCall(function() {
5151
assert.strictEqual(socket.connecting, true);
5252
assert.strictEqual(socket.readyState, 'opening');
5353

54-
// Make sure that anything besides a buffer or a string throws.
55-
common.expectsError(() => socket.write(null),
56-
{
57-
code: 'ERR_STREAM_NULL_VALUES',
58-
type: TypeError,
59-
message: 'May not write null values to stream'
60-
});
61-
[
62-
true,
63-
false,
64-
undefined,
65-
1,
66-
1.0,
67-
+Infinity,
68-
-Infinity,
69-
[],
70-
{}
71-
].forEach((value) => {
72-
// We need to check the callback since 'error' will only
73-
// be emitted once per instance.
74-
socket.write(value, common.expectsError({
75-
code: 'ERR_INVALID_ARG_TYPE',
76-
type: TypeError,
77-
message: 'The "chunk" argument must be one of type string or Buffer. ' +
78-
`Received type ${typeof value}`
79-
}));
80-
});
81-
8254
// Write a string that contains a multi-byte character sequence to test that
8355
// `bytesWritten` is incremented with the # of bytes, not # of characters.
8456
const a = "L'État, c'est ";

‎test/parallel/test-net-socket-write-error.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ function connectToServer() {
1313
type: TypeError
1414
});
1515

16-
client.end();
16+
client.destroy();
1717
})
18-
.on('end', () => server.close());
18+
.on('close', () => server.close());
1919
}

‎test/parallel/test-net-write-arguments.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
'use strict';
2+
const common = require('../common');
3+
const net = require('net');
4+
5+
const socket = net.Stream({ highWaterMark: 0 });
6+
7+
// Make sure that anything besides a buffer or a string throws.
8+
common.expectsError(() => socket.write(null),
9+
{
10+
code: 'ERR_STREAM_NULL_VALUES',
11+
type: TypeError,
12+
message: 'May not write null values to stream'
13+
});
14+
[
15+
true,
16+
false,
17+
undefined,
18+
1,
19+
1.0,
20+
+Infinity,
21+
-Infinity,
22+
[],
23+
{}
24+
].forEach((value) => {
25+
// We need to check the callback since 'error' will only
26+
// be emitted once per instance.
27+
socket.write(value, common.expectsError({
28+
code: 'ERR_INVALID_ARG_TYPE',
29+
type: TypeError,
30+
message: 'The "chunk" argument must be one of type string or Buffer. ' +
31+
`Received type ${typeof value}`
32+
}));
33+
});

‎test/parallel/test-stream-writable-write-writev-finish.js‎

Lines changed: 10 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,10 @@ const stream = require('stream');
1414
cb(new Error('write test error'));
1515
};
1616

17-
let firstError = false;
18-
writable.on('finish', common.mustCall(function() {
19-
assert.strictEqual(firstError, true);
20-
}));
21-
22-
writable.on('prefinish', common.mustCall());
23-
17+
writable.on('finish', common.mustNotCall());
18+
writable.on('prefinish', common.mustNotCall());
2419
writable.on('error', common.mustCall((er) => {
2520
assert.strictEqual(er.message, 'write test error');
26-
firstError = true;
2721
}));
2822

2923
writable.end('test');
@@ -36,16 +30,10 @@ const stream = require('stream');
3630
setImmediate(cb, new Error('write test error'));
3731
};
3832

39-
let firstError = false;
40-
writable.on('finish', common.mustCall(function() {
41-
assert.strictEqual(firstError, true);
42-
}));
43-
44-
writable.on('prefinish', common.mustCall());
45-
33+
writable.on('finish', common.mustNotCall());
34+
writable.on('prefinish', common.mustNotCall());
4635
writable.on('error', common.mustCall((er) => {
4736
assert.strictEqual(er.message, 'write test error');
48-
firstError = true;
4937
}));
5038

5139
writable.end('test');
@@ -62,16 +50,10 @@ const stream = require('stream');
6250
cb(new Error('writev test error'));
6351
};
6452

65-
let firstError = false;
66-
writable.on('finish', common.mustCall(function() {
67-
assert.strictEqual(firstError, true);
68-
}));
69-
70-
writable.on('prefinish', common.mustCall());
71-
53+
writable.on('finish', common.mustNotCall());
54+
writable.on('prefinish', common.mustNotCall());
7255
writable.on('error', common.mustCall((er) => {
7356
assert.strictEqual(er.message, 'writev test error');
74-
firstError = true;
7557
}));
7658

7759
writable.cork();
@@ -93,16 +75,10 @@ const stream = require('stream');
9375
setImmediate(cb, new Error('writev test error'));
9476
};
9577

96-
let firstError = false;
97-
writable.on('finish', common.mustCall(function() {
98-
assert.strictEqual(firstError, true);
99-
}));
100-
101-
writable.on('prefinish', common.mustCall());
102-
78+
writable.on('finish', common.mustNotCall());
79+
writable.on('prefinish', common.mustNotCall());
10380
writable.on('error', common.mustCall((er) => {
10481
assert.strictEqual(er.message, 'writev test error');
105-
firstError = true;
10682
}));
10783

10884
writable.cork();
@@ -123,14 +99,9 @@ const stream = require('stream');
12399
rs._read = () => {};
124100

125101
const ws = new stream.Writable();
126-
let firstError = false;
127102

128-
ws.on('finish', common.mustCall(function() {
129-
assert.strictEqual(firstError, true);
130-
}));
131-
ws.on('error', common.mustCall(function() {
132-
firstError = true;
133-
}));
103+
ws.on('finish', common.mustNotCall());
104+
ws.on('error', common.mustCall());
134105

135106
ws._write = (chunk, encoding, done) => {
136107
setImmediate(done, new Error());

0 commit comments

Comments
 (0)