streams: implement TransformStream cleanup using "transformer.cancel" #50126

debadree25 · 2023-10-10T18:02:35Z

This is a draft implementation of the spec change but it fails two WPTs a description of the failure is given below if anyone can provide any insight on it would be great

debadree25 · 2023-10-10T18:02:48Z

consider the code here https://gist.github.com/debadree25/c54931a99e493baaf9314a903cd52e12 now when we create a transform stream we have one promise created on the writable side the "startPromise" as noted in https://streams.spec.whatwg.org/#set-up-writable-stream-default-controller and when we called the cancel method on the readable we get a cancellation promise as noted in https://streams.spec.whatwg.org/#transform-stream-default-source-cancel now in the code after this we call on controller.terminate which causes the writable side to go into the "erroring" stage, now comes the interesting part the "startPromise" gets resolved and causes the writable part to go into "errored" mode after which the close promise resolves rejecting the cancellation promise we received I think this is expected behaviour? since promises are executed in a queue manner? strangely this issue doesnt seem to affect the reference implementation here https://github.com/whatwg/streams and the promises get executed in the order:
-created start promise
-cancel promise created
-cancel promise resolving
-start promise resolving.

debadree25 · 2023-10-10T18:03:17Z

a ping to @nodejs/whatwg-stream

(have scratched my head on #50126 (comment) for quite sometime incase someone might have a insight 😅)

debadree25 · 2023-10-23T13:37:13Z

Okay it so appears that there can be much larger issue see whatwg/streams#1298 not sure if we should address that everywhere before this PR or could we skip the tests and go ahead with this PR!

a ping to @nodejs/whatwg-stream again 😅

saschanaz · 2023-10-24T19:42:46Z

Your mention of @nodejs/whatwg-stream is rendered as plaintext to me, does it work as a link to you? Just curious.

debadree25 · 2023-10-25T05:55:47Z

Your mention of @nodejs/whatwg-stream is rendered as plaintext to me, does it work as a link to you? Just curious.

Oh afaik the different github teams are only visible to the members of nodejs org thats why

MattiasBuelens · 2023-11-23T22:03:41Z

If I understand whatwg/streams#1298 correctly, the issue is in these two lines:

node/lib/internal/webstreams/readablestream.js

Line 2447 in 83df02c

PromiseResolve(startResult),

node/lib/internal/webstreams/readablestream.js

Line 3246 in 83df02c

PromiseResolve(startResult),

We're using Promise.resolve(x), whereas WebIDL expects new Promise(resolve => resolve(x)). The difference is that Promise.resolve(x) is allowed to return the original x (in case it's already a Promise), whereas new Promise() always creates a new promise and thus takes an extra microtask to resolve.

I'll try this change out on your branch, and see if that fixes the problem.

debadree25 · 2023-11-24T05:03:17Z

Thank you so much @MattiasBuelens for taking a look, I remember having done your method once but it resulted in some new failures but yes do let me know what you find!

Fixes: nodejs#49971

debadree25 · 2023-11-26T14:37:49Z

Hey @MattiasBuelens so in the latest patches I have updated to remove the PromiseResolve(startResult) with new Promise((r) => r(startResult)) in both readable and writable stream so number of errors have reduced to 1 this single test:
readable.cancel() and a parallel writable.close() should reject if a transformer.cancel() calls controller.error() i am investigating it right now, but any insight you might have here?

lib/internal/webstreams/transformstream.js

MattiasBuelens · 2023-12-04T17:14:06Z

@debadree25 Apologies for the delay. I did some digging, and it turns out it's yet another case for Promise.resolve() versus new Promise().

The specification of TransformStreamDefaultSourceCancelAlgorithm says:

Let cancelPromise be the result of performing controller.[[cancelAlgorithm]], passing reason.

You've implemented this step like this:

node/lib/internal/webstreams/transformstream.js

Lines 665 to 668 in c627596

    
           const cancelPromise = ensureIsPromise( 
        
             controller[kState].cancelAlgorithm, 
        
             controller, 
        
             reason);

which uses Promise.resolve():

node/lib/internal/webstreams/util.js

Lines 183 to 190 in c627596

    
           function ensureIsPromise(fn, thisArg, ...args) { 
        
             try { 
        
               const value = FunctionPrototypeCall(fn, thisArg, ...args); 
        
               return isPromise(value) ? value : PromiseResolve(value); 
        
             } catch (error) { 
        
               return PromiseReject(error); 
        
             } 
        
           }

On the other hand, the reference implementation generates the following helper function for the TransformerCancelCallback IDL type (see reference-implementation/generated/TransformerCancelCallback.js after running npm test):

  function invokeTheCallbackFunction(reason) {
    if (new.target !== undefined) {
      throw new Error("Internal error: invokeTheCallbackFunction is not a constructor");
    }

    const thisArg = utils.tryWrapperForImpl(this);
    let callResult;

    try {
      callResult = Reflect.apply(value, thisArg, [reason]);

      callResult = new Promise(resolve => resolve(callResult));
      return callResult;
    } catch (err) {
      return Promise.reject(err);
    }
  }

which uses new Promise().

If I change ensureIsPromise() to do this, the failure disappears:

return new Promise((r) => r(value));

Unfortunately, this leads to other unexpected failures. Hang on while I investigate those... 😅

debadree25 · 2023-12-05T10:11:00Z

wohoo this seems like a game of whack a mole haha, thank you so much for finding that out let me too investigate along it same lines, i wonder would it be ok to implement this with some of these tests not passing or would that be too much of a deviation.

MattiasBuelens · 2023-12-05T10:39:07Z

Yes, it's quite finnicky. 😛 I did some more digging last night, and I think I got all tests passing now. But it required quite a few changes, see the diff against your branch.

The main thing is that ensureIsPromise should only be used when constructing a stream from a user-land source/sink/transformer. Whenever we create an stream ourselves (like TransformStream.readable or ReadableStream.from(asyncIterable) or ReadableStream.tee()), the spec uses CreateReadableStream (and CreateReadableByteStream) instead. So it skips the whole new Promise() dance from Web IDL's "convert callback result to promise" steps, which means it runs one microtask "faster". And unfortunately, there are some WPT tests that are affected by this... 🙄

It's a bit of a sidetrack though, so maybe we can mark the current test failures as "expected" for this PR and we land my changes in a separate PR?

debadree25 · 2023-12-05T11:02:01Z

@MattiasBuelens i think that makes sense i have pushed a change to mark the one test as skipped we can do a spearate PR with your changes!

debadree25 · 2023-12-05T11:03:12Z

Requesting @nodejs/whatwg-stream to please review this, things to note:

One of the test fails and that shall be fixed by @MattiasBuelens 's PR ref: streams: implement TransformStream cleanup using "transformer.cancel" #50126 (comment)

nodejs-github-bot · 2023-12-13T08:37:53Z

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

nodejs-github-bot · 2023-12-13T10:38:28Z

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

nodejs-github-bot · 2023-12-14T18:03:38Z

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

nodejs-github-bot · 2023-12-15T05:51:27Z

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

nodejs-github-bot · 2023-12-15T06:52:58Z

Commit Queue failed

- Loading data for nodejs/node/pull/50126 ✔ Done loading data for nodejs/node/pull/50126 ----------------------------------- PR info ------------------------------------ Title streams: implement TransformStream cleanup using "transformer.cancel" (#50126) Author Debadree Chatterjee (@debadree25) Branch debadree25:ft/transform-cancel-impl -> nodejs:main Labels author ready, needs-ci, web streams Commits 7 - stream: implement TransformStream cleanup using "transformer.cancel" - test: update wpts - fixup! replace promise resolve - fixup! useless import - fixup! wrong import remove - expect a failure - linting Committers 1 - Debadree Chatterjee PR-URL: https://github.com/nodejs/node/pull/50126 Fixes: https://github.com/nodejs/node/issues/49971 Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/50126 Fixes: https://github.com/nodejs/node/issues/49971 Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 10 Oct 2023 18:02:35 GMT ✔ Approvals: 1 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/50126#pullrequestreview-1778034957 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-12-15T05:51:27Z: https://ci.nodejs.org/job/node-test-pull-request/56302/ - Querying data for job/node-test-pull-request/56302/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 50126 From https://github.com/nodejs/node * branch refs/pull/50126/merge -> FETCH_HEAD ✔ Fetched commits as 5ac658102d8f..2557e8523dc4 -------------------------------------------------------------------------------- [main 49038c9224] stream: implement TransformStream cleanup using "transformer.cancel" Author: Debadree Chatterjee Date: Sun Oct 1 18:22:42 2023 +0530 1 file changed, 105 insertions(+), 10 deletions(-) [main 3f5df10706] test: update wpts Author: Debadree Chatterjee Date: Sun Oct 1 18:28:33 2023 +0530 68 files changed, 301 insertions(+), 80 deletions(-) create mode 100644 test/fixtures/wpt/streams/piping/crashtests/cross-piping.html create mode 100644 test/fixtures/wpt/streams/transform-streams/cancel.any.js Auto-merging lib/internal/webstreams/readablestream.js [main 32bb27144f] fixup! replace promise resolve Author: Debadree Chatterjee Date: Sun Nov 26 20:03:18 2023 +0530 2 files changed, 5 insertions(+), 3 deletions(-) [main e0f3339a27] fixup! useless import Author: Debadree Chatterjee Date: Sun Nov 26 20:04:55 2023 +0530 1 file changed, 2 deletions(-) [main a3deac72aa] fixup! wrong import remove Author: Debadree Chatterjee Date: Wed Nov 29 11:34:40 2023 +0530 1 file changed, 1 insertion(+) Auto-merging test/wpt/status/streams.json [main 8a28e049eb] expect a failure Author: Debadree Chatterjee Date: Tue Dec 5 16:31:19 2023 +0530 1 file changed, 7 insertions(+) [main b247e0c2e3] linting Author: Debadree Chatterjee Date: Tue Dec 5 16:42:34 2023 +0530 1 file changed, 22 insertions(+), 22 deletions(-) ✔ Patches applied There are 7 commits in the PR. Attempting autorebase. Rebasing (2/14)

Executing: git node land --amend --yes
⚠ Found Fixes: #49971, skipping..
--------------------------------- New Message ----------------------------------
stream: implement TransformStream cleanup using "transformer.cancel"

Fixes: #49971
PR-URL: #50126
Reviewed-By: Matteo Collina [email protected]

[detached HEAD ca8b3fdc3e] stream: implement TransformStream cleanup using "transformer.cancel"
Author: Debadree Chatterjee [email protected]
Date: Sun Oct 1 18:22:42 2023 +0530
1 file changed, 105 insertions(+), 10 deletions(-)
Rebasing (3/14)
Rebasing (4/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: update wpts

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

[detached HEAD e5f8ec4916] test: update wpts
Author: Debadree Chatterjee [email protected]
Date: Sun Oct 1 18:28:33 2023 +0530
68 files changed, 301 insertions(+), 80 deletions(-)
create mode 100644 test/fixtures/wpt/streams/piping/crashtests/cross-piping.html
create mode 100644 test/fixtures/wpt/streams/transform-streams/cancel.any.js
Rebasing (5/14)
Rebasing (6/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! replace promise resolve

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

[detached HEAD 1e893e258d] fixup! replace promise resolve
Author: Debadree Chatterjee [email protected]
Date: Sun Nov 26 20:03:18 2023 +0530
2 files changed, 5 insertions(+), 3 deletions(-)
Rebasing (7/14)
Rebasing (8/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! useless import

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

[detached HEAD 93a6ef04b7] fixup! useless import
Author: Debadree Chatterjee [email protected]
Date: Sun Nov 26 20:04:55 2023 +0530
1 file changed, 2 deletions(-)
Rebasing (9/14)
Rebasing (10/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! wrong import remove

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

[detached HEAD 96d8e2ad2b] fixup! wrong import remove
Author: Debadree Chatterjee [email protected]
Date: Wed Nov 29 11:34:40 2023 +0530
1 file changed, 1 insertion(+)
Rebasing (11/14)
Rebasing (12/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
expect a failure

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

[detached HEAD 678fdc7e63] expect a failure
Author: Debadree Chatterjee [email protected]
Date: Tue Dec 5 16:31:19 2023 +0530
1 file changed, 7 insertions(+)
Rebasing (13/14)
Rebasing (14/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
linting

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

[detached HEAD 16dac7d711] linting
Author: Debadree Chatterjee [email protected]
Date: Tue Dec 5 16:42:34 2023 +0530
1 file changed, 22 insertions(+), 22 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/7218936024

nodejs-github-bot · 2023-12-15T07:54:20Z

Landed in 7a2a4d0

debadree25 · 2023-12-15T08:02:55Z

@MattiasBuelens you can now open a PR for the internal stream changes and also rebase the { min } implementation PR too!

Fixes: #49971 PR-URL: #50126 Reviewed-By: Matteo Collina <[email protected]>

richardlau · 2024-03-25T12:01:36Z

This doesn't land cleanly on v20.x-staging and would need a manual backport.

Fixes: nodejs#49971 PR-URL: nodejs#50126 Reviewed-By: Matteo Collina <[email protected]>

Fixes: nodejs#49971 PR-URL: nodejs#50126 Backport-PR-URL: nodejs#52772 Reviewed-By: Matteo Collina <[email protected]>

Fixes: #49971 PR-URL: #50126 Backport-PR-URL: #52772 Reviewed-By: Matteo Collina <[email protected]>

nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Oct 10, 2023

debadree25 mentioned this pull request Oct 15, 2023

Test or specification problem with TransformStream readable.cancel and calling controller.error whatwg/streams#1296

Open

debadree25 force-pushed the ft/transform-cancel-impl branch 2 times, most recently from 9391f66 to 744bd9f Compare October 23, 2023 13:34

MattiasBuelens mentioned this pull request Nov 23, 2023

streams: implement min option for ReadableStreamBYOBReader.read(view) #50888

Merged

1 task

debadree25 added 2 commits November 26, 2023 19:50

stream: implement TransformStream cleanup using "transformer.cancel"

ee89ba1

Fixes: nodejs#49971

test: update wpts

867f64e

debadree25 force-pushed the ft/transform-cancel-impl branch from 744bd9f to 867f64e Compare November 26, 2023 14:21

debadree25 added 2 commits November 26, 2023 20:03

fixup! replace promise resolve

02205e7

fixup! useless import

67b0944

MattiasBuelens reviewed Nov 28, 2023

View reviewed changes

lib/internal/webstreams/transformstream.js Show resolved Hide resolved

fixup! wrong import remove

c627596

expect a failure

23e6183

debadree25 marked this pull request as ready for review December 5, 2023 11:02

debadree25 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 5, 2023

debadree25 requested a review from mcollina December 5, 2023 11:03

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 13, 2023

debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2023

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 15, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2023

nodejs-github-bot merged commit 7a2a4d0 into nodejs:main Dec 15, 2023
72 checks passed

RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023

stream: implement TransformStream cleanup using "transformer.cancel"

71acd36

Fixes: #49971 PR-URL: #50126 Reviewed-By: Matteo Collina <[email protected]>

RafaelGSS mentioned this pull request Dec 15, 2023

v21.5.0 proposal #51166

Merged

MattiasBuelens mentioned this pull request Dec 15, 2023

streams: fixes for webstreams #51168

Merged

richardlau added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Mar 25, 2024

MattiasBuelens pushed a commit to MattiasBuelens/node that referenced this pull request May 1, 2024

stream: implement TransformStream cleanup using "transformer.cancel"

10a0718

Fixes: nodejs#49971 PR-URL: nodejs#50126 Reviewed-By: Matteo Collina <[email protected]>

MattiasBuelens mentioned this pull request May 1, 2024

[v20.x backport] streams: implement TransformStream cleanup using "transformer.cancel" #52772

Closed

marco-ippolito added backport-open-v20.x Indicate that the PR has an open backport and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. labels May 1, 2024

marco-ippolito pushed a commit that referenced this pull request May 16, 2024

stream: implement TransformStream cleanup using "transformer.cancel"

3dd96f1

Fixes: #49971 PR-URL: #50126 Backport-PR-URL: #52772 Reviewed-By: Matteo Collina <[email protected]>

marco-ippolito mentioned this pull request May 23, 2024

v20.14.0 proposal #53120

Merged

targos added backported-to-v20.x PRs backported to the v20.x-staging branch. and removed backport-open-v20.x Indicate that the PR has an open backport labels Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

streams: implement TransformStream cleanup using "transformer.cancel" #50126

streams: implement TransformStream cleanup using "transformer.cancel" #50126

debadree25 commented Oct 10, 2023

debadree25 commented Oct 10, 2023

debadree25 commented Oct 10, 2023 •

edited

Loading

debadree25 commented Oct 23, 2023

saschanaz commented Oct 24, 2023

debadree25 commented Oct 25, 2023

MattiasBuelens commented Nov 23, 2023

debadree25 commented Nov 24, 2023

debadree25 commented Nov 26, 2023

MattiasBuelens commented Dec 4, 2023

debadree25 commented Dec 5, 2023

MattiasBuelens commented Dec 5, 2023

debadree25 commented Dec 5, 2023

debadree25 commented Dec 5, 2023

nodejs-github-bot commented Dec 13, 2023

nodejs-github-bot commented Dec 13, 2023

nodejs-github-bot commented Dec 14, 2023

nodejs-github-bot commented Dec 15, 2023

nodejs-github-bot commented Dec 15, 2023

Fixes: #49971
PR-URL: #50126
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Dec 15, 2023

debadree25 commented Dec 15, 2023

richardlau commented Mar 25, 2024

streams: implement TransformStream cleanup using "transformer.cancel" #50126

streams: implement TransformStream cleanup using "transformer.cancel" #50126

Conversation

debadree25 commented Oct 10, 2023

debadree25 commented Oct 10, 2023

debadree25 commented Oct 10, 2023 • edited Loading

debadree25 commented Oct 23, 2023

saschanaz commented Oct 24, 2023

debadree25 commented Oct 25, 2023

MattiasBuelens commented Nov 23, 2023

debadree25 commented Nov 24, 2023

debadree25 commented Nov 26, 2023

MattiasBuelens commented Dec 4, 2023

debadree25 commented Dec 5, 2023

MattiasBuelens commented Dec 5, 2023

debadree25 commented Dec 5, 2023

debadree25 commented Dec 5, 2023

nodejs-github-bot commented Dec 13, 2023

nodejs-github-bot commented Dec 13, 2023

nodejs-github-bot commented Dec 14, 2023

nodejs-github-bot commented Dec 15, 2023

nodejs-github-bot commented Dec 15, 2023

Fixes: #49971 PR-URL: #50126 Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126 Fixes: #49971 Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126 Fixes: #49971 Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126 Fixes: #49971 Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126 Fixes: #49971 Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126 Fixes: #49971 Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126 Fixes: #49971 Reviewed-By: Matteo Collina [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Dec 15, 2023

debadree25 commented Dec 15, 2023

richardlau commented Mar 25, 2024

debadree25 commented Oct 10, 2023 •

edited

Loading

Fixes: #49971
PR-URL: #50126
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]

PR-URL: #50126
Fixes: #49971
Reviewed-By: Matteo Collina [email protected]