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.

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

Readable[AsyncIterator] doesn't handle premature close #39086

Closed
ronag opened this issue Jun 19, 2021 · 11 comments
Closed

Readable[AsyncIterator] doesn't handle premature close #39086

ronag opened this issue Jun 19, 2021 · 11 comments
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Jun 19, 2021

createAsyncIterator should throw ERR_STREAM_PREMATURE_CLOSE if 'close' is emitted before 'end'. Right now I think it just silently succeeds.

@ronag ronag added stream Issues and PRs related to the stream subsystem. good first issue Issues that are suitable for first-time contributors. labels Jun 19, 2021
@daukadolt
Copy link

Sounds interesting, can I work on the issue?

@daukadolt
Copy link

daukadolt commented Jun 21, 2021

@ronag can you suggest the correct way to debug node's internal modules?

I'm using Webstorm, and the debugger seems to ignore my breakpoints when it gets into internal modules, but works fine otherwise.

Screen Shot 2021-06-22 at 12 15 31 AM

@ronag
Copy link
Member Author

ronag commented Jun 21, 2021

Sorry. I don’t use breakpoint debugging when working with node.

@RaisinTen
Copy link
Contributor

PR: #39117

@daukadolt
Copy link

daukadolt commented Jun 22, 2021

That's confusing and discouraging from your side @RaisinTen.

@ronag can you at least give a feedback on my PR?

Upd: now that I've looked at RaisinTen's PR I see that there are still some parts that I'm missing.

What does this part do?

if (opts.destroyOnReturn || state.autoDestroy) {
        destroyImpl.destroyer(stream, err);
}

@RaisinTen
Copy link
Contributor

I'm sorry, I thought I could work on this as you didn't publish your PR.

@RaisinTen
Copy link
Contributor

What does this part do?

if (opts.destroyOnReturn || state.autoDestroy) {
        destroyImpl.destroyer(stream, err);
}

@daukadolt It destroys the stream if either the destroyOnReturn option or the autoDestroy option have been passed.

@daukadolt
Copy link

But I was still working on it 😞 I don't remember any time limit on PR from Contributing.md.

I'm new to the project, so it took me a while to read Contributing.md and all MD's that followed + Readable's internals

@RaisinTen
Copy link
Contributor

There is no time limit on PRs. Anyone can work on any issue whenever they want to without asking for any permission. I didn't know much about the internal code of readable streams either, so I thought it would be nice to shoot a PR and learn few things about it.

@daukadolt
Copy link

Can we agree to let me finish the ticket then, @RaisinTen?

@RaisinTen
Copy link
Contributor

@daukadolt No one is stopping you. Please feel free to continue your work. :)

RaisinTen added a commit to RaisinTen/node that referenced this issue Jul 5, 2021
RaisinTen added a commit to RaisinTen/node that referenced this issue Jul 5, 2021
ejose19 pushed a commit to ejose19/node that referenced this issue Jul 5, 2021
Fixes: nodejs#39086
Signed-off-by: Darshan Sen <[email protected]>

PR-URL: nodejs#39117
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem.
Projects
None yet
3 participants