Repository navigation
Writable is destroyed before calling callback in rare case #40377
Description
Version
v16.10.0
Platform
Linux solus 5.14.7-198.current #1 SMP PREEMPT Wed Sep 22 16:02:46 UTC 2021 x86_64 GNU/Linux
Subsystem
stream
What steps will reproduce the bug?
const {Writable} = require('stream');
class X extends Writable {
async _destroy(error, callback) {
(async () => {
await new Promise(resolve => setTimeout(resolve, 10));
console.log(w._writableState.closed);
callback(error);
})();
}
}
const w = new X();
w.once('error', error => {
console.log(error);
});
w.destroy(new Error('oh no!'));How often does it reproduce? Is there a required condition?
Always.
What is the expected behavior?
false
Error: oh no!
What do you see instead?
true
Additional information
Some undocumented behavior here:
node/lib/internal/streams/destroy.js
Lines 101 to 118 in aff2a0a
Activity
[-]Writable is destroyed before calling callback[/-][+]Writable is destroyed before calling callback in rare case[/+]targos commentedon Oct 9, 2021
@nodejs/streams
ronag commentedon Oct 9, 2021
Don’t mix thenable with callback.
mcollina commentedon Oct 9, 2021
The behavior is correct for me. After .destroy() is called, the stream is closed synchronously and no more read or write operation can happen. However the destroy cycle is asynchronous because it might take a while to completely clean up a native resource.
szmarczak commentedon Oct 9, 2021
@mcollina I forgot to include
Error: oh no!in expected behavior. The error is not emitted. The issue is that if_destroyreturns a thenable, there's a race condition - either the callback gets called first or the promise resolves. If it waits for the thenable, it should omit thecallbackargument (or throw when it gets called). Also the documentation doesn't mention anything about_destroybeing thenable. Therefore I consider this a bug.mcollina commentedon Oct 9, 2021
Uh? Using a thenable there should not be supported.
ronag commentedon Oct 9, 2021
I think this is a case of insufficient documentation.
mcollina commentedon Oct 10, 2021
I have a feeling I missed something when we added thenable support to destroy.
I'll need to dig into this and check out what's the problem.
mcollina commentedon Oct 10, 2021
This was added in 744a284 without documentation.
The error is not printed because it is not rethrown by the _destroy function. Moreover, the callback should not be mixed with async/await (nor is needed).
ronag commentedon Oct 10, 2021
I think we could maybe emit a warning if function returns a thenable when the function.length has the value for the callback signature.
szmarczak commentedon Oct 10, 2021
Wouldn't it be better to throw after the promise resolves and the callback gets called?
mcollina commentedon Oct 10, 2021
if you implement _destroy(), you need to rethrow or call the callback with the error passed as an argument.
szmarczak commentedon Oct 10, 2021
Again there's this situation where callback may be called after the promise resolves. Currently calling the callback doesn't do anything because at that point the stream is destroyed already because the promise resolved.
One can expect
callbacknot to throw, so indeed a warning would be sufficient I think.mcollina commentedon Oct 11, 2021
The documentation must be updated to better specify all of this. I would recommend to add a warning as well when using a promise with a callback.
Mesteery commentedon Oct 11, 2021
I think this is a good first issue.
RafaelGSS commentedon Dec 22, 2021
@mcollina it can be closed #41040 was merged.