streams: fix enqueue race condition on esm modules#40901

RafaelGSS · 2021-11-21T02:45:45Z

Address: #39758

szmarczak

It's not the WHATWG streams. Those are implemented correctly according to the spec. It's just that finished uses process.nextTick instead.

mcollina · 2021-11-21T08:02:58Z

cc @jasnell

RafaelGSS · 2021-11-21T12:51:50Z

@szmarczak is that c4f0058 that you expect?

ljharb · 2021-11-22T16:53:25Z

cc @nodejs/modules

targos

The added test doesn't fail before the fix.

bmeck · 2021-11-22T17:02:17Z

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 · 2021-11-22T17:10:58Z

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.

szmarczak · 2021-11-22T21:21:37Z

lib/internal/webstreams/readablestream.js

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


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 🤔

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.

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.

RafaelGSS · 2021-11-22T21:24:35Z

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

jasnell · 2021-11-23T15:38:19Z

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 · 2021-11-23T17:00:11Z

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.

test/parallel/test-whatwg-readablestream.mjs

ronag · 2021-12-16T13:39:03Z

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

ronag · 2021-12-16T13:40:27Z

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

jasnell

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

nodejs-github-bot · 2021-12-16T13:58:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/41516/

mcollina · 2021-12-16T14:11:23Z

@targos can you take a look again?

resolved

nodejs-github-bot · 2021-12-20T18:05:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/41564/

nodejs-github-bot · 2021-12-20T19:16:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/41565/

nodejs-github-bot · 2021-12-20T22:07:56Z

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

mcollina

lgtm

nodejs-github-bot · 2021-12-21T10:24:16Z

Landed in 113133b

stream: use nextTick on close PR-URL: #40901 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

stream: use nextTick on close PR-URL: nodejs#40901 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

stream: use nextTick on close PR-URL: #40901 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 21, 2021

RafaelGSS mentioned this pull request Nov 21, 2021
readableStream.tee() doesn't work when using process.nextTick(...)#39758
Closed

szmarczak suggested changes Nov 21, 2021
View reviewed changes

RafaelGSS force-pushed the fix/readable-web-stream branch from 4e0348f to 303211bCompare November 22, 2021 16:50

targos previously requested changes Nov 22, 2021
View reviewed changes

szmarczak reviewed Nov 22, 2021
View reviewed changes

RafaelGSS force-pushed the fix/readable-web-stream branch from 303211b to 37b200cCompare November 22, 2021 21:23

simoneb reviewed Nov 23, 2021
View reviewed changes

test/parallel/test-whatwg-readablestream.mjs Outdated Show resolvedHide resolved

RafaelGSS force-pushed the fix/readable-web-stream branch from 37b200c to 4e10155Compare November 24, 2021 21:30

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

ronag approved these changes Dec 16, 2021
View reviewed changes

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

jasnell approved these changes Dec 16, 2021
View reviewed changes

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2021

RafaelGSS force-pushed the fix/readable-web-stream branch from 4e10155 to f365cddCompare December 17, 2021 00:46

This was referenced Dec 18, 2021
CI Reliability 2021-12-18 nodejs/reliability#148
Open
CI Reliability 2021-12-19 nodejs/reliability#149
Open
CI Reliability 2021-12-20 nodejs/reliability#150
Open

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 20, 2021

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

github-actionsbot mentioned this pull request Dec 21, 2021
CI Reliability 2021-12-21 nodejs/reliability#151
Open
22 tasks

mcollina approved these changes Dec 21, 2021
View reviewed changes

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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2021

nodejs-github-bot merged commit 113133b into nodejs:masterDec 21, 2021

mcollina added dont-land-on-v12.x labels Dec 21, 2021

targos mentioned this pull request Jan 16, 2022
v17.4.0 release proposal #41557
Merged

danielleadams mentioned this pull request Feb 1, 2022
v16.14.0 proposal #41804
Merged

Uh oh!

streams: fix enqueue race condition on esm modules#40901

streams: fix enqueue race condition on esm modules #40901

Uh oh!

Conversation

RafaelGSS commented Nov 21, 2021

Uh oh!

szmarczak left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Nov 21, 2021

Uh oh!

RafaelGSS commented Nov 21, 2021

Uh oh!

ljharb commented Nov 22, 2021

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

bmeck commented Nov 22, 2021

Uh oh!

RafaelGSS commented Nov 22, 2021

Uh oh!

szmarczakNov 22, 2021

Choose a reason for hiding this comment

Uh oh!

RafaelGSSNov 23, 2021

Choose a reason for hiding this comment

Uh oh!

jasnellDec 16, 2021

Choose a reason for hiding this comment

Uh oh!

RafaelGSSDec 17, 2021

Choose a reason for hiding this comment

Uh oh!

RafaelGSS commented Nov 22, 2021

Uh oh!

jasnell commented Nov 23, 2021

Uh oh!

GeoffreyBooth commented Nov 23, 2021

Uh oh!

Uh oh!

ronag commented Dec 16, 2021

Uh oh!

ronag commented Dec 16, 2021

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 16, 2021

Uh oh!

mcollina commented Dec 16, 2021

Uh oh!

nodejs-github-bot commented Dec 20, 2021

Uh oh!

nodejs-github-bot commented Dec 20, 2021

Uh oh!

nodejs-github-bot commented Dec 20, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants