From 7e81a1665bf21ecb57256e4cec6d8969f6fdc643 Mon Sep 17 00:00:00 2001 From: Chetan Karande Date: Tue, 3 Sep 2019 13:34:36 -0400 Subject: [PATCH 1/2] doc: fix unsafe writable stream code example Update writable stream code example using async iterator to use safer `finished()` method instead of a `finish` event to avoid uncaught exceptions Fixes: https://github.com/nodejs/node/issues/29397 --- doc/api/stream.md | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index ccdb7e4180bc32..1ac91d939f86c8 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -2559,6 +2559,7 @@ it is important to ensure the correct handling of backpressure and errors. ```js const { once } = require('events'); +const finished = util.promisify(stream.finished); const writable = fs.createWriteStream('./file'); @@ -2570,18 +2571,24 @@ const writable = fs.createWriteStream('./file'); } writable.end(); // Ensure completion without errors. - await once(writable, 'finish'); + await finished(writable); })(); ``` -In the above, errors on the write stream would be caught and thrown by the two -`once()` listeners, since `once()` will also handle `'error'` events. +In the above, errors on `write()` would be caught and thrown by the +`once()` listener for the `'drain'` event, since `once()` will also handle the +`'error'` event. To ensure completion of the write stream without errors, +it is safer to use the `finished()` method as above, instead of using the +`once()` listener for the `'finish'` event. Under certain cases, an `'error'` +event could be emitted by the writable stream after `'finish'` and as `once()` +will release the `'error'` handler on handling the `'finish'` event, it could +result in an unhandled error. -Alternatively the readable stream could be wrapped with `Readable.from()` and +Alternatively, the readable stream could be wrapped with `Readable.from()` and then piped via `.pipe()`: ```js -const { once } = require('events'); +const finished = util.promisify(stream.finished); const writable = fs.createWriteStream('./file'); @@ -2589,7 +2596,20 @@ const writable = fs.createWriteStream('./file'); const readable = Readable.from(iterator); readable.pipe(writable); // Ensure completion without errors. - await once(writable, 'finish'); + await finished(writable); +})(); +``` + +Or, using `stream.pipeline` to pipe streams: + +```js +const pipeline = util.promisify(stream.pipeline); + +const writable = fs.createWriteStream('./file'); + +(async function() { + const readable = Readable.from(iterator); + await pipeline(readable, writable); })(); ``` From b3a2df2778f94d00b9bef01a9b9273cb5f8a0b75 Mon Sep 17 00:00:00 2001 From: Chetan Karande Date: Wed, 4 Sep 2019 13:30:16 -0400 Subject: [PATCH 2/2] doc: fix to refer pipeline() as a method --- doc/api/stream.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 1ac91d939f86c8..a3b77e701b996c 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -2600,7 +2600,7 @@ const writable = fs.createWriteStream('./file'); })(); ``` -Or, using `stream.pipeline` to pipe streams: +Or, using `stream.pipeline()` to pipe streams: ```js const pipeline = util.promisify(stream.pipeline);