From 3d893b93e1858e7c8e3b06dc90e9bf31a823bd19 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 31 May 2018 16:00:24 +0200 Subject: [PATCH 1/3] lib: require a callback for end-of-stream Make the callback mandatory as mostly done in all other Node.js callback APIs so users explicitly have to decide what to do in error cases. --- lib/internal/streams/end-of-stream.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/internal/streams/end-of-stream.js b/lib/internal/streams/end-of-stream.js index eeb8a61456a730..454aa030f594f8 100644 --- a/lib/internal/streams/end-of-stream.js +++ b/lib/internal/streams/end-of-stream.js @@ -4,11 +4,10 @@ 'use strict'; const { + ERR_INVALID_ARG_TYPE, ERR_STREAM_PREMATURE_CLOSE } = require('internal/errors').codes; -function noop() {} - function isRequest(stream) { return stream.setHeader && typeof stream.abort === 'function'; } @@ -23,10 +22,20 @@ function once(callback) { } function eos(stream, opts, callback) { - if (typeof opts === 'function') return eos(stream, null, opts); - if (!opts) opts = {}; + if (arguments.length === 2) { + callback = opts; + opts = {}; + } + if (opts == null) { + opts = {}; + } else if (typeof opts !== 'object') { + throw new ERR_INVALID_ARG_TYPE('opts', 'object', opts); + } + if (typeof callback !== 'function') { + throw new ERR_INVALID_ARG_TYPE('callback', 'function', callback); + } - callback = once(callback || noop); + callback = once(callback); const ws = stream._writableState; const rs = stream._readableState; From 48b32f954f5261c4525efb19fa5df7be4cbfa02f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 31 May 2018 16:35:08 +0200 Subject: [PATCH 2/3] doc: add options documentation to Stream.finished() When originally implemented it was missed that Stream.finished() has an optional options object. This adds the docs to those. --- doc/api/stream.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index c42df4ee7c757d..8e7d741fab597b 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1294,12 +1294,21 @@ implementors should not override this method, but instead implement [`readable._destroy()`][readable-_destroy]. The default implementation of `_destroy()` for `Transform` also emit `'close'`. -### stream.finished(stream, callback) +### stream.finished(stream[, options], callback) * `stream` {Stream} A readable and/or writable stream. +* `options` {Object} + * `error` {boolean} If set to `false` a call to emit('error', err) is not + treated finished. **Default**: `true`. + * `readable` {boolean} When set to `false` the callback will be called after + the stream ended but the stream might still be readable. **Default**: + `true`. + * `writable` {boolean} When set to `false` the callback will be called after + the stream ended but the stream might still be writable. **Default**: + `true`. * `callback` {Function} A callback function that takes an optional error argument. From c310236565d3e846e40037d8959d57ac31ddcd1e Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 21 Aug 2018 01:26:50 +0200 Subject: [PATCH 3/3] fixup: address comments & add tests --- doc/api/stream.md | 18 ++++++------- lib/internal/streams/end-of-stream.js | 3 +-- test/parallel/test-stream-finished.js | 38 +++++++++++++++++++++++++-- 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 8e7d741fab597b..f8180fa020321b 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1301,14 +1301,14 @@ added: v10.0.0 * `stream` {Stream} A readable and/or writable stream. * `options` {Object} - * `error` {boolean} If set to `false` a call to emit('error', err) is not - treated finished. **Default**: `true`. - * `readable` {boolean} When set to `false` the callback will be called after - the stream ended but the stream might still be readable. **Default**: - `true`. - * `writable` {boolean} When set to `false` the callback will be called after - the stream ended but the stream might still be writable. **Default**: - `true`. + * `error` {boolean} If set to `false`, then a call to `emit('error', err)` is + not treated as finished. **Default**: `true`. + * `readable` {boolean} When set to `false`, the callback will be called when + the stream ends even though the stream might still be readable. + **Default**: `true`. + * `writable` {boolean} When set to `false`, the callback will be called when + the stream ends even though the stream might still be writable. + **Default**: `true`. * `callback` {Function} A callback function that takes an optional error argument. @@ -2447,7 +2447,7 @@ contain multi-byte characters. [zlib]: zlib.html [hwm-gotcha]: #stream_highwatermark_discrepancy_after_calling_readable_setencoding [pipeline]: #stream_stream_pipeline_streams_callback -[finished]: #stream_stream_finished_stream_callback +[finished]: #stream_stream_finished_stream_options_callback [stream-_flush]: #stream_transform_flush_callback [stream-_read]: #stream_readable_read_size_1 [stream-_transform]: #stream_transform_transform_chunk_encoding_callback diff --git a/lib/internal/streams/end-of-stream.js b/lib/internal/streams/end-of-stream.js index 454aa030f594f8..8bbd4827b23a3a 100644 --- a/lib/internal/streams/end-of-stream.js +++ b/lib/internal/streams/end-of-stream.js @@ -25,8 +25,7 @@ function eos(stream, opts, callback) { if (arguments.length === 2) { callback = opts; opts = {}; - } - if (opts == null) { + } else if (opts == null) { opts = {}; } else if (typeof opts !== 'object') { throw new ERR_INVALID_ARG_TYPE('opts', 'object', opts); diff --git a/test/parallel/test-stream-finished.js b/test/parallel/test-stream-finished.js index 3aade5610c7045..2d7eefc494623b 100644 --- a/test/parallel/test-stream-finished.js +++ b/test/parallel/test-stream-finished.js @@ -91,8 +91,8 @@ const { promisify } = require('util'); { const rs = fs.createReadStream('file-does-not-exist'); - finished(rs, common.mustCall((err) => { - assert.strictEqual(err.code, 'ENOENT'); + finished(rs, common.expectsError({ + code: 'ENOENT' })); } @@ -119,3 +119,37 @@ const { promisify } = require('util'); rs.push(null); rs.resume(); } + +// Test faulty input values and options. +{ + const rs = new Readable({ + read() {} + }); + + assert.throws( + () => finished(rs, 'foo'), + { + name: /ERR_INVALID_ARG_TYPE/, + message: /callback/ + } + ); + assert.throws( + () => finished(rs, 'foo', () => {}), + { + name: /ERR_INVALID_ARG_TYPE/, + message: /opts/ + } + ); + assert.throws( + () => finished(rs, {}, 'foo'), + { + name: /ERR_INVALID_ARG_TYPE/, + message: /callback/ + } + ); + + finished(rs, null, common.mustCall()); + + rs.push(null); + rs.resume(); +}