-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Conversation
Review requested:
|
Missing test. Also I would like to understand how this can fix that issue. This seems wrong to me. |
dst.removeAllListeners(); | ||
} | ||
|
||
dst.on('data', data => console.log(data)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
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 😄 |
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 |
I mean part of the problem here is that 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. |
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. |
There was a problem hiding this 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.
There was a problem hiding this 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.
Sorry if naive question. When this can be released? |
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? |
@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. |
@richardlau I think this has been there for most of v14 current cycle - not recent at all. It's not in v12. |
Just checked. This bug appeared in v14.1.0. Previous versions not affected. |
@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. |
Then let's fast-track this |
cc @nodejs/tsc for awareness |
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. |
May be reopen #35926 until fix in v14 (LTS)? Bug is serious, IMO. |
I am confident, the file I modified does not exist on v14.x |
Backport Decide the return status of writeOrBuffer before calling stream.write which can reset state.length Refs: nodejs#35941 Fixes: nodejs#35926
- 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>
Backport Decide the return status of writeOrBuffer before calling stream.write which can reset state.length Refs: nodejs#35941 Fixes: nodejs#35926
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>
- 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>
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>
- 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>
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
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>
- 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>
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
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
Decide the return status of writeOrBuffer before
calling stream.write which can reset state.length
Fixes: #35926
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes