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

streams: fix enqueue race condition on esm modules #40901

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

RafaelGSS
Copy link
Member

Address: #39758

Copy link
Member

@szmarczak szmarczak left a 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.

@mcollina
Copy link
Member

cc @jasnell

@RafaelGSS
Copy link
Member Author

@szmarczak is that c4f0058 that you expect?

@ljharb
Copy link
Member

ljharb commented Nov 22, 2021

cc @nodejs/modules

targos
targos previously requested changes Nov 22, 2021
Copy link
Member

@targos targos left a 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.

@bmeck
Copy link
Member

bmeck commented Nov 22, 2021

I don't think this is necessarily something Modules needs to do anything with, it looks like it is just because import is doing async work that it is showing up? It does look weird though and IDK if this is the right fix since it looks like the close() operation is happening to close both streams before the 2nd gets the data buffered? Not familiar w/ this streams impl but it doesn't seem to be specific to ESM itself.

@RafaelGSS
Copy link
Member Author

The added test doesn't fail before the fix. -- @targos

You're right, I didn't notice it before the changes. I was using a .mjs to validate locally. Fixing it soon.

if this is the right fix since it looks like the close() operation is happening to close both streams before the 2nd gets the data buffered? -- @bmeck

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 streams/web to assume that.

readableStreamDefaultControllerClose(branch2[kState].controller);
if (!canceled1 || !canceled2)
cancelPromise.resolve();
process.nextTick(() => {
Copy link
Member

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 🤔

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@RafaelGSS
Copy link
Member Author

@targos Just created a new test with fails without this modification (.mjs)

@jasnell
Copy link
Member

jasnell commented Nov 23, 2021

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.

@GeoffreyBooth
Copy link
Member

changes the timing guarantees from what is spec’d

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.

@ronag
Copy link
Member

ronag commented Dec 16, 2021

Please don't forget to label with streams and ping @nodejs/streams on these kind of PRs.

@ronag ronag added stream Issues and PRs related to the stream subsystem. web streams labels Dec 16, 2021
@ronag
Copy link
Member

ronag commented Dec 16, 2021

@jasnell Have you considered the following example? #39758 (comment)

@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Dec 16, 2021
Copy link
Member

@jasnell jasnell left a 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

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

@mcollina
Copy link
Member

@targos can you take a look again?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 20, 2021
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 20, 2021
@nodejs-github-bot
Copy link
Collaborator

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/.ncu
https://github.com/nodejs/node/actions/runs/1604180725

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 mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 21, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2021
@nodejs-github-bot nodejs-github-bot merged commit 113133b into nodejs:master Dec 21, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 113133b

targos pushed a commit that referenced this pull request Jan 14, 2022
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>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
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>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
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>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
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. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.