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

[ronag]

Not sure if this is a problem but I think we should at least add a comment in the code that this case has been considered.

[targos]

@nodejs/streams

[mcollina]

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'?

[ronag]

I think:

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

[mcollina]

Should we tag this as good first issue?

[mdaj06]

@ronag Can i take this up?

[mcollina]

Go for it!

[Svanazar]

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

[mcollina]

Go for it!

[Svanazar]

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

[ronag]

You need to look at the time between those being called and their callback being invoked.

[Svanazar]

Thanks! I followed the code between these two times, and wanted to confirm the location. For _write, will it be appropriate to place the check inside onwrite() ?

This will be for the case when the stream gets destroyed during _write but no error is passed to the callback.

[rscherich]

After looking into this, this seems to run counter to a previous pull request (maybe I didn't understand something?): #24062
Of course, the project requirements might have changed since then too.

I can't immediately see a way to throw an error at the start of the callbacks without changing/removing several of the tests. I also foresee some issues with asynchronous implementations of _write and _final (also in the tests). Do we want to go ahead with the change anyway?

[djs113]

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

[SebasQuirogaUCP]

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

[Viper-space]

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

[zeazad-hub]

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

[zeazad-hub]

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

[zeazad-hub]

Can you assign this issue to me?

[Ceres6]

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

[quixote15]

@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!