From e8a4ee214dbbf7a2208d64d43178ccd3580ef917 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 12 Jul 2021 09:42:50 +0200 Subject: [PATCH 1/3] stream: forward errored to callback Refs: https://github.com/nodejs/node/issues/39356 --- lib/internal/streams/writable.js | 4 ++-- test/parallel/test-stream-writable-destroy.js | 13 +++++++++++++ .../test-stream-writable-end-cb-uncaught.js | 5 +++-- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index d8a29620536bbc..02fccbf5fdce69 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -515,12 +515,12 @@ function errorBuffer(state) { const { chunk, callback } = state.buffered[n]; const len = state.objectMode ? 1 : chunk.length; state.length -= len; - callback(new ERR_STREAM_DESTROYED('write')); + callback(state.errored ?? new ERR_STREAM_DESTROYED('write')); } const onfinishCallbacks = state[kOnFinished].splice(0); for (let i = 0; i < onfinishCallbacks.length; i++) { - onfinishCallbacks[i](new ERR_STREAM_DESTROYED('end')); + onfinishCallbacks[i](state.errored ?? new ERR_STREAM_DESTROYED('end')); } resetBuffer(state); diff --git a/test/parallel/test-stream-writable-destroy.js b/test/parallel/test-stream-writable-destroy.js index 2e6e1f975a2be1..b7101f65427cf1 100644 --- a/test/parallel/test-stream-writable-destroy.js +++ b/test/parallel/test-stream-writable-destroy.js @@ -471,3 +471,16 @@ const assert = require('assert'); write.destroy(); write.destroy(); } + +{ + // https://github.com/nodejs/node/issues/39356 + const s = new Writable({ + final() {} + }); + const _err = new Error('oh no'); + // Remove `callback` and it works + s.end(common.mustCall((err) => { + assert.strictEqual(err, _err); + })); + s.destroy(_err); +} diff --git a/test/parallel/test-stream-writable-end-cb-uncaught.js b/test/parallel/test-stream-writable-end-cb-uncaught.js index 5c1753aa067342..02586b45d99a2f 100644 --- a/test/parallel/test-stream-writable-end-cb-uncaught.js +++ b/test/parallel/test-stream-writable-end-cb-uncaught.js @@ -9,15 +9,16 @@ process.on('uncaughtException', common.mustCall((err) => { })); const writable = new stream.Writable(); +const _err = new Error('kaboom'); writable._write = (chunk, encoding, cb) => { cb(); }; writable._final = (cb) => { - cb(new Error('kaboom')); + cb(_err); }; writable.write('asd'); writable.end(common.mustCall((err) => { - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(err, _err); })); From 346cda70997ee3c7042c0ea30bb3fa2b3e4ee18e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 12 Jul 2021 13:25:44 +0200 Subject: [PATCH 2/3] fixup --- test/parallel/test-stream-writable-destroy.js | 17 +++++++++++------ .../test-stream-writable-end-cb-error.js | 19 +++++++------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/test/parallel/test-stream-writable-destroy.js b/test/parallel/test-stream-writable-destroy.js index b7101f65427cf1..814d82b01a74fb 100644 --- a/test/parallel/test-stream-writable-destroy.js +++ b/test/parallel/test-stream-writable-destroy.js @@ -351,33 +351,35 @@ const assert = require('assert'); const write = new Writable({ write(chunk, enc, cb) { process.nextTick(cb); } }); + const _err = new Error('asd'); write.once('error', common.mustCall((err) => { assert.strictEqual(err.message, 'asd'); })); write.end('asd', common.mustCall((err) => { - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(err, _err); })); - write.destroy(new Error('asd')); + write.destroy(_err); } { // Call buffered write callback with error + const _err = new Error('asd'); const write = new Writable({ write(chunk, enc, cb) { - process.nextTick(cb, new Error('asd')); + process.nextTick(cb, _err); }, autoDestroy: false }); write.cork(); write.write('asd', common.mustCall((err) => { - assert.strictEqual(err.message, 'asd'); + assert.strictEqual(err, _err); })); write.write('asd', common.mustCall((err) => { - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(err, _err); })); write.on('error', common.mustCall((err) => { - assert.strictEqual(err.message, 'asd'); + assert.strictEqual(err, _err); })); write.uncork(); } @@ -482,5 +484,8 @@ const assert = require('assert'); s.end(common.mustCall((err) => { assert.strictEqual(err, _err); })); + s.on('error', (err) => { + assert.strictEqual(err, _err); + }); s.destroy(_err); } diff --git a/test/parallel/test-stream-writable-end-cb-error.js b/test/parallel/test-stream-writable-end-cb-error.js index 20428f1777fd17..f871c50dbe972c 100644 --- a/test/parallel/test-stream-writable-end-cb-error.js +++ b/test/parallel/test-stream-writable-end-cb-error.js @@ -8,19 +8,20 @@ const stream = require('stream'); // Invoke end callback on failure. const writable = new stream.Writable(); + const _err = new Error('kaboom'); writable._write = (chunk, encoding, cb) => { - process.nextTick(cb, new Error('kaboom')); + process.nextTick(cb, _err); }; writable.on('error', common.mustCall((err) => { - assert.strictEqual(err.message, 'kaboom'); + assert.strictEqual(err, _err); })); writable.write('asd'); writable.end(common.mustCall((err) => { - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(err, _err); })); writable.end(common.mustCall((err) => { - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(err, _err); })); } @@ -57,18 +58,12 @@ const stream = require('stream'); } }); w.end('testing ended state', common.mustCall((err) => { - // This errors since .destroy(err), which is invoked by errors - // in same tick below, will error all pending callbacks. - // Does this make sense? Not sure. - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END'); })); assert.strictEqual(w.destroyed, false); assert.strictEqual(w.writableEnded, true); w.end(common.mustCall((err) => { - // This errors since .destroy(err), which is invoked by errors - // in same tick below, will error all pending callbacks. - // Does this make sense? Not sure. - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); + assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END'); })); assert.strictEqual(w.destroyed, false); assert.strictEqual(w.writableEnded, true); From e00459feb0421c81d0170639b571612b2c80cc68 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 13 Jul 2021 14:51:49 +0200 Subject: [PATCH 3/3] Update test/parallel/test-stream-writable-destroy.js Co-authored-by: Luigi Pinca --- test/parallel/test-stream-writable-destroy.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-stream-writable-destroy.js b/test/parallel/test-stream-writable-destroy.js index 814d82b01a74fb..910cca4adc21aa 100644 --- a/test/parallel/test-stream-writable-destroy.js +++ b/test/parallel/test-stream-writable-destroy.js @@ -484,8 +484,8 @@ const assert = require('assert'); s.end(common.mustCall((err) => { assert.strictEqual(err, _err); })); - s.on('error', (err) => { + s.on('error', common.mustCall((err) => { assert.strictEqual(err, _err); - }); + })); s.destroy(_err); }