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

Don't use dependent abort signals inside Subscriber #154

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

domfarolino
Copy link
Collaborator

@domfarolino domfarolino commented Jul 9, 2024

Before this pull request, a Subscriber's internal signal — the one exposed by Subscriber#signal() — was a dependent signal based on two input signals:

  1. The SubscribeOptions#signal, if non-null; and
  2. The Subscriber's internal AbortController's (named complete or error controller before this CL) AbortSignal.

Then, whenever the internal dependent signal is aborted, we run the abort algorithm which (a) "closes" the subscription (sets subscriber.active to false), and (b) executes all teardowns.

But (1) the use of dependent AbortSignals (i.e., making each Subscriber's signal dependent on its downstream Subscriber's signal), and (2) tying a Subscriber's teardown execution to its internal signal's abort algorithms, introduced inconsistent teardown ordering for Observables chained together. Specifically, the order of teardown execution across Observables in a chain depended on whether the Observable chain experienced producer-initiated vs. consumer-initiated
unsubscription. Example:

const a = new Observable(subscriber => {
  window.sourceSubscriber = subscriber;
  subscriber.addTeardown(() => console.log('A teardown'));
});

const b = new Observable(subscriber => {
  subscriber.addTeardown(() => console.log('B teardown'));
  a.subscribe({complete: () => subscriber.complete()}, {signal: subscriber.signal});
})

const ac = new AbortController();
const signal = ac.signal;
b.subscribe({}, {signal});

// Prints `B teardown`, then `A teardown`
// ac.abort();

// Prints `A teardown`, then `B teardown`
// window.sourceSubscriber.complete();

Upstream teardowns (a) should run before downstream teardowns (b). The reason the consumer-initiated case is backwards is due to the ordering or abortion among dependent AbortSignals. Specifically, once we abort the developer passed-in AbortSignal, the first thing that runs are the abort algorithms, which runs local teardowns first. Then, DOM proceeds abort any dependent (aka upstream) signals, which then runs those upstream teardowns, before aborting even more upstream signals, and so on.


To fix this, this PR introduces two small but significant changes to the underlying AbortSignal infrastructure in Observables.

The first thing this PR does is stop making a Subscriber's internal signal a dependent signal based on the developer-supplied one (if one exists). The abort timing of dependent signals led to downstream Subscribers being aborted (teardowns and all) before upstream Subscribers (in the consumer-initiated unsubscription case, that is).

Instead, now the Subscriber maintains an internal AbortController (similar to today's "complete or error controller"), and its internal signal is that controller's signal. Then, like before, if a signal is provided in SubscribeOptions, we add an abort algorithm to it to close the subscription.

The second change this PR introduces is a change to the "close" algorithm itself. In addition to just setting active to false, it now aborts the Subscriber's internal controller. This kicks off the aborting of any upstream Subscribers right away, since those upstream "close" algorithms run (as an abort algorithm) in response to the downstream Subscriber's signal abort. This means "close" climbs all the way to the top-most upstream Subscriber/AbortSignal right away, before running any teardowns. Only after a subscription has been "closed", and after all upstream signals have been aborted, does the current Subscriber run its teardowns.

This changes the timing of things. In the consumer-initiated unsubscription case, instead of downstream signals running their teardowns and then "closing" their dependent (upstream) signals after, the first thing a downstream signal does in response to abort is "close" any upstream subscribers first.

Now imagine you have the following Observable chain:

a
  .operator() <-- (b) Observable
  .operator() <-- (c) Observable
  .subscribe({}, {signal});

Upon aborting signal you'd get the following flow:

1. Downstream signal aborted
  1. (C) "Close" algorithm runs
    1. (C) Aborts its internal controller
      1. (B) "Close" algorithm runs
        1. (B) Aborts its internal controller
          1. (A) "Close" algorithm runs
            1. (A) Aborts its internal controller
            2. (A) Teardowns run
        2. (B) Teardowns run
    2. (C) Teardowns run

Before this PR, the timing flow was like this:

1. Downstream signal aborted
  1. (C) Signal aborted
    1. (C) Close algorithm
      1. (C) Teardowns run
    2. (B) Signal aborted
      1. (B) Close algorithm
        1. (B) Teardowns run
      2. (A) Signal aborted
        1. (A) Close algorithm
          2. (A) Teardowns run

Tests are being written and landed in https://chromium-review.googlesource.com/c/chromium/src/+/5676226.


Preview | Diff

@domfarolino domfarolino requested a review from benlesh July 9, 2024 19:53
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 10, 2024
This CL introduces subtle timing differences in Subscriber abortion and
teardown execution, across Subscribers in a chain of Observables. These
timing differences are the result of no longer using the DOM Standard's
dependent AbortSignal concept for Observables that are chained together.

For a full description of this change, see
WICG/observable#154.

Bug: 40282760
Change-Id: I4feb6f9ad67e2dd7d7a4d5ec51fdecebc4e6ae18
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 10, 2024
This CL introduces subtle timing differences in Subscriber abortion and
teardown execution, across Subscribers in a chain of Observables. These
timing differences are the result of no longer using the DOM Standard's
dependent AbortSignal concept for Observables that are chained together.

For a full description of this change, see
WICG/observable#154.

Bug: 40282760
Change-Id: I4feb6f9ad67e2dd7d7a4d5ec51fdecebc4e6ae18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5676226
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1325506}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 10, 2024
This CL introduces subtle timing differences in Subscriber abortion and
teardown execution, across Subscribers in a chain of Observables. These
timing differences are the result of no longer using the DOM Standard's
dependent AbortSignal concept for Observables that are chained together.

For a full description of this change, see
WICG/observable#154.

Bug: 40282760
Change-Id: I4feb6f9ad67e2dd7d7a4d5ec51fdecebc4e6ae18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5676226
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1325506}
@domfarolino
Copy link
Collaborator Author

Ben has approved the tests, and the Chromium implementation updates for this have landed, so I'll merge the spec now.

@domfarolino domfarolino merged commit f954c91 into master Jul 13, 2024
2 checks passed
@domfarolino domfarolino deleted the abortsignal-timing branch July 13, 2024 15:55
github-actions bot added a commit that referenced this pull request Jul 13, 2024
SHA: f954c91
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 15, 2024
…r Observable Subscribers, a=testonly

Automatic update from web-platform-tests
DOM: Don't use dependent AbortSignals for Observable Subscribers

This CL introduces subtle timing differences in Subscriber abortion and
teardown execution, across Subscribers in a chain of Observables. These
timing differences are the result of no longer using the DOM Standard's
dependent AbortSignal concept for Observables that are chained together.

For a full description of this change, see
WICG/observable#154.

Bug: 40282760
Change-Id: I4feb6f9ad67e2dd7d7a4d5ec51fdecebc4e6ae18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5676226
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1325506}

--

wpt-commits: 1c736ae64328a24e9c3a0c384929a5227b1c9274
wpt-pr: 47074
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jul 16, 2024
…r Observable Subscribers, a=testonly

Automatic update from web-platform-tests
DOM: Don't use dependent AbortSignals for Observable Subscribers

This CL introduces subtle timing differences in Subscriber abortion and
teardown execution, across Subscribers in a chain of Observables. These
timing differences are the result of no longer using the DOM Standard's
dependent AbortSignal concept for Observables that are chained together.

For a full description of this change, see
WICG/observable#154.

Bug: 40282760
Change-Id: I4feb6f9ad67e2dd7d7a4d5ec51fdecebc4e6ae18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5676226
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1325506}

--

wpt-commits: 1c736ae64328a24e9c3a0c384929a5227b1c9274
wpt-pr: 47074
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jul 18, 2024
…r Observable Subscribers, a=testonly

Automatic update from web-platform-tests
DOM: Don't use dependent AbortSignals for Observable Subscribers

This CL introduces subtle timing differences in Subscriber abortion and
teardown execution, across Subscribers in a chain of Observables. These
timing differences are the result of no longer using the DOM Standard's
dependent AbortSignal concept for Observables that are chained together.

For a full description of this change, see
WICG/observable#154.

Bug: 40282760
Change-Id: I4feb6f9ad67e2dd7d7a4d5ec51fdecebc4e6ae18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5676226
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1325506}

--

wpt-commits: 1c736ae64328a24e9c3a0c384929a5227b1c9274
wpt-pr: 47074
sadym-chromium pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 18, 2024
This CL introduces subtle timing differences in Subscriber abortion and
teardown execution, across Subscribers in a chain of Observables. These
timing differences are the result of no longer using the DOM Standard's
dependent AbortSignal concept for Observables that are chained together.

For a full description of this change, see
WICG/observable#154.

Bug: 40282760
Change-Id: I4feb6f9ad67e2dd7d7a4d5ec51fdecebc4e6ae18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5676226
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1325506}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant