From 1d9c349d473a9b4dd66ab97bc0f1cb2cd491cda8 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 5 Aug 2019 16:07:49 +0200 Subject: [PATCH 1/7] stream: don't emit finish on error After 'error' only 'close' should be emitted. --- lib/_stream_writable.js | 10 +---- ...est-stream-writable-write-writev-finish.js | 45 +++++-------------- 2 files changed, 12 insertions(+), 43 deletions(-) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index e212881c4ac555..5eef75aa646c23 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -437,19 +437,12 @@ function onwriteError(stream, state, sync, er, cb) { // Defer the callback if we are being called synchronously // to avoid piling up things on the stack process.nextTick(cb, er); - // This can emit finish, and it will always happen - // after error - process.nextTick(finishMaybe, stream, state); - errorOrDestroy(stream, er); } else { // The caller expect this to happen before if // it is async cb(er); - errorOrDestroy(stream, er); - // This can emit finish, but finish must - // always follow error - finishMaybe(stream, state); } + errorOrDestroy(stream, er); } function onwrite(stream, er) { @@ -619,6 +612,7 @@ function needFinish(state) { return (state.ending && state.length === 0 && state.bufferedRequest === null && + !state.errorEmitted && !state.finished && !state.writing); } diff --git a/test/parallel/test-stream-writable-write-writev-finish.js b/test/parallel/test-stream-writable-write-writev-finish.js index 4399f1ca503b37..5f870932bc0167 100644 --- a/test/parallel/test-stream-writable-write-writev-finish.js +++ b/test/parallel/test-stream-writable-write-writev-finish.js @@ -14,16 +14,11 @@ const stream = require('stream'); cb(new Error('write test error')); }; - let firstError = false; - writable.on('finish', common.mustCall(function() { - assert.strictEqual(firstError, true); - })); - - writable.on('prefinish', common.mustCall()); + writable.on('finish', common.mustNotCall()); + writable.on('prefinish', common.mustNotCall()); writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'write test error'); - firstError = true; })); writable.end('test'); @@ -36,16 +31,11 @@ const stream = require('stream'); setImmediate(cb, new Error('write test error')); }; - let firstError = false; - writable.on('finish', common.mustCall(function() { - assert.strictEqual(firstError, true); - })); - - writable.on('prefinish', common.mustCall()); + writable.on('finish', common.mustNotCall()); + writable.on('prefinish', common.mustNotCall()); writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'write test error'); - firstError = true; })); writable.end('test'); @@ -62,16 +52,11 @@ const stream = require('stream'); cb(new Error('writev test error')); }; - let firstError = false; - writable.on('finish', common.mustCall(function() { - assert.strictEqual(firstError, true); - })); - - writable.on('prefinish', common.mustCall()); + writable.on('finish', common.mustNotCall()); + writable.on('prefinish', common.mustNotCall()); writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'writev test error'); - firstError = true; })); writable.cork(); @@ -93,16 +78,11 @@ const stream = require('stream'); setImmediate(cb, new Error('writev test error')); }; - let firstError = false; - writable.on('finish', common.mustCall(function() { - assert.strictEqual(firstError, true); - })); - - writable.on('prefinish', common.mustCall()); + writable.on('finish', common.mustNotCall()); + writable.on('prefinish', common.mustNotCall()); writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'writev test error'); - firstError = true; })); writable.cork(); @@ -123,14 +103,9 @@ const stream = require('stream'); rs._read = () => {}; const ws = new stream.Writable(); - let firstError = false; - ws.on('finish', common.mustCall(function() { - assert.strictEqual(firstError, true); - })); - ws.on('error', common.mustCall(function() { - firstError = true; - })); + ws.on('finish', common.mustNotCall()); + ws.on('error', common.mustCall()); ws._write = (chunk, encoding, done) => { setImmediate(done, new Error()); From 2ab23999d7d2b289fbeab8ea2e4c8a8d30999da0 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 23 Aug 2019 19:53:04 +0200 Subject: [PATCH 2/7] fixup --- lib/_stream_writable.js | 1 - .../test-stream-writable-write-writev-finish.js | 16 +++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 5eef75aa646c23..b3cacc1ec6844a 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -612,7 +612,6 @@ function needFinish(state) { return (state.ending && state.length === 0 && state.bufferedRequest === null && - !state.errorEmitted && !state.finished && !state.writing); } diff --git a/test/parallel/test-stream-writable-write-writev-finish.js b/test/parallel/test-stream-writable-write-writev-finish.js index 5f870932bc0167..65ebaa2ed9b48b 100644 --- a/test/parallel/test-stream-writable-write-writev-finish.js +++ b/test/parallel/test-stream-writable-write-writev-finish.js @@ -14,11 +14,16 @@ const stream = require('stream'); cb(new Error('write test error')); }; - writable.on('finish', common.mustNotCall()); - writable.on('prefinish', common.mustNotCall()); + let firstError = false; + writable.on('finish', common.mustCall(function() { + assert.strictEqual(firstError, true); + })); + + writable.on('prefinish', common.mustCall()); writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'write test error'); + firstError = true; })); writable.end('test'); @@ -33,10 +38,7 @@ const stream = require('stream'); writable.on('finish', common.mustNotCall()); writable.on('prefinish', common.mustNotCall()); - - writable.on('error', common.mustCall((er) => { - assert.strictEqual(er.message, 'write test error'); - })); + writable.on('error', common.mustCall()); writable.end('test'); } @@ -53,7 +55,7 @@ const stream = require('stream'); }; writable.on('finish', common.mustNotCall()); - writable.on('prefinish', common.mustNotCall()); + writable.on('prefinish', common.mustCall()); writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'writev test error'); From 102b23258d3c0469d7c84b93e02cbd27e91c5e0a Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 24 Aug 2019 12:55:11 +0200 Subject: [PATCH 3/7] fixup: check errorEmitted --- lib/_stream_writable.js | 1 + ...test-stream-writable-write-writev-finish.js | 18 ++++++------------ 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index b3cacc1ec6844a..4cb3be5c008e9e 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -611,6 +611,7 @@ Object.defineProperty(Writable.prototype, 'writableLength', { function needFinish(state) { return (state.ending && state.length === 0 && + !state.errorEmitted && state.bufferedRequest === null && !state.finished && !state.writing); diff --git a/test/parallel/test-stream-writable-write-writev-finish.js b/test/parallel/test-stream-writable-write-writev-finish.js index 65ebaa2ed9b48b..aa43b1490c8600 100644 --- a/test/parallel/test-stream-writable-write-writev-finish.js +++ b/test/parallel/test-stream-writable-write-writev-finish.js @@ -14,16 +14,10 @@ const stream = require('stream'); cb(new Error('write test error')); }; - let firstError = false; - writable.on('finish', common.mustCall(function() { - assert.strictEqual(firstError, true); - })); - - writable.on('prefinish', common.mustCall()); - + writable.on('finish', common.mustNotCall()); + writable.on('prefinish', common.mustNotCall()); writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'write test error'); - firstError = true; })); writable.end('test'); @@ -38,7 +32,9 @@ const stream = require('stream'); writable.on('finish', common.mustNotCall()); writable.on('prefinish', common.mustNotCall()); - writable.on('error', common.mustCall()); + writable.on('error', common.mustCall((er) => { + assert.strictEqual(er.message, 'write test error'); + })); writable.end('test'); } @@ -55,8 +51,7 @@ const stream = require('stream'); }; writable.on('finish', common.mustNotCall()); - writable.on('prefinish', common.mustCall()); - + writable.on('prefinish', common.mustNotCall()); writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'writev test error'); })); @@ -82,7 +77,6 @@ const stream = require('stream'); writable.on('finish', common.mustNotCall()); writable.on('prefinish', common.mustNotCall()); - writable.on('error', common.mustCall((er) => { assert.strictEqual(er.message, 'writev test error'); })); From 43b1537b55db8909ffbbbf2be0f27dd91222930e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 24 Aug 2019 13:21:46 +0200 Subject: [PATCH 4/7] fixup --- test/parallel/test-net-connect-buffer.js | 28 ----------------- test/parallel/test-net-connect-buffer2.js | 33 ++++++++++++++++++++ test/parallel/test-net-socket-write-error.js | 4 +-- 3 files changed, 35 insertions(+), 30 deletions(-) create mode 100644 test/parallel/test-net-connect-buffer2.js diff --git a/test/parallel/test-net-connect-buffer.js b/test/parallel/test-net-connect-buffer.js index 41df6a06662400..749eee519904f5 100644 --- a/test/parallel/test-net-connect-buffer.js +++ b/test/parallel/test-net-connect-buffer.js @@ -51,34 +51,6 @@ tcp.listen(0, common.mustCall(function() { assert.strictEqual(socket.connecting, true); assert.strictEqual(socket.readyState, 'opening'); - // Make sure that anything besides a buffer or a string throws. - common.expectsError(() => socket.write(null), - { - code: 'ERR_STREAM_NULL_VALUES', - type: TypeError, - message: 'May not write null values to stream' - }); - [ - true, - false, - undefined, - 1, - 1.0, - +Infinity, - -Infinity, - [], - {} - ].forEach((value) => { - // We need to check the callback since 'error' will only - // be emitted once per instance. - socket.write(value, common.expectsError({ - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "chunk" argument must be one of type string or Buffer. ' + - `Received type ${typeof value}` - })); - }); - // Write a string that contains a multi-byte character sequence to test that // `bytesWritten` is incremented with the # of bytes, not # of characters. const a = "L'État, c'est "; diff --git a/test/parallel/test-net-connect-buffer2.js b/test/parallel/test-net-connect-buffer2.js new file mode 100644 index 00000000000000..19b037ee0fc94c --- /dev/null +++ b/test/parallel/test-net-connect-buffer2.js @@ -0,0 +1,33 @@ +'use strict'; +const common = require('../common'); +const net = require('net'); + +const socket = net.Stream({ highWaterMark: 0 }); + +// Make sure that anything besides a buffer or a string throws. +common.expectsError(() => socket.write(null), + { + code: 'ERR_STREAM_NULL_VALUES', + type: TypeError, + message: 'May not write null values to stream' + }); +[ + true, + false, + undefined, + 1, + 1.0, + +Infinity, + -Infinity, + [], + {} +].forEach((value) => { + // We need to check the callback since 'error' will only + // be emitted once per instance. + socket.write(value, common.expectsError({ + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "chunk" argument must be one of type string or Buffer. ' + + `Received type ${typeof value}` + })); +}); diff --git a/test/parallel/test-net-socket-write-error.js b/test/parallel/test-net-socket-write-error.js index 800d6020da4cbe..178cebe9903cfb 100644 --- a/test/parallel/test-net-socket-write-error.js +++ b/test/parallel/test-net-socket-write-error.js @@ -13,7 +13,7 @@ function connectToServer() { type: TypeError }); - client.end(); + client.destroy(); }) - .on('end', () => server.close()); + .on('close', () => server.close()); } From c4a2047e8a5fbca36c373029eb7e53f681aaf3ae Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 30 Aug 2019 19:42:05 +0200 Subject: [PATCH 5/7] fixup --- .../{test-net-connect-buffer2.js => test-net-write-arguments.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/parallel/{test-net-connect-buffer2.js => test-net-write-arguments.js} (100%) diff --git a/test/parallel/test-net-connect-buffer2.js b/test/parallel/test-net-write-arguments.js similarity index 100% rename from test/parallel/test-net-connect-buffer2.js rename to test/parallel/test-net-write-arguments.js From 62d16ec413618e2d548acc6a81fd566ff38b73eb Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 11 Sep 2019 21:29:41 +0200 Subject: [PATCH 6/7] fixup: docs --- doc/api/stream.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index d670c7bc6fe381..83c59b81b3eede 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -2413,7 +2413,8 @@ and `stream.Readable` classes, respectively. The `'finish'` event is emitted after [`stream.end()`][stream-end] is called and all chunks have been processed by [`stream._transform()`][stream-_transform]. The `'end'` event is emitted after all data has been output, which occurs after the callback in -[`transform._flush()`][stream-_flush] has been called. +[`transform._flush()`][stream-_flush] has been called. In the case of an error +neither `'finish'` nor `'end'` *should* be emitted. #### transform.\_flush(callback) From 7b33a42de48db62879ad7c95c9860d6ea9bc89a2 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 12 Sep 2019 07:26:24 +0200 Subject: [PATCH 7/7] Apply suggestions from code review Co-Authored-By: Rich Trott --- doc/api/stream.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 83c59b81b3eede..cbdfbd68185c19 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -2413,8 +2413,8 @@ and `stream.Readable` classes, respectively. The `'finish'` event is emitted after [`stream.end()`][stream-end] is called and all chunks have been processed by [`stream._transform()`][stream-_transform]. The `'end'` event is emitted after all data has been output, which occurs after the callback in -[`transform._flush()`][stream-_flush] has been called. In the case of an error -neither `'finish'` nor `'end'` *should* be emitted. +[`transform._flush()`][stream-_flush] has been called. In the case of an error, +neither `'finish'` nor `'end'` should be emitted. #### transform.\_flush(callback)