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.

Sign up for GitHub

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

Jump to bottom

add TrasformStream finally callback #1231

Closed
wants to merge 3 commits into from
Closed

add TrasformStream finally callback #1231

wants to merge 3 commits into from

Conversation

crowlKats
Copy link
Contributor

@crowlKats crowlKats commented May 20, 2022 •
edited by pr-preview bot
Loading

Closes #1212

The only thing left is what to do in case of the callback throwing/rejecting (and potentially somehow awaiting the callback? unsure if thats necessary).

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

@crowlKats crowlKats mentioned this pull request May 20, 2022
Closed
@domenic
Copy link
Member

domenic commented May 23, 2022

I thought the plan was to add finally() handlers to all streams?

Adding a cancel() handler doesn't make a lot of sense to me since you can't cancel a transform stream...

@crowlKats
Copy link
Contributor Author

crowlKats commented May 23, 2022 •
edited
Loading

sure, seems i somehow missed that in the original issue, will change this PR to that

@crowlKats crowlKats changed the title add TrasformStream cancel add TrasformStream finally callback May 23, 2022
@crowlKats
Copy link
Contributor Author

@domenic adjusted the behaviour & name

@crowlKats
Copy link
Contributor Author

crowlKats commented May 23, 2022 •
edited
Loading

one issue i currently havent handled yet is what to do in case the finally callback is a promise. The call in the flushPromise fulfilled case should be easier, however the other call in transformStreamErrorWritableAndUnblockWrite, unsure how to proceed.

MattiasBuelens
MattiasBuelens reviewed May 23, 2022
View reviewed changes
index.bs
1. Perform ! [$TransformStreamDefaultControllerClearAlgorithms$](|stream|.[=TransformStream/[[controller]]=]).
1. Perform !
[$WritableStreamDefaultControllerErrorIfNeeded$](|stream|.[=TransformStream/[[writable]]=].[=WritableStream/[[controller]]=], |e|).
1. If |stream|.[=TransformStream/[[backpressure]]=] is true, perform ! [$TransformStreamSetBackpressure$](|stream|,
false).
1. Perform ! |stream|.[=TransformStream/[[controller]]=].[=TransformStreamDefaultController/[[finallyAlgorithm]]=]().
Copy link
Collaborator

Choose a reason for hiding this comment

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

This algorithm will already have been cleared by TransformStreamDefaultControllerClearAlgorithms in step 1, so this won't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, it currently isnt, as i am unsure how to proceed without moving the clear algs

index.bs
@@ -5965,6 +5981,7 @@ side=] of [=transform streams=].
1. Perform ! [$TransformStreamDefaultControllerClearAlgorithms$](|controller|).
1. Return the result of [=reacting=] to |flushPromise|:
1. If |flushPromise| was fulfilled, then:
1. Perform ! |stream|.[=TransformStream/[[controller]]=].[=TransformStreamDefaultController/[[finallyAlgorithm]]=]().
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once again, this algorithm has already been cleared in step 4.

@andreubotella
Copy link
Member

andreubotella commented Jan 12, 2023 •
edited
Loading

one issue i currently havent handled yet is what to do in case the finally callback is a promise. The call in the flushPromise fulfilled case should be easier, however the other call in transformStreamErrorWritableAndUnblockWrite, unsure how to proceed.

Handling this is complicated, since TransformStreamErrorWritableAndUnblockWrite is making it so writing to the writable stream will throw, and this cannot be delayed until the finally promise resolves. So the finally callback can't work in a similar way to Promise.prototype.finally.

Should we ignore the returned promise and let it cause an unhandled promise rejection?

@lucacasonato
Copy link
Member

Opened a new PR at #1283 that specs this with an alternative approach and fixes the "use after free" issues. That PR also has tests and an the reference implementation has been updated.

@crowlKats
Copy link
Contributor Author

Closing as Luca implemented this.

@crowlKats crowlKats closed this Dec 21, 2023
@crowlKats crowlKats deleted the transformstream_cancel branch December 21, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattiasBuelens MattiasBuelens MattiasBuelens left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handling stream errors in a TransformStream transformer
5 participants
@crowlKats @domenic @andreubotella @lucacasonato @MattiasBuelens