Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 4c819d6

Browse filesBrowse files
ronagdanielleadams
authored andcommitted
stream: fix .end() error propagation
PR-URL: #36817 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 1c9ec25 commit 4c819d6
Copy full SHA for 4c819d6

File tree

Expand file treeCollapse file tree

2 files changed

+62
-13
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+62
-13
lines changed
Open diff view settings
Collapse file

‎lib/internal/streams/writable.js‎

Copy file name to clipboardExpand all lines: lib/internal/streams/writable.js
+27-13Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
const {
2929
FunctionPrototype,
30+
Error,
3031
ObjectDefineProperty,
3132
ObjectDefineProperties,
3233
ObjectSetPrototypeOf,
@@ -290,8 +291,8 @@ Writable.prototype.pipe = function() {
290291
errorOrDestroy(this, new ERR_STREAM_CANNOT_PIPE());
291292
};
292293

293-
Writable.prototype.write = function(chunk, encoding, cb) {
294-
const state = this._writableState;
294+
function _write(stream, chunk, encoding, cb) {
295+
const state = stream._writableState;
295296

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

334335
if (err) {
335336
process.nextTick(cb, err);
336-
errorOrDestroy(this, err, true);
337-
return false;
337+
errorOrDestroy(stream, err, true);
338+
return err;
338339
}
339340
state.pendingcb++;
340-
return writeOrBuffer(this, state, chunk, encoding, cb);
341+
return writeOrBuffer(stream, state, chunk, encoding, cb);
342+
}
343+
344+
Writable.prototype.write = function(chunk, encoding, cb) {
345+
return _write(this, chunk, encoding, cb) === true;
341346
};
342347

343348
Writable.prototype.cork = function() {
@@ -607,21 +612,30 @@ Writable.prototype.end = function(chunk, encoding, cb) {
607612
encoding = null;
608613
}
609614

610-
if (chunk !== null && chunk !== undefined)
611-
this.write(chunk, encoding);
615+
let err;
616+
617+
if (chunk !== null && chunk !== undefined) {
618+
const ret = _write(this, chunk, encoding);
619+
if (ret instanceof Error) {
620+
err = ret;
621+
}
622+
}
612623

613624
// .end() fully uncorks.
614625
if (state.corked) {
615626
state.corked = 1;
616627
this.uncork();
617628
}
618629

619-
// This is forgiving in terms of unnecessary calls to end() and can hide
620-
// logic errors. However, usually such errors are harmless and causing a
621-
// hard error can be disproportionately destructive. It is not always
622-
// trivial for the user to determine whether end() needs to be called or not.
623-
let err;
624-
if (!state.errored && !state.ending) {
630+
if (err) {
631+
// Do nothing...
632+
} else if (!state.errored && !state.ending) {
633+
// This is forgiving in terms of unnecessary calls to end() and can hide
634+
// logic errors. However, usually such errors are harmless and causing a
635+
// hard error can be disproportionately destructive. It is not always
636+
// trivial for the user to determine whether end() needs to be called
637+
// or not.
638+
625639
state.ending = true;
626640
finishMaybe(this, state, true);
627641
state.ended = true;
Collapse file

‎test/parallel/test-stream-writable-end-cb-error.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-stream-writable-end-cb-error.js
+35Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,38 @@ const stream = require('stream');
4646
writable.emit('error', new Error('kaboom'));
4747
}));
4848
}
49+
50+
{
51+
const w = new stream.Writable({
52+
write(chunk, encoding, callback) {
53+
setImmediate(callback);
54+
},
55+
finish(callback) {
56+
setImmediate(callback);
57+
}
58+
});
59+
w.end('testing ended state', common.mustCall((err) => {
60+
// This errors since .destroy(err), which is invoked by errors
61+
// in same tick below, will error all pending callbacks.
62+
// Does this make sense? Not sure.
63+
assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED');
64+
}));
65+
assert.strictEqual(w.destroyed, false);
66+
assert.strictEqual(w.writableEnded, true);
67+
w.end(common.mustCall((err) => {
68+
// This errors since .destroy(err), which is invoked by errors
69+
// in same tick below, will error all pending callbacks.
70+
// Does this make sense? Not sure.
71+
assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED');
72+
}));
73+
assert.strictEqual(w.destroyed, false);
74+
assert.strictEqual(w.writableEnded, true);
75+
w.end('end', common.mustCall((err) => {
76+
assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END');
77+
}));
78+
assert.strictEqual(w.destroyed, true);
79+
w.on('error', common.mustCall((err) => {
80+
assert.strictEqual(err.code, 'ERR_STREAM_WRITE_AFTER_END');
81+
}));
82+
w.on('finish', common.mustNotCall());
83+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.