-
Notifications
You must be signed in to change notification settings - Fork 30k
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
streams: fix enqueue race condition on esm modules #40901
streams: fix enqueue race condition on esm modules #40901
Conversation
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.
It's not the WHATWG streams. Those are implemented correctly according to the spec. It's just that finished
uses process.nextTick
instead.
cc @jasnell |
@szmarczak is that c4f0058 that you expect? |
4e0348f
to
303211b
Compare
cc @nodejs/modules |
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.
The added test doesn't fail before the fix.
I don't think this is necessarily something Modules needs to do anything with, it looks like it is just because |
You're right, I didn't notice it before the changes. I was using a
I felt the same when looking at it, looks like a conditional flag before closing the stream is needed. But, I don't have enough knowledge in the |
readableStreamDefaultControllerClose(branch2[kState].controller); | ||
if (!canceled1 || !canceled2) | ||
cancelPromise.resolve(); | ||
process.nextTick(() => { |
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.
No, I meant that opposite. I think https://github.com/nodejs/node/blob/master/lib/internal/streams/end-of-stream.js should be queueMicrotask
instead of process.nextTick
but I'm not sure 🤔
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 did a simple test by just calling console.log
on eos
and it doesn't call, so not sure either if the end-of-stream is the best place. Looks like the kClose
is suitable for that otherwise my second comment #40901 (comment) is valid.
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 add a comment here explaining that the addition of the process.nextTick is not part of the spec and why it's needed with a link back to the original issue.
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.
Done.
303211b
to
37b200c
Compare
@targos Just created a new test with fails without this modification (.mjs) |
I'll have to stew over this change a bit. Changing the timing on the close using nextTick just doesn't feel right and changes the timing guarantees from what is spec'd. But, I need to think through about whether that's going to be a problem or not. I'm not giving this a thumbs down, just need to think about it more. |
I’m also wondering if this is a breaking change. Also how does equivalent code behave in browsers? This seems like it’s essentially a spec question, as in are we following the spec-defined behavior. If browsers behave differently then Node does now (and like how this PR changes Node to behave) that would be a strong argument for making the change, because it would imply that Node’s implementation is incorrect. |
37b200c
to
4e10155
Compare
Please don't forget to label with streams and ping @nodejs/streams on these kind of PRs. |
@jasnell Have you considered the following example? #39758 (comment) |
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'm still not 100% sure this is the right approach but LGTM for now with the addition of some comments in the code explaining why the change is made
@targos can you take a look again? |
4e10155
to
f365cdd
Compare
Commit Queue failed- Loading data for nodejs/node/pull/40901 ✔ Done loading data for nodejs/node/pull/40901 ----------------------------------- PR info ------------------------------------ Title streams: fix enqueue race condition on esm modules (#40901) Author Rafael Gonzaga (@RafaelGSS) Branch RafaelGSS:fix/readable-web-stream -> nodejs:master Labels stream, web streams Commits 1 - stream: fix enqueue race condition on esm modules Committers 1 - RafaelGSS PR-URL: https://github.com/nodejs/node/pull/40901 Reviewed-By: Robert Nagy Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/40901 Reviewed-By: Robert Nagy Reviewed-By: James M Snell -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - stream: fix enqueue race condition on esm modules ℹ This PR was created on Sun, 21 Nov 2021 02:45:45 GMT ✔ Approvals: 2 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/40901#pullrequestreview-834119884 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/40901#pullrequestreview-834121218 ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2021-12-20T19:16:43Z: https://ci.nodejs.org/job/node-test-pull-request/41565/ - Querying data for job/node-test-pull-request/41565/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/1604180725 |
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
Landed in 113133b |
stream: use nextTick on close PR-URL: #40901 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
stream: use nextTick on close PR-URL: #40901 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
stream: use nextTick on close PR-URL: nodejs#40901 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
stream: use nextTick on close PR-URL: #40901 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Address: #39758