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.

a ping to @nodejs/whatwg-stream

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

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 😅

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

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

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

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

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!

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?

@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:

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

You've implemented this step like this:

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... 😅

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.

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?

@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!

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)

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

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

- 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 matteo.collina@gmail.com
[detached HEAD ca8b3fdc3e] stream: implement TransformStream cleanup using "transformer.cancel"

Author: Debadree Chatterjee debadree333@gmail.com

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 matteo.collina@gmail.com
[detached HEAD e5f8ec4916] test: update wpts

Author: Debadree Chatterjee debadree333@gmail.com

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 matteo.collina@gmail.com
[detached HEAD 1e893e258d] fixup! replace promise resolve

Author: Debadree Chatterjee debadree333@gmail.com

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 matteo.collina@gmail.com
[detached HEAD 93a6ef04b7] fixup! useless import

Author: Debadree Chatterjee debadree333@gmail.com

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 matteo.collina@gmail.com
[detached HEAD 96d8e2ad2b] fixup! wrong import remove

Author: Debadree Chatterjee debadree333@gmail.com

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 matteo.collina@gmail.com
[detached HEAD 678fdc7e63] expect a failure

Author: Debadree Chatterjee debadree333@gmail.com

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 matteo.collina@gmail.com
[detached HEAD 16dac7d711] linting

Author: Debadree Chatterjee debadree333@gmail.com

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.

Landed in 7a2a4d0

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

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