Description
The current official API for destroying is destroy(err)
. Where the user can supply a custom destroyer on the prototype through _destroy(err, cb)
and where err
comes from the call to destroy(err)
. The user can choose to ignore, replace, forward or modify err
through the callback.
However, what is the actual vs expected behavior of this in regards to multiple calls to destroy(err)
? The _destroy
destroyer can only be invoked during the first call and any further calls to destroy(err)
will currently bypass this and directly emit 'error'
.
Examples
Example 1: We want to swallow errors. However, a second call to destroy(err)
will actually emit an error.
const s = new Readable({
read(),
_destroy (err, cb) {
// Swallow error.
cb();
}
});
s.on('error', common.mustNotCall()); // Fails. Invoked with 'error 2'.
s.destroy(new Error('error 1'));
s.destroy(new Error('error 2'));
Example 2: We want to (in some manner) modify the error.
{
// Mutate
const s = new Readable({
read(),
_destroy (err, cb) {
if (err) {
err.parent = this;
}
process.nextTick(cb, err);
}
});
s.on('error', common.mustCall(err => {
// Invoked with 'error 2'.
assert.strictEqual(err.parent, s); // Fails. 'error 2'
assert.strictEqual(err.message, 'error 1'); // Fails. 'error 2'
}));
s.destroy(new Error('error 1'));
s.destroy(new Error('error 2'));
}
{
// Replace
const s = new Readable({
read(),
_destroy (err, cb) {
process.nextTick(cb, new Error('error 3'));
}
});
s.on('error', common.mustCall(err => {
// Invoked with 'error 2'.
assert.strictEqual(err.message, 'error 3'); // Fails. 'error 2'
}));
s.destroy(new Error('error 1'));
s.destroy(new Error('error 2'));
}
Example 3: When the destroyer has fully completed and the stream is effectivly destructed an error can unexpectedly be injected.
let closed = false;
// Async
const s = new Readable({
read(),
_destroy (err, cb) {
cb();
closed = true;
}
});
s.destroy();
assert.strictEqual(closed, true); // OK
// 'close' emits in nextTick
s.destroy(new Error('error 2')); // Fails. uncaught exception
This would fail even with #29197 where errors are swallowed after 'close'
.
Solutions
-
Originally in stream: fix multiple
destroy()
calls #29197 we discussed that only the first call todestroy(err)
is applied and any further calls are noop. This might be logical since we can only call_destroy
once to apply any related logic. This would resolve the issues in all the examples above. The downside of this is in casedestroy(err)
is called afterdestroy()
while destruction is still on-going, in which case the error would be swallowed. Due to this objection we came to the compromise of "don't emit'error'
after'close'
" to resolve the specific issue that PR was trying to resolve (i.e. when can no further'error'
events be emitted). -
The first error always wins stream: first error wins and cannot be overriden #30982. Which would partly resolve Example 3 but not allow other error handling through
_destroy
. -
Just leave things as they are and possibly document the edge cases.
Refs
- stream: don't emit 'error' after 'close' (stream: fix multiple
destroy()
calls #29197) - custom
_destroy
can swallow error? (custom_destroy
can swallow error? #30979) - stream: first error wins and cannot be overriden (stream: first error wins and cannot be overriden #30982)