Skip to content

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

Closed
mmomtchev wants to merge 1 commit intonodejs:v14.x-stagingfrom
mmomtchev:backport-35941-to-v14.x
Closed

[14.x] stream: fix regression on duplex end#36229
mmomtchev wants to merge 1 commit intonodejs:v14.x-stagingfrom
mmomtchev:backport-35941-to-v14.x

Conversation

@mmomtchev
Copy link
Contributor

@mmomtchev mmomtchev commented Nov 22, 2020

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

nodejs-github-bot commented Nov 22, 2020

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

mcollina commented Nov 23, 2020

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

BethGriggs commented Dec 7, 2020

👋 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

BethGriggs commented Dec 7, 2020

@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

mmomtchev commented Dec 7, 2020

@BethGriggs these are two 100% identical PRs

@BethGriggs
Copy link
Member

BethGriggs commented Dec 9, 2020

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