Repository navigation
An error is passed to writable.end(callback) if _final finishes later than .destroy() #39356
Description
Version
v16.4.2
Platform
Linux solus 5.13.1-187.current #1 SMP PREEMPT Wed Jul 7 19:52:26 UTC 2021 x86_64 GNU/Linux
Subsystem
stream
What steps will reproduce the bug?
const {Writable} = require('stream');
const s = new Writable({
final() {}
});
const callback = error => {
console.error(error);
};
// Remove `callback` and it works
s.end(callback); // Error: Cannot call end after a stream was destroyed
s.destroy(new Error('oh no')); // Error: oh noHow often does it reproduce? Is there a required condition?
Always.
What is the expected behavior?
node:events:371
throw er; // Unhandled 'error' event
^
Error: oh no
at Object.<anonymous> (/home/szm/Desktop/http2-wrapper/bug.js:13:11)
at Module._compile (node:internal/modules/cjs/loader:1095:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
at Module.load (node:internal/modules/cjs/loader:975:32)
at Function.Module._load (node:internal/modules/cjs/loader:816:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
at node:internal/main/run_main_module:17:47
Emitted 'error' event on Writable instance at:
at emitErrorNT (node:internal/streams/destroy:193:8)
at emitErrorCloseNT (node:internal/streams/destroy:158:3)
at processTicksAndRejections (node:internal/process/task_queues:83:21)
What do you see instead?
+Error [ERR_STREAM_DESTROYED]: Cannot call end after a stream was destroyed
+ at new NodeError (node:internal/errors:363:5)
+ at errorBuffer (node:internal/streams/writable:522:26)
+ at processTicksAndRejections (node:internal/process/task_queues:82:21) {
+ code: 'ERR_STREAM_DESTROYED'
+}
node:events:371
throw er; // Unhandled 'error' event
^
Error: oh no
at Object.<anonymous> (/home/szm/Desktop/http2-wrapper/bug.js:13:11)
at Module._compile (node:internal/modules/cjs/loader:1095:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
at Module.load (node:internal/modules/cjs/loader:975:32)
at Function.Module._load (node:internal/modules/cjs/loader:816:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
at node:internal/main/run_main_module:17:47
Emitted 'error' event on Writable instance at:
at emitErrorNT (node:internal/streams/destroy:193:8)
at emitErrorCloseNT (node:internal/streams/destroy:158:3)
at processTicksAndRejections (node:internal/process/task_queues:83:21)Additional information
This could be the wanted behavior. But I don't think there's a way to recognize if _final actually timed out or not.
The error in .destroy(error) is more important in my case, so the error saying that _final timed out can be safely discarded - I'm not sure if it's possible though...
Activity
targos commentedon Jul 12, 2021
@nodejs/streams
mcollina commentedon Jul 12, 2021
This is wanted behavior, not a bug.
The source of current behavior is that the stream resumes on the next tick and
.destroy()sets.destroyedsynchronously. So whenever.end()is actually executed, the stream has already been destroyed.The key question to improve the user experience is if we should retain the
Errorobject that is passed to.destroy()inside the state and give this to users instead. I'm not sure that would be correct either as the same error could be presented multiple times.ronag commentedon Jul 12, 2021
We already do this. I think the best would be if end got the destroy error if it was called before destroy.
vweevers commentedon Jul 12, 2021
Can we use a different error message in this case? The message
Cannot call end after a stream was destroyedsuggests that the user did something wrong. A more general message likeStream was destroyedwould avoid that.I prefer that over reusing the
oh noerror passed to.destroy()because that error explains why the stream was destroyed, while the error passed to.end(callback)should explain why.end()didn't succeed. In other words, thatdestroy()is the cause of not ending andoh nois the cause of destroying. Splitting these allows theend()callback to recognize and handle this error in a generic way, which would not be possible if the error is user-defined.ronag commentedon Jul 12, 2021
I think the follow two should have the same error, otherwise it's confusing:
stream: forward errored to callback