Skip to content

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

Closed
Closed
An error is passed to writable.end(callback) if _final finishes later than .destroy()#39356
Labels
streamIssues and PRs related to Node.js streams.

Description

@szmarczak
szmarczak
opened on Jul 11, 2021

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

Activity

  1. targos commented on Jul 12, 2021

    @targos
    targos
    on Jul 12, 2021
    Member

    @nodejs/streams

  2. mcollina commented on Jul 12, 2021

    @mcollina
    mcollina
    on Jul 12, 2021
    SponsorMember

    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.

  3. ronag commented on Jul 12, 2021

    @ronag
    ronag
    on Jul 12, 2021
    Member

    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.

  4. vweevers commented on Jul 12, 2021

    @vweevers
    vweevers
    on Jul 12, 2021
    Contributor

    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.

  5. ronag commented on Jul 12, 2021

    @ronag
    ronag
    on Jul 12, 2021
    Member

    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);
  6. Ayase-252
    added
    streamIssues and PRs related to Node.js streams.
    on Jul 13, 2021
  7. ronag
    added a commit that references this issue on Jul 14, 2021

    stream: forward errored to callback

    efd40ea
  8. Mesteery
    closed this as completedon Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    streamIssues and PRs related to Node.js streams.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions