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

Revert "stream: make finished call the callback if the stream is closed" #29717

Closed
wants to merge 2 commits into from
Closed

Revert "stream: make finished call the callback if the stream is closed" #29717

wants to merge 2 commits into from

Conversation

mcollina
Copy link
Member

Fixes #29699.

This reverts commit b03845b.

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

@mcollina mcollina added semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem. and removed abandoned labels Sep 26, 2019
@mcollina
Copy link
Member Author

cc @ronag

@mcollina
Copy link
Member Author

@BethGriggs this would need to be pulled in v13 and/or I would remove b03845b from there asap anyway.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25714/

@mcollina
Copy link
Member Author

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2012/

@ronag
Copy link
Member

ronag commented Sep 26, 2019 •
edited
Loading

I'm a big -1 on this.

Instead of reverting the whole thing, please at least let me make a PR that addresses the specific behavioural change referenced in #29699.

Including:

  • Documentation for the behaviour.
  • A big note in the code.
  • Added test for expected compat.
  • Workaround for relevant places (if any) in core that uses finished e.g. async iterators.

I can have this done by the end of the day.

Also, I would ask us to continue the discussion in #29699 for finding a long term sustainable strategy for this and similar issues. This indirectly impacts pending work and PR's I have that could end up in a similar situation.

With this I would be -0.

benjamingr
benjamingr approved these changes Sep 26, 2019
View reviewed changes
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.

I would prefer it if we picked the original one out before the release.

@jasnell
Copy link
Member

jasnell commented Sep 26, 2019

So long as the changes do not go out in a release and @ronag and @mafintosh can collaborate on a revised acceptable solution, then I think we can afford to hold off reverting this immediately. I think we should time box that however, and the change should definitely be pulled out of the 13.0.0 working branch for now while the fix is being worked on.

@ronag
Copy link
Member

ronag commented Sep 26, 2019 •
edited
Loading

I believe I have found an elegant solution which fixes #29699 and possibly make everyone happy (fingers crossed). #29724

@Trott
Copy link
Member

Trott commented Oct 1, 2019

@mcollina Unfortunately, there's a merge conflict to be resolved.

@jasnell Do you have a proposal for what should be the time-box deadline? I suppose if @BethGriggs has an opinion, that's probably the opinion we should pay the most attention to.

@BethGriggs
Copy link
Member

I'd prefer not picking the commit out of the v13.x branch and release - it has already gone into the first RC, and would need to be forced out of a protected branch.

I can pull in this PR or #29724 into the v13.x release once either of those has landed (subject to no TSC objections as per https://github.com/nodejs/node/blob/master/doc/releases.md#release-branch). My plan is to update the v13.x-proposal each Tuesday up until 15th October - after which I'd like to avoid pulling any additional commits into the release.

@mcollina
Copy link
Member Author

mcollina commented Oct 1, 2019

Then I think we should land this asap and then reapply the PR with the fix if we can land it in time.

@jasnell
Copy link
Member

jasnell commented Oct 1, 2019

I'm happy with whichever approach @BethGriggs and @mcollina are

@Trott
Copy link
Member

Trott commented Oct 1, 2019

Am I correct that the two options are either rebase this and land it soon, or we can land #29724? (And in either event, the fix needs to get into the 13.x release.) If we know which one we're going with, we can add it to the 13.x milestone.

@mcollina
Copy link
Member Author

mcollina commented Oct 9, 2019

Given that we are running out of time, I propose we land this as soon as possible (tomorrow) as a precaution. That commit should not reach v13 as-is and we should likely do a an RC release without it.

@mafintosh
Copy link
Member

Yes, I'm +1 on reverting at this stage as well. This is all super tricky stuff and we need to understand the changes better before trying to land a fix.

@Trott Trott added this to the 13.0.0 milestone Oct 9, 2019
@Trott
Copy link
Member

Trott commented Oct 9, 2019

Given that we are running out of time, I propose we land this as soon as possible (tomorrow) as a precaution. That commit should not reach v13 as-is and we should likely do a an RC release without it.

Yes, I'm +1 on reverting at this stage as well. This is all super tricky stuff and we need to understand the changes better before trying to land a fix.

Based on this, I've added it to the 13.x milestone.

@mcollina This needs a rebase.

Trott
Trott approved these changes Oct 9, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM if CI passes and CITGM doesn't show anything surprising and alarming.

@Trott
Copy link
Member

Trott commented Oct 9, 2019

@nodejs/tsc This is labeled semver-major so will need a second TSC review.

@ronag
Copy link
Member

ronag commented Oct 9, 2019

Also needs to revert ce62e96 to avoid the conflict.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 9, 2019 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/25930/ ✔️

@ronag ronag mentioned this pull request Oct 9, 2019
Closed
3 tasks
@Trott

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member

Trott commented Oct 9, 2019 •
edited
Loading

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2043/

This time, without a typo in the ref so it might actually run the tests.

@mcollina
Copy link
Member Author

CITGM seems ok to me, landing.

mcollina added a commit that referenced this pull request Oct 10, 2019
@mcollina
Revert "stream: remove ambiguous code"
8c60685 
This reverts commit ce62e96.

PR-URL: #29717
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
mcollina added a commit that referenced this pull request Oct 10, 2019
@mcollina
Revert "stream: make finished call the callback if the stream is closed"
9f873b3 
This reverts commit b03845b.

PR-URL: #29717
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@mcollina
Copy link
Member Author

Landed in 7682874...9f873b3

@mcollina mcollina closed this Oct 10, 2019
@mcollina
Copy link
Member Author

@BethGriggs would you mind adding this to v13?

@BethGriggs
Copy link
Member

@mcollina I've pulled it into #29504 and I'm currently building rc-2

@ronag ronag mentioned this pull request Oct 10, 2019
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs BethGriggs approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

@mafintosh mafintosh mafintosh approved these changes

@sam-github sam-github Awaiting requested review from sam-github

@jasnell jasnell Awaiting requested review from jasnell

@addaleax addaleax Awaiting requested review from addaleax

@lpinca lpinca Awaiting requested review from lpinca

Assignees
No one assigned
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
13.0.0
Development

Successfully merging this pull request may close these issues.

stream.finished behaviour change
9 participants
@mcollina @nodejs-github-bot @ronag @jasnell @Trott @BethGriggs @mafintosh @benjamingr @MylesBorins