Writable does not check if stream has been destroyed during _final and _write #39030

@nodejs/streams

I don't know to be honest as I don't want to make things too stringent. However adding a check is going to improve the developer experience.

What should the check do? Throw? emit 'error'?

I think:

1. cancel any further substeps
2. if stream was destroyed without error, override with error

Should we tag this as good first issue?

@ronag Can i take this up?

Go for it!

Hey! I was wondering about the status of this issue, and if I could try looking into it?

Go for it!

I went through Writable.js, but I'm not sure where should the checks be added. Specifically, I found these to be already present:

• _write at line 321 checks for state.destroyed before going to the user-provided _write function
• _final seems to be called through prefinish which also checks for state.destroyed at line 718

I'll really appreciate some guidance on this

When I went through Writable.js I was unable to understand the state.sync flag, could anyone explain what it is?

Seems to be an interesting research topic.
Let me know and we arrange a meeting for discussing it.

Hey i was wondering if this issue is still open and if i can take a crack at it 😸

Hi, is this issue still open. If so, I can try and resolve it.

This would also be my first issue if I am able to work on it.

Can you assign this issue to me?

Hi. Is this still open for people to work on?

If so is the following the expected behaviour?

I think:

1. cancel any further substeps
2. if stream was destroyed without error, override with error

@mcollina @ronag I think that emitting ERR_STREAM_DESTROYED is the behavior this PR is trying to avoid #45062

I've dug into this issue, and I've got some insights to share. Take a look:

It seems that throwing ERR_STREAM_DESTROYED is the behavior the PR #45062 is attempting to avoid.. Also, in PR #25973, they tweaked the documentation on destroy. Now it implies that there might be cases where ERR_STREAM_DESTROYED won't be triggered.

Previously, it said:

After this call, the writable stream has ended and subsequent calls
to `write()` or `end()` will result in an `ERR_STREAM_DESTROYED` error.

Now:

This is a destructive and immediate way to destroy a stream. Previous calls to
`write()` may not have drained, and may trigger an `ERR_STREAM_DESTROYED` error.

Upon examining the code, it's apparent that the ERR_STREAM_DESTROYED error occurs when destroy is called while there is still data in the buffer awaiting write.

For example:

 const callbacks = [];
  const wb = new Writable({
    write(data, enc, cb) {
      callbacks.push(cb);
    },
    // Effectively disable the HWM to observe 'drain' events more easily.
    highWaterMark: 1
  });

  wb.write('abc', onWrite); 
  // Second write goes to buffer since highWaterMark===1
  wb.write('bbb', onWrite); // Throws ERR_STREAM_DESTROYED
  wb.destroy();
  callbacks.shift()();

So, based on what I've seen, I'm thinking we might not need to dive deeper into this issue. I would appreciate your thoughts on this.

Cheers!

Hi! I’d like to take a look at this issue and see whether the behavior is safe by design or needs a guard/comment. Let me know if that sounds good.

This issue has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

I'm wondering about something: the operation actually completes successfully, the flow isn't broken, no exception is thrown, and the data fulfills its purpose during the operation — so is there really "a bug that needs fixing" here, or is what's actually needed "a way to make this situation observable"? If it's the latter, I think we could move forward without ever having to settle the throw-vs-silent question. The write/final callback could keep doing exactly what it does today — no behavior change, no breaking change. The publicly readable destroyed getter (lib/internal/streams/writable.js:996-1008) is already updated the moment .destroy() is called, independent of when the callback eventually returns. So the information isn't actually missing — it's just never surfaced anywhere at the point onwrite/onFinish runs. Instead of changing what the callback means, would it make sense for onwrite and onFinish to attach a purely additive, opt-in signal at that point — an event or a field — saying "this operation completed successfully, but the stream had already been destroyed while it was in flight"? That way no existing behavior changes, but anyone who wants to observe it can, and if there's an underlying error condition behind it, it becomes much easier to spot during debugging.

This issue slipped through the cracks because our previous stale bot only tracked issues and couldn't catch all the issues.
Our new stale bot flagged this, and would have closed it shortly after RenderATL, but I'm just doing it a bit early so
maintainer's can focus on new code-and-learn PRs during the event.

If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.