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

stream: eos premature close #28720

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 16, 2019

For writable premature close is close before finish, i.e. it should look for finished instead of ended. Would be better with a finishEmitted but we don't have that.

For readable we should be checking endEmitted (which we do first but not later).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag
Copy link
Member Author

ronag commented Jul 16, 2019

I'm unsure how to make tests that actually trigger this... but the semantics I think are more correct like this

@ronag ronag force-pushed the stream-pipeline-premature branch from 554b8e0 to 291d05c Compare July 16, 2019 18:12
@ronag
Copy link
Member Author

ronag commented Jul 16, 2019

This actually fixes a bug (close before end should be an error). Though it should probably be sem-major due to the updated test.

@ronag
Copy link
Member Author

ronag commented Jul 17, 2019

@benjamingr who's a good ping on this one?

@benjamingr
Copy link
Member

I don't mind reviewing it - can you explain the tradeoffs in the new behavior vs. the existing one?

@ronag
Copy link
Member Author

ronag commented Jul 18, 2019

@benjamincburns close before finish (writable) and close before end (readable) should be premature close error. Which it isn't in every case before this PR (see test update).

@benjamingr
Copy link
Member

@nodejs/streams

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a green citgm run

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 18, 2019
@mcollina
Copy link
Member

Marking it as semver-major as it changes behavior.

@mcollina
Copy link
Member

What is the bug this is fixing? What’s the goal of this change?

@ronag
Copy link
Member Author

ronag commented Jul 18, 2019

@mcollina

What is the bug this is fixing? What’s the goal of this change?

close before finish (writable) and close before end (readable) should be premature close error. Which it isn't in every case before this PR (see test update).

@ronag ronag force-pushed the stream-pipeline-premature branch 2 times, most recently from afd02b1 to 1ee52d4 Compare July 18, 2019 11:23
@benjamingr
Copy link
Member

@ronag just to elaborate on Matteo's comment regarding semverness - changes to the order of the events streams (or in general node APIs) fire are pretty substantial changes with possible big implications on the ecosystem so generally they're all semver-major (means they'll be released in the next major release - in this case 13)

@ronag
Copy link
Member Author

ronag commented Jul 18, 2019

@benjamingr no problem, makes perfect sense

This was referenced Aug 6, 2019
@ronag ronag force-pushed the stream-pipeline-premature branch from 1ee52d4 to 77ff0d3 Compare August 9, 2019 16:06
@ronag
Copy link
Member Author

ronag commented Aug 28, 2019

updated test

@ronag ronag mentioned this pull request Aug 28, 2019
4 tasks
@ronag
Copy link
Member Author

ronag commented Sep 1, 2019

merged into #28748

@ronag ronag closed this Sep 1, 2019
ronag added a commit to nxtedition/node that referenced this pull request Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants