An error is passed to `writable.end(callback)` if `_final` finishes later than `.destroy()`

[szmarczak]

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 no

How 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...

[targos]

@nodejs/streams

[mcollina]

This is wanted behavior, not a bug.

The source of current behavior is that the stream resumes on the next tick and .destroy() sets .destroyed synchronously. 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 Error object 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]

if we should retain the Error object that is passed to .destroy() inside the state

We already do this. I think the best would be if end got the destroy error if it was called before destroy.

[vweevers]

Can we use a different error message in this case? The message Cannot call end after a stream was destroyed suggests that the user did something wrong. A more general message like Stream was destroyed would avoid that.

I prefer that over reusing the oh no error 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, that destroy() is the cause of not ending and oh no is the cause of destroying. Splitting these allows the end() callback to recognize and handle this error in a generic way, which would not be possible if the error is user-defined.

[ronag]

I think the follow two should have the same error, otherwise it's confusing:

s.end(callback);
s.destroy(new Error('oh no'));

s.end(callback);
setTimeout(() => s.destroy(new Error('oh no')), 1e3);