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

Add AbortSignal.any() #1152

Merged
merged 10 commits into from
May 17, 2023
Merged

Add AbortSignal.any() #1152

merged 10 commits into from
May 17, 2023

Conversation

shaseley
Copy link
Contributor

@shaseley shaseley commented Feb 7, 2023

Hi DOM folks,

I've got a working implementation of AbortSignal.any() with tentative WPT tests, so I wanted to send out a draft spec for thoughts/consideration. It's probably not ready as-is because it would break other specs (see below), but this is how I thought the end state might look.

PR Description

This adds IDL and algorithms for AbortSignal.any().

Implementation details:

  • This implements an optimization that puts all children on non-composite signals (i.e. those associated with a controller or timeout). This allows "intermediate" nodes (e.g. B in A follows B follows C) to be garbage collected if they are only being kept alive to propagate aborts.

  • This removes the follow algorithm, so callsites will need to be updated.

  • This makes "signal abort" private. This is done so we can make assumptions about when a signal can no longer abort. The few places that directly signal abort will be converted to use an AbortController (this also exposes an algorithm to "request abort" on a controller for this).

  • 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 AbortSignal factory APIs specify a realm.

Dependent / Follow-up work

As-is, this PR changes how other specs would interact with abort signals and controllers. The related work is to:

  1. Don't invoke "signal abort" outside of the DOM spec, use "request abort" on an abort controller instead: Stop exporting AbortSignal's signal abort #1194.
  2. Don't invoke "follow" anywhere, use "create a composite abort signal" instead.
    1. Editorial: account for removal of AbortSignal's follow fetch#1646
      1. Editorial: pass signal to Request/create w3c/ServiceWorker#1678
    2. Editorial: account for removal of AbortSignal's follow streams#1277
  3. (probably) Make sure algorithms are being removed when they will no longer have an effect, e.g. when the underlying operation is completed or failed: also Stop exporting AbortSignal's signal abort #1194.

Implementation Notes

There's an implementation of this in Chromium behind a flag in Canary (M112). The basic implementation was straightforward, but there was a bit of work to get the memory management implemented (what's described in the Garbage Collection section in the draft PR). Composite signals can't be GCed if they have abort algorithms (or abort event listeners) and there's a source signal that can still signal abort, but we weren't removing abort algorithms once they were "done", which prevented GC. I fixed this in Chromium, but we should probably also update specs too.

PR Questions

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

Additional Info


Preview | Diff

@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest addition/proposal New features or enhancements topic: aborting AbortController and AbortSignal labels Feb 7, 2023
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I've done a quick editorial take. This would probably benefit from review by @smaug----, @jakearchibald, and @domenic.

dom.bs Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated
Comment on lines 1977 to 1979
{{AbortSignal}} objects <var>signals</var> or a single {{AbortSignal}} <var>signal</var>, an
optional <var>signalInterface</var>, which must be either {{AbortSignal}} or an interface that
inherits from it, and an optional <var>realm</var>, run these steps:
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 confusing. Let's always require a list. Folks can use the list syntax from Infra.

And do we want an interface or some kind of creation algorithm? (Also, if you want it to default to AbortSignal you should specify that here using the language from Infra, but please only do that if there are many callers. Otherwise it's not really worth it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair - changed to requiring a list.

Regarding interface vs. creation algorithm: The main reason it has this form is to ensure the signal is newly created (i.e. not associated with a controller), since that's an assumption in the algorithm. The language ~= that of "create an event". There's another alternative where the exported algorithm just takes an optional realm and delegates to a private algorithm that takes a signal (to also be used by TaskSignal), but that's probably overkill?

I removed the default value since this likely won't have many callers callers (users of follow (i.e. fetch) and the TaskSignal specialization).

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@shaseley
Copy link
Contributor Author

shaseley commented Feb 8, 2023

Thanks for taking a first pass, @annevk; comments addressed.

@jakearchibald
Copy link
Collaborator

@shaseley can you provide a bit of detail on why the spec needs to make the garbage collection more explicit than other specs? Is it observable in some way?

My understanding (please correct me if I'm wrong @annevk) is that spec algorithms should be written as simply as possible, and UAs are invited to implement them in a more complex way that makes them work faster / more efficient as long as the observable behaviour is the same.

@annevk
Copy link
Member

annevk commented Feb 9, 2023

Well, although we don't always do a good job at this, we do need to describe relationships between objects in a way that allows GC to eventually succeed. And I don't think we should accept "agent lifetime" as a way out, although sometimes that is acceptable.

I hadn't looked at the GC section here yet, I was hoping @smaug---- could take a look at that. But now that I have:

  • Strong references are implied so don't need to be stated.
  • That something is a weak reference should be mentioned as part of the definition, not in a separate section.
  • The GC section should mainly state when something can be GC'd or ideally what prevents GC when GC would otherwise be possible. https://xhr.spec.whatwg.org/#garbage-collection is an example of that, although I think there's been a suggestion that user agents will never collect when the connection is still open either, so maybe that should be a must not instead.

(I hope that at some point JS/IDL formalize some of these aspects better so we can ground terminology like "weak reference" in terms of a larger web platform memory model.)

@jakearchibald
Copy link
Collaborator

Fair enough. I guess it ensures we don't later break it in a way that prevents this particular kind of GC.

@annevk
Copy link
Member

annevk commented Feb 9, 2023

Yeah and it also helps if we all think a bit about memory usage and leaks. (E.g., why you need to explicitly clone a response to read it twice, as I'm sure you're well aware.)

@shaseley
Copy link
Contributor Author

shaseley commented Feb 9, 2023

Yeah, I wasn't sure if this section actually belonged or not, but there is precedent in the DOM spec here. I included this because GC/memory management of composite signals was a concern in the related issue, so I wanted to include it to hopefully help guide other implementors.

FYI: there's one design decision related to the linking algorithm that helps support memory management and is observable: AbortSignal.any() always links the resulting signal to a non-composite signal (timeout or associated with a controller). This basically flattens the signal graph to a depth of 1, meaning intermediate composite signals don't need to be kept alive just to propagate abort. This affects the order events fire, which is covered in tests.

Well, although we don't always do a good job at this, we do need to describe relationships between objects in a way that allows GC to eventually succeed. And I don't think we should accept "agent lifetime" as a way out, although sometimes that is acceptable.

I hadn't looked at the GC section here yet, I was hoping @smaug---- could take a look at that. But now that I have:

  • Strong references are implied so don't need to be stated.
  • That something is a weak reference should be mentioned as part of the definition, not in a separate section.

FWIW I was using https://dom.spec.whatwg.org/#garbage-collection as an example, which should probably also get cleaned up then?:

  1. "Nodes have a strong reference to registered observers in their registered observer list." → strong is implied
  2. "Registered observers in a node’s registered observer list have a weak reference to the node." → this should probably move to the section where registered observers are defined? Or maybe this is about a mutation observer's node list? I can't tell exactly what that reference is.
  • The GC section should mainly state when something can be GC'd or ideally what prevents GC when GC would otherwise be possible. https://xhr.spec.whatwg.org/#garbage-collection is an example of that, although I think there's been a suggestion that user agents will never collect when the connection is still open either, so maybe that should be a must not instead.

Thanks, I'll have a look at that and clean this up.

(I hope that at some point JS/IDL formalize some of these aspects better so we can ground terminology like "weak reference" in terms of a larger web platform memory model.)

@shaseley
Copy link
Contributor Author

Ok, I updated the GC section based on @annevk's suggestions. The idea is that source signals can store dependent signals weakly as long as dependent signals are kept alive as outlined in the GC section. Doing so prevents long-lived source signals (e.g. application-level signal) from holding onto composite signals indefinitely.

dom.bs Outdated
these steps:
<p>To <dfn export>create a composite abort signal</dfn> from a list of {{AbortSignal}} objects
<var>signals</var>, using <var>signalInterface</var>, which must be either {{AbortSignal}} or an
interface that inherits from it, and an optional <var>realm</var>, run these steps:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it rather useless to say "or an interface that inherits from it"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is here is to allow subclasses to invoke this as an internal constructor, since they may need to specialize the behavior (if this is premature, I can remove and monkeypatch for TaskSignal). I'm borrowing the language from event creation, trying to keep this locally consistent.

Can you recommend alternative wording? To me, "which must be an {{AbortSignal}}" implies an AbortSignal object and "which must be {{AbortSignal}}" implies subclasses are excluded since it names it specifically. I didn't find anything in infra about inheritance wording, which is one reason I looked for something locally in the DOM spec.

Copy link

Choose a reason for hiding this comment

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

“object implementing X” is defined by Web IDL and includes objects implementing Y if Y extends X.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But here the parameter is the interface itself and not a platform object, so I don't think that can be used (the definition requires an object)? The interface parameter is used below to create an object that implements it. (That the parameter is an interface and not object is why I don't think the phrasing is useless -- but it would be if it was an object.)

Copy link

Choose a reason for hiding this comment

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

Ah, got it, I read that too quickly. That makes sense to me.

dom.bs Outdated
<li><p>If <var>followingSignal</var> is [=AbortSignal/aborted=], then return.
<li><p>Let <var>resultSignal</var> be a <a for=/>new</a> object implementing
<var>signalInterface</var> using <var>realm</var> if given, otherwise using the default behavior
defined in Web IDL.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we say something like "defined in Foo", I'd expect that to be then link to the definition. Especially since this is doing something unusual with realms, the default behavior should be clear too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I copied the language from event creation, which does something similar, but I didn't copy the note w/ related Web IDL issue. I added the same note; does this address the concern?

FYI the reason this takes an optional realm is because new Request() in the fetch spec (step 29) creates a new signal in a specific realm, but in this spec we just create new signals without specifying a realm.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, in general realm-of-creation is a giant mess across all web platform standards. We've had a few attempts at trying to clean that up, but none successful thus far. There's a Web IDL issue open on defining some kind of implicit realm that we could use in most cases, but it hasn't been worked on.

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@shaseley shaseley left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing!

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
Copy link
Member

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

Normatively LGTM, a few suggestions for making things a bit cleaner.

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Show resolved Hide resolved
dom.bs Outdated

<p>An {{AbortSignal}} object has associated <dfn for=AbortSignal>dependent signals</dfn>, which is a
<a for=/>set</a> of weak references to {{AbortSignal}} objects that are dependent on it for their
[=AbortSignal/aborted=] state. Unless specified otherwise, its value is the empty set.
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 these should be "weak sets" instead of "sets of weak references". The difference is not behaviorally significant, but I think you'd need more verbiage to rigorously manage a set of weak references. E.g., instead of "Append sourceSignal to ..." you'd need "Append a weak reference to sourceSignal to...". And you'd need to dereference and null-check and so on.

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 good catch, thanks, "weak set" semantics are indeed what I want (I still had Blink/Oilpan in mind where references/collection weakness happens automatically based on type).

Similarly, Mutation Observer's node list should probably be a "weak list". It does add weak references to nodes, but the values aren't null-checked on iteration (I missed this in review, thinking a list of weak references would behave like a weak list).

Some clarification in Infra could be useful here, e.g. define weak sets & lists, weak references, their relationship, etc. I'll file an issue.

This adds IDL and algorithms for AbortSignal.any(). Details:
 - This implements an optimization that puts all children on
   non-composite 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

 - This makes "signal abort" private. This is done so we can make
   assumptions about when a signal can no longer abort. The few
   places that directly signal abort will be converted to use an
   AbortController (this also exposes an algorithm to "request abort"
   on a controller for this).

 - 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 AbortSignal factory APIs specify a realm.
Copy link
Member

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

Looks great!

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@shaseley shaseley left a comment

Choose a reason for hiding this comment

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

Thanks Domenic!

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
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 <[email protected]>
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Scott Haseley <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1144953}

<p class=note>The [=AbortSignal/abort algorithms=] enable APIs with complex
requirements to react in a reasonable way to {{AbortController/abort()}}. For example, a given API's
[=AbortSignal/abort reason=] might need to be propagated to a cross-thread environment, such as a
service worker.

<p>An {{AbortSignal}} object has a <dfn for="AbortSignal">dependent</dfn> (a boolean), which is
Copy link

Choose a reason for hiding this comment

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

Do we really need this flag? Why can't we rely on source signals being non-empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference has to do with empty signals and GC eligibility.

const signal = AbortSignal.any([]);

const signal2 = AbortSignal.any([signal]);

signal is an "empty dependent signal", and such signals can never abort. When creating signal2, the "create a dependent signal" algorithm will not add signal as a source signal because its sources are empty; if we remove the "dependent" flag, it will. It's a minor difference that won't affect correctness, but it could affect GC eligibility.

[=AbortSignal/source signals=]:

<ol>
<li><p>Assert: <var>sourceSignal</var> is not [=AbortSignal/aborted=] and not
Copy link

Choose a reason for hiding this comment

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

It is not clear to me what ensure that the second part of the assertion holds. How do we know for sure that AbortSignal.any() cannot be called with an AbortSignal parameter that has it's dependent flag set?

Maybe we should recursively go through source signals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be called with dependent signals. When it encounters one in the outer for-loop, it iterates over the dependent signal's source signals and links those. And by always linking to a dependent's source signals, dependent signals are never added as sources. So this is saying "a dependent signal's source signal cannot be a dependent signal."

The idea is something like this:

const root = someController.signal;

// Non-dependent signal passed, simple case (append it directly):
//   root.__dependent_signals = [signal1];
//   signal1.__source_signals = [root];
const signal1 = AbortSignal.any([someController.signal]);

// Dependent signal passed, iterate over its source signals and link those:
//   root.__dependent_signals = [signal1, signal2];
//   signal2.__source_signals = [root]; (note: root is linked, not signal1).
const signal2 = AbortSignal.any([signal1]);

// Same as above:
// root.__dependent_signals = [signal1, signal2, signal3];
// signal3.__source_signals = [root];
const signal3 = AbortSignal.any([signal2]);

This is by design to "flatten" the signal dependency graph by linking directly to signals that can abort directly (timeouts or controller signals), which helps optimize GC since intermediates aren't kept alive to propagate abort. (Note also that new sources are fixed at construction time).

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 <[email protected]>
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Scott Haseley <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
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 <[email protected]>
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Scott Haseley <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1144953}
@annevk annevk merged commit 6c02df2 into whatwg:main May 17, 2023
@annevk
Copy link
Member

annevk commented May 17, 2023

@shaseley thank you so much for driving this to completion! I think it's very nice that we now have this composition method.

Reason for the delay in merging: to double check everyone was still comfortable with the approach here given the uncertainty at the end, I reached out to Olli (for Gecko) and Chris (for WebKit) and they both indicated this still seemed good.

If folks notice anything that was overlooked here please raise a new issue. Although we often do end up seeing comments in them, closed issues are in principle not monitored and definitely not a good place to write something down you think should be tracked.

annevk added a commit to whatwg/fetch that referenced this pull request May 17, 2023
annevk added a commit to whatwg/fetch that referenced this pull request May 17, 2023
webkit-commit-queue pushed a commit to cdumez/WebKit that referenced this pull request May 17, 2023
https://bugs.webkit.org/show_bug.cgi?id=256176
rdar://109056627

Reviewed by Brent Fulgham.

Implement AbortSignal.any() as per:
- whatwg/dom#1152

The feature is behind a runtime flag, disabled by default.

* LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-any.any.worker-expected.txt:
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/bindings/js/JSAbortSignalCustom.cpp:
(WebCore::JSAbortSignalOwner::isReachableFromOpaqueRoots):
* Source/WebCore/dom/AbortSignal.cpp:
(WebCore::AbortSignal::any):
(WebCore::AbortSignal::addSourceSignal):
(WebCore::AbortSignal::addDependentSignal):
(WebCore::AbortSignal::signalAbort):
* Source/WebCore/dom/AbortSignal.h:
* Source/WebCore/dom/AbortSignal.idl:

Canonical link: https://commits.webkit.org/264163@main
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request May 18, 2023
PR-URL: #47821
Fixes: #47811
Refs: whatwg/dom#1152
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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 <[email protected]>
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Scott Haseley <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1144953}

--

wpt-commits: 29c729d6b60e74383e8e8222f06bb7c3680b2022
wpt-pr: 39951
fasenderos pushed a commit to fasenderos/node that referenced this pull request May 22, 2023
PR-URL: nodejs#47821
Fixes: nodejs#47811
Refs: whatwg/dom#1152
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ErichDonGubler pushed a commit to erichdongubler-mozilla/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 <[email protected]>
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Scott Haseley <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1144953}

--

wpt-commits: 29c729d6b60e74383e8e8222f06bb7c3680b2022
wpt-pr: 39951
@shaseley shaseley deleted the abort-signal-any branch May 25, 2023 17:07
targos pushed a commit to nodejs/node that referenced this pull request May 30, 2023
PR-URL: #47821
Fixes: #47811
Refs: whatwg/dom#1152
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
atlowChemi added a commit to atlowChemi/node that referenced this pull request Jul 17, 2023
PR-URL: nodejs#47821
Fixes: nodejs#47811
Refs: whatwg/dom#1152
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit to nodejs/node that referenced this pull request Jul 17, 2023
PR-URL: #47821
Backport-PR-URL: #48800
Fixes: #47811
Refs: whatwg/dom#1152
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: aborting AbortController and AbortSignal
Development

Successfully merging this pull request may close these issues.

9 participants