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

Use abort reason in ReadableStreamPipeTo #1182

Merged
merged 19 commits into from
Nov 19, 2021
Merged

Use abort reason in ReadableStreamPipeTo #1182

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
5f36dda
Merge pull request #1 from whatwg/master
nidhijaju Nov 24, 2020
b5dbc0e
Merge pull request #2 from whatwg/master
nidhijaju Dec 1, 2020
f423409
Merge pull request #3 from whatwg/master
nidhijaju Dec 2, 2020
631447b
Merge remote-tracking branch 'upstream/master'
nidhijaju Dec 4, 2020
72e8f57
Merge remote-tracking branch 'upstream/master'
nidhijaju Dec 9, 2020
7c80a45
Merge branch 'whatwg:main' into master
nidhijaju Aug 30, 2021
e1cfefe
Merge branch 'whatwg:main' into master
nidhijaju Aug 31, 2021
8ed62f7
Merge branch 'whatwg:main' into master
nidhijaju Oct 12, 2021
3763ad2
Merge branch 'whatwg:main' into master
nidhijaju Oct 19, 2021
53562da
Merge branch 'whatwg:main' into master
nidhijaju Oct 27, 2021
ec7a41a
use abort reason in PipeTo
nidhijaju Oct 27, 2021
8b2053c
update ref impl
nidhijaju Oct 27, 2021
d2abdfa
pass reason for signal abort
nidhijaju Oct 27, 2021
122b7ff
update ref impl
nidhijaju Oct 27, 2021
0f057fc
fix formatting
nidhijaju Oct 27, 2021
d36baab
remove defaulting logic
nidhijaju Oct 28, 2021
15e4efe
use aborted predicate
nidhijaju Nov 1, 2021
2bf43f6
update wpt submodule
nidhijaju Nov 9, 2021
8bd9267
Update wpt-runner
MattiasBuelens Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2101,7 +2101,9 @@ The following abstract operations operate on {{ReadableStream}} instances at a h
1. Let |promise| be [=a new promise=].
1. If |signal| is not undefined,
1. Let |abortAlgorithm| be the following steps:
1. Let |error| be a new "{{AbortError}}" {{DOMException}}.
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 •
edited
Loading

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

1. Let |actions| be an empty [=ordered set=].
1. If |preventAbort| is false, [=set/append=] the following action to |actions|:
1. If |dest|.[=WritableStream/[[state]]=] is "`writable`", return !
Expand Down
4 changes: 3 additions & 1 deletion reference-implementation/lib/abstract-ops/readable-streams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,9 @@ function ReadableStreamPipeTo(source, dest, preventClose, preventAbort, preventC
let abortAlgorithm;
if (signal !== undefined) {
abortAlgorithm = () => {
const error = new DOMException('Aborted', 'AbortError');
const error = (signal.reason !== undefined) ?
signal.reason :
new DOMException('Aborted', 'AbortError');
const actions = [];
if (preventAbort === false) {
actions.push(() => {
Expand Down