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

[14.x] stream: fix regression on duplex end #36229

Closed
wants to merge 1 commit into from
Closed

[14.x] stream: fix regression on duplex end #36229

wants to merge 1 commit into from

Conversation

mmomtchev
Copy link
Contributor

Backport
Decide the return status of writeOrBuffer before
calling stream.write which can reset state.length

Refs: #35941
Fixes: #35926

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

@mmomtchev
stream: fix regression on duplex end
c6fa9a6 
Backport
Decide the return status of writeOrBuffer before
calling stream.write which can reset state.length

Refs: nodejs#35941
Fixes: nodejs#35926
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added stream Issues and PRs related to the stream subsystem. v14.x labels Nov 22, 2020
mcollina
mcollina approved these changes Nov 23, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

cc @nodejs/releasers this is a backport.

@richardlau richardlau changed the title stream: fix regression on duplex end [14.x] stream: fix regression on duplex end Nov 23, 2020
@schmod schmod mentioned this pull request Nov 23, 2020
Closed
@schmod
Copy link

schmod commented Nov 24, 2020 •
edited
Loading

NB: This fixes a fairly major regression that broke the fairly-popular ssh2 library.

@BethGriggs BethGriggs added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@enov enov mentioned this pull request Dec 4, 2020
Closed
3 tasks
@BethGriggs
Copy link
Member

👋 I'm working on preparing the next v14.x release.

#35349 landed on v14.x-staging, which has introduced conflicts here. Could you advise whether this PR should be updated or #36375 should land instead?

@mcollina
Copy link
Member

mcollina commented Dec 7, 2020

@BethGriggs what's the timeline for the next 14 release?

@BethGriggs
Copy link
Member

@mcollina no concrete dates yet, but I am hoping we can get a patch release out before the holidays. My (probably ambitious) plan is to get the proposal ready by tomorrow/Weds and aim for Tues 15th release date. That reduces our typical proposal baking time to ~1 week but that seems preferable to releasing on 22nd Dec or not getting one out at all.

@mcollina
Copy link
Member

mcollina commented Dec 7, 2020

I would just land #36375 and skip this.

@mmomtchev
Copy link
Contributor Author

@BethGriggs these are two 100% identical PRs

@BethGriggs
Copy link
Member

Closing as #36375 has landed.

@BethGriggs BethGriggs closed this Dec 9, 2020
@enov
Copy link

enov commented Dec 9, 2020

Thank you! @mmomtchev we're affected by this and I thought to expedite by resolving the conflict. Did not mean to steal anybody's work and apologies if it did look that way. Thank you all for your hard work ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mmomtchev @nodejs-github-bot @mcollina @schmod @BethGriggs @enov @benjamingr