From 71b7f733e8b924fac6ff019b5a326171eee6f2c2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 21 Mar 2018 13:01:41 +0100 Subject: [PATCH 1/3] stream: give error message if `write()` cb called twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise, this condition would result in an error that just reads `cb is not a function`, and which additionally could have lost stack trace context through a `process.nextTick()` call. $ ./node benchmark/compare.js --new ./node --old ./node-master --filter writable --runs 200 streams | Rscript benchmark/compare.R [00:09:36|% 100| 1/1 files | 400/400 runs | 1/1 configs]: Done confidence improvement accuracy (*) (**) (***) streams/writable-manywrites.js n=2000000 -0.28 % ±3.51% ±4.62% ±5.92% --- lib/_stream_writable.js | 4 ++ lib/internal/errors.js | 1 + .../test-stream-writable-write-cb-twice.js | 49 +++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 test/parallel/test-stream-writable-write-cb-twice.js diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 5f9bf79ae41a26..cb3083e55d3735 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -41,6 +41,7 @@ const { ERR_STREAM_DESTROYED, ERR_STREAM_NULL_VALUES, ERR_STREAM_WRITE_AFTER_END, + ERR_STREAM_WRITE_CALLBACK_TWICE, ERR_UNKNOWN_ENCODING } = require('internal/errors').codes; @@ -445,6 +446,9 @@ function onwrite(stream, er) { var sync = state.sync; var cb = state.writecb; + if (typeof cb !== 'function') + throw new ERR_STREAM_WRITE_CALLBACK_TWICE(); + onwriteStateUpdate(state); if (er) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 33a3185f797ca9..c87c1de9fe4f64 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -852,6 +852,7 @@ E('ERR_STREAM_UNSHIFT_AFTER_END_EVENT', 'stream.unshift() after end event', Error); E('ERR_STREAM_WRAP', 'Stream has StringDecoder set or is in objectMode', Error); E('ERR_STREAM_WRITE_AFTER_END', 'write after end', Error); +E('ERR_STREAM_WRITE_CALLBACK_TWICE', '_write() callback called twice', Error); E('ERR_TLS_CERT_ALTNAME_INVALID', 'Hostname/IP does not match certificate\'s altnames: %s', Error); E('ERR_TLS_DH_PARAM_SIZE', 'DH parameter size %s is less than 2048', Error); diff --git a/test/parallel/test-stream-writable-write-cb-twice.js b/test/parallel/test-stream-writable-write-cb-twice.js new file mode 100644 index 00000000000000..754e684c47db12 --- /dev/null +++ b/test/parallel/test-stream-writable-write-cb-twice.js @@ -0,0 +1,49 @@ +'use strict'; +const common = require('../common'); +const { Writable } = require('stream'); + +{ + // Sync + Sync + const writable = new Writable({ + write: common.mustCall((buf, enc, cb) => { + cb(); + common.expectsError(cb, { + code: 'ERR_STREAM_WRITE_CALLBACK_TWICE', + type: Error + }); + }) + }); + writable.write('hi'); +} + +{ + // Sync + Async + const writable = new Writable({ + write: common.mustCall((buf, enc, cb) => { + cb(); + process.nextTick(() => { + common.expectsError(cb, { + code: 'ERR_STREAM_WRITE_CALLBACK_TWICE', + type: Error + }); + }); + }) + }); + writable.write('hi'); +} + +{ + // Async + Async + const writable = new Writable({ + write: common.mustCall((buf, enc, cb) => { + process.nextTick(cb); + process.nextTick(() => { + common.expectsError(cb, { + code: 'ERR_STREAM_WRITE_CALLBACK_TWICE', + type: Error + }); + }); + }) + }); + writable.write('hi'); +} From dccdccc01def3129fe717d3f38c858965a544ca6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 21 Mar 2018 14:38:26 +0100 Subject: [PATCH 2/3] [squash] use ERR_MULTIPLE_CALLBACK --- lib/_stream_writable.js | 4 ++-- test/parallel/test-stream-writable-write-cb-twice.js | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index cb3083e55d3735..c3faf0e1346d90 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -37,11 +37,11 @@ const { getHighWaterMark } = require('internal/streams/state'); const { ERR_INVALID_ARG_TYPE, ERR_METHOD_NOT_IMPLEMENTED, + ERR_MULTIPLE_CALLBACK, ERR_STREAM_CANNOT_PIPE, ERR_STREAM_DESTROYED, ERR_STREAM_NULL_VALUES, ERR_STREAM_WRITE_AFTER_END, - ERR_STREAM_WRITE_CALLBACK_TWICE, ERR_UNKNOWN_ENCODING } = require('internal/errors').codes; @@ -447,7 +447,7 @@ function onwrite(stream, er) { var cb = state.writecb; if (typeof cb !== 'function') - throw new ERR_STREAM_WRITE_CALLBACK_TWICE(); + throw new ERR_MULTIPLE_CALLBACK(); onwriteStateUpdate(state); diff --git a/test/parallel/test-stream-writable-write-cb-twice.js b/test/parallel/test-stream-writable-write-cb-twice.js index 754e684c47db12..f066a0ecc4f250 100644 --- a/test/parallel/test-stream-writable-write-cb-twice.js +++ b/test/parallel/test-stream-writable-write-cb-twice.js @@ -8,7 +8,7 @@ const { Writable } = require('stream'); write: common.mustCall((buf, enc, cb) => { cb(); common.expectsError(cb, { - code: 'ERR_STREAM_WRITE_CALLBACK_TWICE', + code: 'ERR_MULTIPLE_CALLBACK', type: Error }); }) @@ -23,7 +23,7 @@ const { Writable } = require('stream'); cb(); process.nextTick(() => { common.expectsError(cb, { - code: 'ERR_STREAM_WRITE_CALLBACK_TWICE', + code: 'ERR_MULTIPLE_CALLBACK', type: Error }); }); @@ -39,7 +39,7 @@ const { Writable } = require('stream'); process.nextTick(cb); process.nextTick(() => { common.expectsError(cb, { - code: 'ERR_STREAM_WRITE_CALLBACK_TWICE', + code: 'ERR_MULTIPLE_CALLBACK', type: Error }); }); From fc665d32a66fb8a354334b9d2ddcd87565952964 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 21 Mar 2018 14:41:38 +0100 Subject: [PATCH 3/3] [squash] fix --- lib/internal/errors.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index c87c1de9fe4f64..33a3185f797ca9 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -852,7 +852,6 @@ E('ERR_STREAM_UNSHIFT_AFTER_END_EVENT', 'stream.unshift() after end event', Error); E('ERR_STREAM_WRAP', 'Stream has StringDecoder set or is in objectMode', Error); E('ERR_STREAM_WRITE_AFTER_END', 'write after end', Error); -E('ERR_STREAM_WRITE_CALLBACK_TWICE', '_write() callback called twice', Error); E('ERR_TLS_CERT_ALTNAME_INVALID', 'Hostname/IP does not match certificate\'s altnames: %s', Error); E('ERR_TLS_DH_PARAM_SIZE', 'DH parameter size %s is less than 2048', Error);