Skip to content

Commit 32c51f1

Browse files
BridgeARaddaleax
authored andcommitted
stream: make the pipeline callback mandatory
Right now when not adding a callback to the pipeline it could cause an uncaught exception if there is an error. Instead, just make the callback mandatory as mostly done in all other Node.js callback APIs so users explicitly have to decide what to do in such situations. PR-URL: #21054 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 505bfdc commit 32c51f1

File tree

3 files changed

+14
-24
lines changed

3 files changed

+14
-24
lines changed

doc/api/stream.md

+3-3
Original file line numberDiff line numberDiff line change
@@ -1340,14 +1340,14 @@ run().catch(console.error);
13401340
rs.resume(); // drain the stream
13411341
```
13421342

1343-
### stream.pipeline(...streams[, callback])
1343+
### stream.pipeline(...streams, callback)
13441344
<!-- YAML
13451345
added: v10.0.0
13461346
-->
13471347

13481348
* `...streams` {Stream} Two or more streams to pipe between.
1349-
* `callback` {Function} A callback function that takes an optional error
1350-
argument.
1349+
* `callback` {Function} Called when the pipeline is fully done.
1350+
* `err` {Error}
13511351

13521352
A module method to pipe between streams forwarding errors and properly cleaning
13531353
up and provide a callback when the pipeline is complete.

lib/internal/streams/pipeline.js

+6-7
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
let eos;
77

88
const {
9+
ERR_INVALID_CALLBACK,
910
ERR_MISSING_ARGS,
1011
ERR_STREAM_DESTROYED
1112
} = require('internal/errors').codes;
@@ -19,11 +20,6 @@ function once(callback) {
1920
};
2021
}
2122

22-
function noop(err) {
23-
// Rethrow the error if it exists to avoid swallowing it
24-
if (err) throw err;
25-
}
26-
2723
function isRequest(stream) {
2824
return stream.setHeader && typeof stream.abort === 'function';
2925
}
@@ -66,8 +62,11 @@ function pipe(from, to) {
6662
}
6763

6864
function popCallback(streams) {
69-
if (!streams.length) return noop;
70-
if (typeof streams[streams.length - 1] !== 'function') return noop;
65+
// Streams should never be an empty array. It should always contain at least
66+
// a single stream. Therefore optimize for the average case instead of
67+
// checking for length === 0 as well.
68+
if (typeof streams[streams.length - 1] !== 'function')
69+
throw new ERR_INVALID_CALLBACK();
7170
return streams.pop();
7271
}
7372

test/parallel/test-stream-pipeline.js

+5-14
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ common.crashOnUnhandledRejection();
6060
}, /ERR_MISSING_ARGS/);
6161
assert.throws(() => {
6262
pipeline();
63-
}, /ERR_MISSING_ARGS/);
63+
}, /ERR_INVALID_CALLBACK/);
6464
}
6565

6666
{
@@ -493,17 +493,8 @@ common.crashOnUnhandledRejection();
493493
}
494494
});
495495

496-
read.on('close', common.mustCall());
497-
transform.on('close', common.mustCall());
498-
write.on('close', common.mustCall());
499-
500-
process.on('uncaughtException', common.mustCall((err) => {
501-
assert.deepStrictEqual(err, new Error('kaboom'));
502-
}));
503-
504-
const dst = pipeline(read, transform, write);
505-
506-
assert.strictEqual(dst, write);
507-
508-
read.push('hello');
496+
assert.throws(
497+
() => pipeline(read, transform, write),
498+
{ code: 'ERR_INVALID_CALLBACK' }
499+
);
509500
}

0 commit comments

Comments
 (0)