Skip to content

Commit

Permalink
stream: fix .end() error propagation
Browse files Browse the repository at this point in the history
PR-URL: #36817
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
ronag authored and danielleadams committed Jan 12, 2021
1 parent 1c9ec25 commit 4c819d6
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 13 deletions.
40 changes: 27 additions & 13 deletions lib/internal/streams/writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

const {
FunctionPrototype,
Error,
ObjectDefineProperty,
ObjectDefineProperties,
ObjectSetPrototypeOf,
Expand Down Expand Up @@ -290,8 +291,8 @@ Writable.prototype.pipe = function() {
errorOrDestroy(this, new ERR_STREAM_CANNOT_PIPE());
};

Writable.prototype.write = function(chunk, encoding, cb) {
const state = this._writableState;
function _write(stream, chunk, encoding, cb) {
const state = stream._writableState;

if (typeof encoding === 'function') {
cb = encoding;
Expand Down Expand Up @@ -333,11 +334,15 @@ Writable.prototype.write = function(chunk, encoding, cb) {

if (err) {
process.nextTick(cb, err);
errorOrDestroy(this, err, true);
return false;
errorOrDestroy(stream, err, true);
return err;
}
state.pendingcb++;
return writeOrBuffer(this, state, chunk, encoding, cb);
return writeOrBuffer(stream, state, chunk, encoding, cb);
}

Writable.prototype.write = function(chunk, encoding, cb) {
return _write(this, chunk, encoding, cb) === true;
};

Writable.prototype.cork = function() {
Expand Down Expand Up @@ -607,21 +612,30 @@ Writable.prototype.end = function(chunk, encoding, cb) {
encoding = null;
}

if (chunk !== null && chunk !== undefined)
this.write(chunk, encoding);
let err;

if (chunk !== null && chunk !== undefined) {
const ret = _write(this, chunk, encoding);
if (ret instanceof Error) {
err = ret;
}
}

// .end() fully uncorks.
if (state.corked) {
state.corked = 1;
this.uncork();
}

// This is forgiving in terms of unnecessary calls to end() and can hide
// logic errors. However, usually such errors are harmless and causing a
// hard error can be disproportionately destructive. It is not always
// trivial for the user to determine whether end() needs to be called or not.
let err;
if (!state.errored && !state.ending) {
if (err) {
// Do nothing...
} else if (!state.errored && !state.ending) {
// This is forgiving in terms of unnecessary calls to end() and can hide
// logic errors. However, usually such errors are harmless and causing a
// hard error can be disproportionately destructive. It is not always
// trivial for the user to determine whether end() needs to be called
// or not.

state.ending = true;
finishMaybe(this, state, true);
state.ended = true;
Expand Down
35 changes: 35 additions & 0 deletions test/parallel/test-stream-writable-end-cb-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,38 @@ const stream = require('stream');
writable.emit('error', new Error('kaboom'));
}));
}

{
const w = new stream.Writable({
write(chunk, encoding, callback) {
setImmediate(callback);
},
finish(callback) {
setImmediate(callback);
}
});
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(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(w.destroyed, false);
assert.strictEqual(w.writableEnded, true);
w.end('end', common.mustCall((err) => {
assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END');
}));
assert.strictEqual(w.destroyed, true);
w.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END');
}));
w.on('finish', common.mustNotCall());
}

0 comments on commit 4c819d6

Please sign in to comment.