Skip to content

Commit

Permalink
Revert "stream: fix .end() error propagation"
Browse files Browse the repository at this point in the history
This reverts commit 4c819d6.

PR-URL: #37060
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
  • Loading branch information
mcollina authored and danielleadams committed Feb 15, 2021
1 parent 2680979 commit d2a487e
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 62 deletions.
40 changes: 13 additions & 27 deletions lib/internal/streams/writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@

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

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

if (typeof encoding === 'function') {
cb = encoding;
Expand Down Expand Up @@ -334,15 +333,11 @@ function _write(stream, chunk, encoding, cb) {

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

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

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

let err;

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

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

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.

// 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) {
state.ending = true;
finishMaybe(this, state, true);
state.ended = true;
Expand Down
35 changes: 0 additions & 35 deletions test/parallel/test-stream-writable-end-cb-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,38 +46,3 @@ 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 d2a487e

Please sign in to comment.