Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream.pipeline doesn't wait for 'close' on error #51540

Open
wh0 opened this issue Jan 22, 2024 · 8 comments · May be fixed by #53462
Open

stream.pipeline doesn't wait for 'close' on error #51540

wh0 opened this issue Jan 22, 2024 · 8 comments · May be fixed by #53462
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@wh0
Copy link
Contributor

wh0 commented Jan 22, 2024 •
edited

Version

v20.11.0

Platform

Linux cs-63018580108-ephemeral-4kum 6.1.58+ #1 SMP PREEMPT_DYNAMIC Sat Dec 30 15:31:26 UTC 2023 x86_64 GNU/Linux

Subsystem

internal/streams/pipeline

What steps will reproduce the bug?

const stream = require('stream');

const src = new stream.Readable();
const dst = new stream.Writable({
  write(chunk, encoding, callback) {
    callback();
  },
  destroy(error, callback) {
    // takes a while to destroy
    setImmediate(() => {
      callback(error);
    });
  },
});

stream.pipeline(src, dst, (err) => {
  console.log(`pipeline done, err=${err.message}, dst.closed=${dst.closed}`);
});
src.destroy(new Error('problem'));

How often does it reproduce? Is there a required condition?

always

What is the expected behavior? Why is that the expected behavior?

dst.closed=true

maybe? #32158 sounds like it should

What do you see instead?

pipeline done, err=problem, dst.closed=false

Additional information

I notice that this finishCount countdown goes from 2 down to -3.

  1. (2 -> 1) src error event ("Catch stream errors that occur after pipe/pump has completed.")
  2. (1 -> 0, final) src err event (same one) (pipe src eos onerror)
  3. (0 -> -1) dst error event ("Catch stream errors that occur after pipe/pump has completed.")
  4. (-1 -> -2) dst error event (same one) (pipe dst eos onerror)
  5. (-2 -> -3) dst close event ("Finish if the destination closes before the source has completed.")
@RedYetiDev RedYetiDev added the stream Issues and PRs related to the stream subsystem. label Apr 28, 2024
@jakecastelli
Copy link
Contributor

Hi, I am trying to get a bit more context here, do you expect the close event on Writable stream happens before the callback is being called?

@wh0
Copy link
Contributor Author

wh0 commented Jun 8, 2024

yes. from all that I've seen, I do think the intention of stream.pipeline was to wait for the Writable to finish closing before calling the callback. there's this finishCount code that's meant to keep track of everything closing, but it's getting decremented way more times than the code that sets up the initial count expects

@jakecastelli jakecastelli mentioned this issue Jun 8, 2024
Closed
@jakecastelli
Copy link
Contributor

jakecastelli commented Jun 8, 2024 •
edited

I do think the intention of stream.pipeline was to wait for the Writable to finish closing before calling the callback

I think you are right on this one. I have taken some time to dig, what I found it out is that because the ERR_METHOD_NOT_IMPLEMENTED error that messed up the finishCount (I used the debugger but you can just log the err here and I think you will understand what I mean.

If you implement the read{} function in the Readable stream then it is the behaviour you expected, or if you can take a look the patch I made here and let me know if it makes sense or not 👀

Thanks a lot for narrowing down the directions, if the fix is good are you happy for me to add you as a co-author on the PR?

EDIT: Sorry, I think my fix was incorrect!

@jakecastelli
Copy link
Contributor

Hi @ronag @lpinca, the doc said the callback is called when the pipeline is fully done

How do we understand its fully done here? 👀 as we can see when the callback is called, the dst (writable stream) is not closed.

@lpinca
Copy link
Member

lpinca commented Jun 8, 2024

IIRC the first error wins. The pipeline is "done" and all streams destroyed when the first error occurs.

@jakecastelli
Copy link
Contributor

so if I understand correctly "done" here means all the streams have been destroyed but not guaranteed closed?

@lpinca
Copy link
Member

lpinca commented Jun 8, 2024

Yes.

@wh0
Copy link
Contributor Author

wh0 commented Jun 8, 2024 •
edited

about read not implemented: that was unintentional. wasn't new stream.Readable() the way to make readable stream where you'd call .push(chunk) and .push(null)? it should use the default read implementation that waits for something to call push.

I was using the closed property to check that the stream was destroyed, done being destroyed, specifically. now that I look more into the streams API, I see that you can configure streams not to emit a close event when you destroy them. that complicates things.

for longer pipelines, it looks like the current logic would wait for about half of the streams to emit error before calling the callback, and that also feels weird.

jakecastelli added a commit to jakecastelli/node that referenced this issue Jun 15, 2024
@jakecastelli
stream: wait for close before calling the callback
13d24b0 
The pipeline should wait for close event to finish before calling
the callback.

The `finishCount` should not below 0 when calling finish function.

Fixes: nodejs#51540
@jakecastelli jakecastelli linked a pull request Jun 15, 2024 that will close this issue
Open
jakecastelli added a commit to jakecastelli/node that referenced this issue Jun 15, 2024
@jakecastelli
stream: wait for close before calling the callback
de856a4 
The pipeline should wait for close event to finish before calling
the callback.

The `finishCount` should not below 0 when calling finish function.

Fixes: nodejs#51540
jakecastelli added a commit to jakecastelli/node that referenced this issue Jun 16, 2024
@jakecastelli @wh0
stream: pipeline wait for close before calling the callback
0154e63 
The pipeline should wait for close event to finish before calling
the callback.

The `finishCount` should not below 0 when calling finish function.

Fixes: nodejs#51540

Co-authored-by: wh0 <wh0@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
4 participants
@wh0 @lpinca @RedYetiDev @jakecastelli