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

Listen for AbortSignal in writeAlgorithm #160

Merged
merged 2 commits into from
Feb 10, 2022
Merged

Conversation

reillyeon
Copy link
Collaborator

When controller.signal is aborted, abort the current write operation so that the stream can close more quickly.

Partially resolves #152.

When controller.signal is aborted, abort the current write operation so
that the stream can close more quickly.

Partially resolves WICG#152.
@reillyeon reillyeon requested a review from domenic February 3, 2022 01:28
@reillyeon
Copy link
Collaborator Author

This specifies the behavior implemented in https://chromium-review.googlesource.com/c/chromium/src/+/3366788 and so resolves an issue which has been confusing me for awhile about one of the tests I wrote for aborting a stream while there was a write pending. This doesn't add any steps to closeAlgorithm but as implemented this would also unblock a close() as well.

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Although the overall logic looks good, writeAlgorithm does not currently accept a controller, and it's bad form to access controller's public signal property. Let me add some better hooks to the streams spec for you to use.

@reillyeon
Copy link
Collaborator Author

While you're adding better hooks I noticed that "set up" doesn't take a startAlgorithm and that would be a better place to add abort steps than every time writeAlgorithm is invoked.

@domenic
Copy link
Collaborator

domenic commented Feb 3, 2022

Oh, the intention was that startAlgorithm wasn't needed since you could just do that yourself as a sibling step of "set up". I think that works, if I expose "stream's signal"?

index.html Outdated Show resolved Hide resolved
@reillyeon
Copy link
Collaborator Author

Oh, the intention was that startAlgorithm wasn't needed since you could just do that yourself as a sibling step of "set up". I think that works, if I expose "stream's signal"?

That's an excellent point.

domenic added a commit to whatwg/streams that referenced this pull request Feb 7, 2022
@reillyeon reillyeon requested a review from domenic February 9, 2022 19:06
Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Beautiful!

@reillyeon reillyeon merged commit 881d4b9 into WICG:main Feb 10, 2022
@reillyeon reillyeon deleted the write_abort branch February 10, 2022 02:12
github-actions bot added a commit that referenced this pull request Feb 10, 2022
SHA: 881d4b9
Reason: push, by @reillyeon

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abort write/close when WritableStreamDefaultController.signal is signaled
2 participants