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

Use abort reason in ReadableStreamPipeTo #1182

Merged
merged 19 commits into from
Nov 19, 2021

Conversation

nidhijaju
Copy link
Contributor

@nidhijaju nidhijaju commented Oct 27, 2021

Following on from whatwg/dom#1027, this PR uses the AbortSignal's abort reason in the ReadableStreamPipeTo function. It also updates WritableStreamAbort to use the reason to signal abort.

/cc @annevk, @yutakahirano, @domenic

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


Preview | Diff

index.bs Outdated
Comment on lines 2104 to 2106
1. If |signal|'s [=AbortSignal/abort reason=] is not undefined, let |error| be |signal|'s
[=AbortSignal/abort reason=].
1. Otherwise, let |error| be a new "{{AbortError}}" {{DOMException}}.
Copy link
Member

Choose a reason for hiding this comment

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

I think we might want an abstraction for this if every specification that adopts signals needs this. I put a suggestion in whatwg/fetch#1343 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need these logic? signal.reason will be an "AbortError" DOMException when controller.abort(undefined) is called so it should be rare to see undefined here. I don't think we should care about the case.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point, when would we see undefined here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, fair point. I'll remove the if condition here and just set error to the signal's abort reason directly. Do you think it would be a good idea to have an assert here to check that the reason is not undefined just to be sure?

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering about that. Perhaps it should be in DOM's "signal abort" as otherwise we'd have to put the assert in each spec for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be better to have the assert here instead of DOM's "signal abort" because error is not only used to signal abort in this function.

Copy link
Member

Choose a reason for hiding this comment

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

This is run as the result of signal abort being run, no? Or I suppose in the case the signal was already aborted. Either way, perhaps whatwg/dom#1027 (comment) is a better clarification?

Copy link
Contributor Author

@nidhijaju nidhijaju Oct 28, 2021

Choose a reason for hiding this comment

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

Ah yes, I think you are right. I've made the changes to the DOM spec based on your suggestion :)

@MattiasBuelens
Copy link
Collaborator

I think we should also pass reason when we "signal abort on stream.[[controller]].[[signal]]" in step 2 of WritableStreamAbort?

nidhijaju added a commit to web-platform-tests/wpt that referenced this pull request Nov 9, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 13, 2021
…l's abort reason, a=testonly

Automatic update from web-platform-tests
Streams: Update WPTs with AbortSignal.reason (#31400)

See whatwg/streams#1182 for the accompanying spec change.
--

wpt-commits: b724cac4269298cfa57064bd5751e7e6e7593727
wpt-pr: 31400
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 15, 2021
…l's abort reason, a=testonly

Automatic update from web-platform-tests
Streams: Update WPTs with AbortSignal.reason (#31400)

See whatwg/streams#1182 for the accompanying spec change.
--

wpt-commits: b724cac4269298cfa57064bd5751e7e6e7593727
wpt-pr: 31400
@domenic
Copy link
Member

domenic commented Nov 15, 2021

@annevk, @jan-ivar, want to confirm Mozilla interest in this change? It's the replacement for the abortReason part of #1132 based on TAG feedback in #1165

@jan-ivar
Copy link
Contributor

I'm interested in this as part of WebTransport. @annevk anyone else we should run this by?

@annevk
Copy link
Member

annevk commented Nov 16, 2021

I think that's good enough, but let's copy @mgaudet just in case. We also want the AbortSignal change and as far as I know this is the logical follow-up to that.

Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this pull request Nov 18, 2021
@nidhijaju
Copy link
Contributor Author

Hi, I'm wondering if this PR is ready to submit now that we have the 3 tasks in the checklist ticked off as well? Please let me know if you want me to address anything else :)

@domenic
Copy link
Member

domenic commented Nov 19, 2021

We wanted to confirm with the Gecko folks; @annevk just did so offline. So, merging now!

@domenic domenic merged commit 95973b4 into whatwg:main Nov 19, 2021
@nidhijaju nidhijaju deleted the pipeto-abortreason branch November 22, 2021 05:06
aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 7, 2021
This CL follows whatwg/streams#1182, which
introduces custom abort reasons using the AbortSignal's abort reason
in the ReadableStreamPipeTo function[1]. This change also updates
WritableStreamAbort[2] to use the reason to signal abort.

[1] https://streams.spec.whatwg.org/#readable-stream-pipe-to
[2] https://streams.spec.whatwg.org/#writable-stream-abort

Intent to Ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/T6B5czAke1I

Bug: 1267135, 1268515
Change-Id: Ia9bdc0d0d46505f3e764d7a396a4fe19da843b27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3295823
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Cr-Commit-Position: refs/heads/main@{#948860}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This CL follows whatwg/streams#1182, which
introduces custom abort reasons using the AbortSignal's abort reason
in the ReadableStreamPipeTo function[1]. This change also updates
WritableStreamAbort[2] to use the reason to signal abort.

[1] https://streams.spec.whatwg.org/#readable-stream-pipe-to
[2] https://streams.spec.whatwg.org/#writable-stream-abort

Intent to Ship: https://groups.google.com/a/chromium.org/g/blink-dev/c/T6B5czAke1I

Bug: 1267135, 1268515
Change-Id: Ia9bdc0d0d46505f3e764d7a396a4fe19da843b27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3295823
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Cr-Commit-Position: refs/heads/main@{#948860}
NOKEYCHECK=True
GitOrigin-RevId: 4f6cd86be20bfd972bf35c87335b4337ebfeb927
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants