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: fix regression on duplex end #35941

Merged
merged 1 commit into from
Nov 4, 2020
Merged

Conversation

mmomtchev
Copy link
Contributor

@mmomtchev mmomtchev commented Nov 3, 2020

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

Fixes: #35926

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@ronag
Copy link
Member

ronag commented Nov 3, 2020

Missing test. Also I would like to understand how this can fix that issue. This seems wrong to me.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
dst.removeAllListeners();
}

dst.on('data', data => console.log(data));
Copy link
Member

Choose a reason for hiding this comment

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

please remove the console.log and use a common.mustCall() with an assertion instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reshuffled it a little bit, 'data' doesn't get called on correct execution - but must be there in order to trigger the reading

read: common.mustCallAtMost(() => {
if (loops--)
src.push(Buffer.alloc(20000));
}, 2)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a mustCall(), not mustCallAtMost(). The algorithm is deterministic.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@nodejs-github-bot
Copy link
Collaborator

@mmomtchev
Copy link
Contributor Author

Missing test. Also I would like to understand how this can fix that issue. This seems wrong to me.

Obtained by blindly comparing NODE_DEBUG outputs of node 12 vs node-master - they remain remarkably similar despite the fact that the code base has been throughly refactored

@mmomtchev
Copy link
Contributor Author

Missing test. Also I would like to understand how this can fix that issue. This seems wrong to me.

Obtained by blindly comparing NODE_DEBUG outputs of node 12 vs node-master - they remain remarkably similar despite the fact that the code base has been throughly refactored

Maybe it just reproduces the bug that made it working 😄

@ronag
Copy link
Member

ronag commented Nov 3, 2020

I feel there is a more fundamental issue here and suspect that this might just hide it.

@mmomtchev
Copy link
Contributor Author

I feel there is a more fundamental issue here and suspect that this might just hide it.

I spent two hours yesterday evening and couldn't fully understand it. My theory is that returning false from _write pauses the stream and gives the 'end' chance to get delivered. Otherwise everything remains stuck in an endless loop with the 'end' waiting in a nextTick handler.

@ronag
Copy link
Member

ronag commented Nov 3, 2020

I mean part of the problem here is that Readable.push(null) does not Writable.end() the writable side of the Transform.

We could fix that by:

if (this.readableEnded) {
  this.end()
}

inside the transform callback but that would cause a write after end error as I mentioned here.

@ronag
Copy link
Member

ronag commented Nov 3, 2020

Just to be clear that I don't mind landing this PR to fix behavior regression (it has some perf regression for sync case) but I don't think it actually solves the fundamental problem here.

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, I think this is fine.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@nodejs-github-bot
Copy link
Collaborator

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.

Echoing Robert's "the fundamental issue needs addressing but this can land in the meantime" sentiment.

@puzrin
Copy link

puzrin commented Nov 3, 2020

Sorry if naive question. When this can be released?

@mcollina
Copy link
Member

mcollina commented Nov 3, 2020

This can land in another 40 hours. Then it'll ship in the next v15 in the next week or so. It needs to sit in a current release before being bumped to LTS for 2 weeks. My best guess is at least a month, if not a month and a half.

Given that it seems a pretty urgent regression, we might be ok in short-circuiting this. What do @nodejs/tsc @nodejs/releasers @nodejs/lts think?

@richardlau
Copy link
Member

@mcollina Do you know when the regression went out in the LTS releases? I'm open to short-circuiting the wait time if it's a recent regression and this PR has sign off from @nodejs/streams.

@mcollina
Copy link
Member

mcollina commented Nov 3, 2020

@richardlau I think this has been there for most of v14 current cycle - not recent at all. It's not in v12.

@puzrin
Copy link

puzrin commented Nov 3, 2020

Just checked. This bug appeared in v14.1.0. Previous versions not affected.

@richardlau
Copy link
Member

@targos has been preparing a 15.x release for tomorrow, so if this should be urgently fixed perhaps fast tracking into tomorrow's release is an option? We pencilled in a 14.x release for this month but AFAIK haven't settled on an exact date but if this does go out in tomorrow's 15.x release it'll probably make it into the next 14.x release without needing to short circuit LTS wait times.

@mcollina
Copy link
Member

mcollina commented Nov 3, 2020

Then let's fast-track this

@mcollina mcollina added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 3, 2020
@mcollina
Copy link
Member

mcollina commented Nov 3, 2020

cc @nodejs/tsc for awareness

@Trott
Copy link
Member

Trott commented Nov 20, 2020

@Trott , @lpinca was this supposed to be included? I am sorry if it was up to me to backport it? What is the usual process when a PR needs backporting?

I think releasers will cherry-pick and leave a note here if it doesn't land cleanly. @nodejs/releasers Should a backport to 14.x be opened at this time? Or hold off?

@mmomtchev If you're confident that this will not be able to be cherry-picked to the v14.x-staging branch and that it should be included, you can get open a backport release by following the instructions at https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md.

@puzrin
Copy link

puzrin commented Nov 20, 2020

May be reopen #35926 until fix in v14 (LTS)? Bug is serious, IMO.

@mmomtchev
Copy link
Contributor Author

I am confident, the file I modified does not exist on v14.x

mmomtchev added a commit to mmomtchev/node that referenced this pull request Nov 22, 2020
Backport
Decide the return status of writeOrBuffer before
calling stream.write which can reset state.length

Refs: nodejs#35941
Fixes: nodejs#35926
@mmomtchev
Copy link
Contributor Author

@puzrin @Trott I submitted a PR to v14.x-staging with a backport of this change

codebytere pushed a commit that referenced this pull request Nov 22, 2020
- Remove unneeded listener for the `'error'` event.
- Use `common.mustCall()`.
- Verify that the `src` stream gets paused.

PR-URL: #36056
Refs: #35941
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
enov pushed a commit to enov/node that referenced this pull request Dec 4, 2020
Backport
Decide the return status of writeOrBuffer before
calling stream.write which can reset state.length

Refs: nodejs#35941
Fixes: nodejs#35926
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
Decide the return status of writeOrBuffer before
calling stream.write which can reset state.length

Add unit test for #35926

Refs: #35926

Backport-PR-URL: #36375
PR-URL: #35941
Fixes: #35926
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
- Remove unneeded listener for the `'error'` event.
- Use `common.mustCall()`.
- Verify that the `src` stream gets paused.

PR-URL: #36056
Refs: #35941
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Decide the return status of writeOrBuffer before
calling stream.write which can reset state.length

Add unit test for #35926

Refs: #35926

Backport-PR-URL: #36375
PR-URL: #35941
Fixes: #35926
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
- Remove unneeded listener for the `'error'` event.
- Use `common.mustCall()`.
- Verify that the `src` stream gets paused.

PR-URL: #36056
Refs: #35941
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs added a commit that referenced this pull request Dec 15, 2020
Notable Changes:

- **deps**:
  - upgrade npm to 6.14.9 (Myles Borins)
    #36450
  - update acorn to v8.0.4 (Michaël Zasso)
    #35791
- **doc**: add release key for Danielle Adams (Danielle Adams)
    #35545
- **http2**: check write not scheduled in scope destructor (David Halls)
    #36241
- **stream**: fix regression on duplex end (Momtchil Momtchev)
    #35941

PR-URL: #36476
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
Decide the return status of writeOrBuffer before
calling stream.write which can reset state.length

Add unit test for #35926

Refs: #35926

Backport-PR-URL: #36375
PR-URL: #35941
Fixes: #35926
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
- Remove unneeded listener for the `'error'` event.
- Use `common.mustCall()`.
- Verify that the `src` stream gets paused.

PR-URL: #36056
Refs: #35941
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs added a commit that referenced this pull request Dec 15, 2020
Notable Changes:

- **deps**:
  - upgrade npm to 6.14.9 (Myles Borins)
    #36450
  - update acorn to v8.0.4 (Michaël Zasso)
    #35791
- **doc**: add release key for Danielle Adams (Danielle Adams)
    #35545
- **http2**: check write not scheduled in scope destructor (David Halls)
    #36241
- **stream**: fix regression on duplex end (Momtchil Momtchev)
    #35941

PR-URL: #36476
BethGriggs added a commit that referenced this pull request Dec 15, 2020
Notable Changes:

- **deps**:
  - upgrade npm to 6.14.9 (Myles Borins)
    #36450
  - update acorn to v8.0.4 (Michaël Zasso)
    #35791
- **doc**: add release key for Danielle Adams (Danielle Adams)
    #35545
- **http2**: check write not scheduled in scope destructor (David Halls)
    #36241
- **stream**: fix regression on duplex end (Momtchil Momtchev)
    #35941

PR-URL: #36476
@targos targos added backported-to-v14.x stream Issues and PRs related to the stream subsystem. and removed fast-track PRs that do not need to wait for 48 hours to land. labels Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stream: regression in v14, this.push(null) in Transform doesn't emit end
10 participants