Editorial: account for removal of AbortSignal's follow #1646
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
annevk
added
the
do not merge yet
|
Thanks for drafting this! I started hacking on this yesterday too, and was leaning towards having "create" take a signal and setting the Request's signal to that, with SW passing a new signal and fetch passing a composite signal. The reason for doing that vs. always creating a composite signal is because SW invokes "signal abort" directly on the signal, acting as its own (abort) controller. Having a composite signal only be aborted by its sources is a useful property (and IMO expected), e.g. it can be GCed when all its sources are gone; having the signal itself be a source complicates that. Aside: It might be nice if "signal abort" was private and always done through a controller, separating the control/signal functionality like we do in userland. Specs that need to trigger an abort (SW, streams, maybe navigation API) could hold onto a controller, although it adds a bit of complexity. (Implementation-wise, I've annotated these signals as "internal" and allow/expect them to act like a controller/signal pair). |
|
@shaseley this should be good for review now. Can I ask you to create the corresponding SW PR? See also the discussion in the Streams PR. I'm not opposed to making "signal abort" private (as the current DOM PR seems to be doing), but we'll need even more downstream PRs to pull that off. |
shaseley
reviewed
May 3, 2023
|
Looks great % linking and possible rename, depending on the outcome of whatwg/dom#1152 (comment).
Yep, happy to update SW. And I flipped "signal abort" back to exported -- I'll work on that as a follow-up. |
|
I was implementing this and noticed the change is observable via event dispatch order (wpt failure). See whatwg/dom#1152 (comment). |
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
May 10, 2023
AbortSignal's "follow" algorithm is replaced with a new algorithm for
creating a dependent abort signal ([1], [2]), which is implemented as
a new AbortSignal constructor. This CL replaces Follow with the new
ctor, behind the AbortSignalAny REF.
There is a slight compat risk with the change (when the feature ships):
1. fetch: abort is propagated to dependent signals after abort event
dispatch for a signal, whereas "follow" propagated abort before (as
abort algorithms), which is observable. We could alternatively use
weak abort algorithms in AbortSignal.any(), but that would be more
complicated and partially limit GC optimizations.
2. cache: The use of "follow" in cache.addAll was not specced and
could lead to abort events firing when they shouldn't have. The new
behavior preserves the specced behavior (aborting outstanding
requests on failure) without dispatching events when abort is
triggered internally.
Note: the virtual test expectations match the current expectations
except for the test modified in fetch/api/abort/general.any.js.
[1] whatwg/dom#1152
[2] whatwg/fetch#1646
Bug: 1323391
Low-Coverage-Reason: the uncovered lines in request.cc are covered by WPT tests
Change-Id: I5c25048fcf9f8db759e2f0181ac338c8b603b451
aarongable
pushed a commit
to chromium/chromium
that referenced
this pull request
May 16, 2023
AbortSignal's "follow" algorithm is replaced with a new algorithm for
creating a dependent abort signal ([1], [2]), which is implemented as
a new AbortSignal constructor. This CL replaces Follow with the new
ctor, behind the AbortSignalAny REF.
There is a slight compat risk with the change (when the feature ships):
1. fetch: abort is propagated to dependent signals after abort event
dispatch for a signal, whereas "follow" propagated abort before (as
abort algorithms), which is observable. We could alternatively use
weak abort algorithms in AbortSignal.any(), but that would be more
complicated and partially limit GC optimizations.
2. cache: The use of "follow" in cache.addAll was not specced and
could lead to abort events firing when they shouldn't have. The new
behavior preserves the specced behavior (aborting outstanding
requests on failure) without dispatching events when abort is
triggered internally.
Note: the virtual test expectations match the current expectations
except for the test modified in fetch/api/abort/general.any.js.
[1] whatwg/dom#1152
[2] whatwg/fetch#1646
Bug: 1323391
Low-Coverage-Reason: the uncovered lines in request.cc are covered by WPT tests
Change-Id: I5c25048fcf9f8db759e2f0181ac338c8b603b451
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4515276
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1144953}
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
May 16, 2023
AbortSignal's "follow" algorithm is replaced with a new algorithm for
creating a dependent abort signal ([1], [2]), which is implemented as
a new AbortSignal constructor. This CL replaces Follow with the new
ctor, behind the AbortSignalAny REF.
There is a slight compat risk with the change (when the feature ships):
1. fetch: abort is propagated to dependent signals after abort event
dispatch for a signal, whereas "follow" propagated abort before (as
abort algorithms), which is observable. We could alternatively use
weak abort algorithms in AbortSignal.any(), but that would be more
complicated and partially limit GC optimizations.
2. cache: The use of "follow" in cache.addAll was not specced and
could lead to abort events firing when they shouldn't have. The new
behavior preserves the specced behavior (aborting outstanding
requests on failure) without dispatching events when abort is
triggered internally.
Note: the virtual test expectations match the current expectations
except for the test modified in fetch/api/abort/general.any.js.
[1] whatwg/dom#1152
[2] whatwg/fetch#1646
Bug: 1323391
Low-Coverage-Reason: the uncovered lines in request.cc are covered by WPT tests
Change-Id: I5c25048fcf9f8db759e2f0181ac338c8b603b451
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4515276
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1144953}
chromium-wpt-export-bot
pushed a commit
to web-platform-tests/wpt
that referenced
this pull request
May 17, 2023
AbortSignal's "follow" algorithm is replaced with a new algorithm for
creating a dependent abort signal ([1], [2]), which is implemented as
a new AbortSignal constructor. This CL replaces Follow with the new
ctor, behind the AbortSignalAny REF.
There is a slight compat risk with the change (when the feature ships):
1. fetch: abort is propagated to dependent signals after abort event
dispatch for a signal, whereas "follow" propagated abort before (as
abort algorithms), which is observable. We could alternatively use
weak abort algorithms in AbortSignal.any(), but that would be more
complicated and partially limit GC optimizations.
2. cache: The use of "follow" in cache.addAll was not specced and
could lead to abort events firing when they shouldn't have. The new
behavior preserves the specced behavior (aborting outstanding
requests on failure) without dispatching events when abort is
triggered internally.
Note: the virtual test expectations match the current expectations
except for the test modified in fetch/api/abort/general.any.js.
[1] whatwg/dom#1152
[2] whatwg/fetch#1646
Bug: 1323391
Low-Coverage-Reason: the uncovered lines in request.cc are covered by WPT tests
Change-Id: I5c25048fcf9f8db759e2f0181ac338c8b603b451
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4515276
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1144953}
annevk
pushed a commit
to whatwg/dom
that referenced
this pull request
May 17, 2023
- This implements an optimization that puts all children on non-dependent signals (i.e., those associated with a controller). This allows "intermediate" nodes (e.g., B in A follows B follows C) to be garbage collected if they are being kept alive to propagate aborts. - This removes the follow algorithm, so callsites will need to be updated. - The "create a composite abort signal" algorithm takes an interface so that TaskSignal.any() can easily hook into it, but create a TaskSignal. - Some algorithms that invoke "follow" create an AbortSignal in a particular realm. This enables doing that, but borrows some language from elsewhere in the spec w.r.t. doing the default thing. Neither of the other two static members specify a realm. Follow-up PRs: - whatwg/fetch#1646 - w3c/ServiceWorker#1678 - whatwg/streams#1277 This also sets the stage to make AbortSignal's "signal abort" fully internal. #1194 tracks the remainder. Tests: web-platform-tests/wpt#37434 and web-platform-tests/wpt#39785. Fixes #920.
annevk
removed
the
do not merge yet
annevk
pushed a commit
to w3c/ServiceWorker
that referenced
this pull request
May 17, 2023
See whatwg/fetch#1646 for context.
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
May 22, 2023
…nd cache-storage, a=testonly
Automatic update from web-platform-tests
Don't use AbortSignal::Follow in fetch and cache-storage
AbortSignal's "follow" algorithm is replaced with a new algorithm for
creating a dependent abort signal ([1], [2]), which is implemented as
a new AbortSignal constructor. This CL replaces Follow with the new
ctor, behind the AbortSignalAny REF.
There is a slight compat risk with the change (when the feature ships):
1. fetch: abort is propagated to dependent signals after abort event
dispatch for a signal, whereas "follow" propagated abort before (as
abort algorithms), which is observable. We could alternatively use
weak abort algorithms in AbortSignal.any(), but that would be more
complicated and partially limit GC optimizations.
2. cache: The use of "follow" in cache.addAll was not specced and
could lead to abort events firing when they shouldn't have. The new
behavior preserves the specced behavior (aborting outstanding
requests on failure) without dispatching events when abort is
triggered internally.
Note: the virtual test expectations match the current expectations
except for the test modified in fetch/api/abort/general.any.js.
[1] whatwg/dom#1152
[2] whatwg/fetch#1646
Bug: 1323391
Low-Coverage-Reason: the uncovered lines in request.cc are covered by WPT tests
Change-Id: I5c25048fcf9f8db759e2f0181ac338c8b603b451
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4515276
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1144953}
--
wpt-commits: 29c729d6b60e74383e8e8222f06bb7c3680b2022
wpt-pr: 39951
ErichDonGubler
pushed a commit
to ErichDonGubler/firefox
that referenced
this pull request
May 23, 2023
…nd cache-storage, a=testonly
Automatic update from web-platform-tests
Don't use AbortSignal::Follow in fetch and cache-storage
AbortSignal's "follow" algorithm is replaced with a new algorithm for
creating a dependent abort signal ([1], [2]), which is implemented as
a new AbortSignal constructor. This CL replaces Follow with the new
ctor, behind the AbortSignalAny REF.
There is a slight compat risk with the change (when the feature ships):
1. fetch: abort is propagated to dependent signals after abort event
dispatch for a signal, whereas "follow" propagated abort before (as
abort algorithms), which is observable. We could alternatively use
weak abort algorithms in AbortSignal.any(), but that would be more
complicated and partially limit GC optimizations.
2. cache: The use of "follow" in cache.addAll was not specced and
could lead to abort events firing when they shouldn't have. The new
behavior preserves the specced behavior (aborting outstanding
requests on failure) without dispatching events when abort is
triggered internally.
Note: the virtual test expectations match the current expectations
except for the test modified in fetch/api/abort/general.any.js.
[1] whatwg/dom#1152
[2] whatwg/fetch#1646
Bug: 1323391
Low-Coverage-Reason: the uncovered lines in request.cc are covered by WPT tests
Change-Id: I5c25048fcf9f8db759e2f0181ac338c8b603b451
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4515276
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1144953}
--
wpt-commits: 29c729d6b60e74383e8e8222f06bb7c3680b2022
wpt-pr: 39951
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For whatwg/dom#1152.
This is incomplete. https://fetch.spec.whatwg.org/#dom-request-clone needs to be updated as well. While we could probably leave https://fetch.spec.whatwg.org/#request-create (also called by Service Workers) alone, that doesn't seem entirely right as we'd be creating a redundant
AbortSignalobject.Would the best solution here be that "create" takes a signal(s?) argument and then uses that to create a new composite signal for itself?
It doesn't have to do anything for service workers it seems, but if service workers were to pass an empty list it'd do the same thing it does today.
cc @jakearchibald @shaseley
Preview | Diff