From 36468ca92862f2e1bc086be7e5e12ec8e012982b Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Thu, 31 May 2018 16:00:24 +0200
Subject: [PATCH] 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.

This also documents the options for `Stream.finished()`.

When originally implemented it was missed that Stream.finished() has
an optional options object.

PR-URL: https://github.com/nodejs/node/pull/21058
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>
---
 doc/api/stream.md                     | 13 +++++++--
 lib/internal/streams/end-of-stream.js | 18 +++++++++----
 test/parallel/test-stream-finished.js | 38 +++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/doc/api/stream.md b/doc/api/stream.md
index c42df4ee7c757d..f8180fa020321b 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)
 <!-- YAML
 added: v10.0.0
 -->
 
 * `stream` {Stream} A readable and/or writable stream.
+* `options` {Object}
+  * `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.
 
@@ -2438,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 eeb8a61456a730..8bbd4827b23a3a 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,19 @@ 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 = {};
+  } else 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;
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();
+}