Use abort reason in ReadableStreamPipeTo by nidhijaju · Pull Request #1182 · whatwg/streams

nidhijaju · 2021-10-27T05:39:25Z

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

At least two implementers are interested (and none opposed):
- Google
- Mozilla
Tests are written and can be reviewed and commented upon at:
- Streams: Add/update WPTs with AbortSignal's abort reason web-platform-tests/wpt#31400
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1267135
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1739564
- Safari: https://bugs.webkit.org/show_bug.cgi?id=232747

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

Preview | Diff

Merge from original repo

Merge from Original Repo

Update forked repo

annevk · 2021-10-27T07:40:50Z

+   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}}.


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

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.

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

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?

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?

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.

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?

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

MattiasBuelens · 2021-10-27T07:41:59Z

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

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

…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 · 2021-11-15T21:19:53Z

@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 · 2021-11-15T23:55:49Z

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

annevk · 2021-11-16T07:50:47Z

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.

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

nidhijaju · 2021-11-19T07:14:30Z

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 · 2021-11-19T15:57:42Z

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

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}

…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

nidhijaju and others added 12 commits November 24, 2020 16:57

Merge pull request #1 from whatwg/master

5f36dda

Merge from original repo

Merge pull request #2 from whatwg/master

b5dbc0e

Merge from Original Repo

Merge pull request #3 from whatwg/master

f423409

Update forked repo

Merge remote-tracking branch 'upstream/master'

631447b

Merge remote-tracking branch 'upstream/master'

72e8f57

Merge branch 'whatwg:main' into master

7c80a45

Merge branch 'whatwg:main' into master

e1cfefe

Merge branch 'whatwg:main' into master

8ed62f7

Merge branch 'whatwg:main' into master

3763ad2

Merge branch 'whatwg:main' into master

53562da

use abort reason in PipeTo

ec7a41a

update ref impl

8b2053c

annevk reviewed Oct 27, 2021

View reviewed changes

pass reason for signal abort

d2abdfa

MattiasBuelens reviewed Oct 27, 2021

View reviewed changes

Comment thread index.bs

nidhijaju added 2 commits October 27, 2021 09:13

update ref impl

122b7ff

fix formatting

0f057fc

nidhijaju mentioned this pull request Oct 27, 2021

Streams: Add/update WPTs with AbortSignal's abort reason web-platform-tests/wpt#31400

Merged

nidhijaju added 3 commits October 28, 2021 07:40

remove defaulting logic

d36baab

use aborted predicate

15e4efe

update wpt submodule

2bf43f6

nidhijaju added a commit to web-platform-tests/wpt that referenced this pull request Nov 9, 2021

Streams: Update WPTs with AbortSignal.reason (#31400)

b724cac

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

annevk mentioned this pull request Nov 10, 2021

AbortSignal's abort reason downstream integration whatwg/dom#1030

Closed

13 tasks

Update wpt-runner

8bd9267

MattiasBuelens approved these changes Nov 12, 2021

View reviewed changes

Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this pull request Nov 18, 2021

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

f6509b5

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

domenic merged commit 95973b4 into whatwg:main Nov 19, 2021

nidhijaju deleted the pipeto-abortreason branch November 22, 2021 05:06

MattiasBuelens mentioned this pull request Nov 24, 2021

Roll WPT for abort reason #1189

Merged

Conversation

nidhijaju commented Oct 27, 2021 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

yutakahirano Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

annevk Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

nidhijaju Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

annevk Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

nidhijaju Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

annevk Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

nidhijaju Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattiasBuelens commented Oct 27, 2021

Uh oh!

Uh oh!

domenic commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-ivar commented Nov 15, 2021

Uh oh!

annevk commented Nov 16, 2021

Uh oh!

nidhijaju commented Nov 19, 2021

Uh oh!

domenic commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

nidhijaju commented Oct 27, 2021 •

edited by pr-preview bot

Loading

nidhijaju Oct 28, 2021 •

edited

Loading

domenic commented Nov 15, 2021 •

edited

Loading