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

finally() operator timing semantics #151

Closed
domfarolino opened this issue Jun 26, 2024 · 3 comments
Closed

finally() operator timing semantics #151

domfarolino opened this issue Jun 26, 2024 · 3 comments

Comments

@domfarolino
Copy link
Collaborator

TL;DR: On unsubscription (i.e., abort()), what should the timing of the finally() operator's callback be with respect to the source Observable's teardowns?

When writing the spec for, and implementing, the currently-unspecified finally() operator, I was reviewing the proposed web platform tests and had a comment about this bit: https://github.com/web-platform-tests/wpt/pull/44482/files#diff-b1a6cf94fa2c551227215a7556fbc3114006fca82549ba48a27f3dafe2564f01R186. This made me think more about the timing of the finally() operator's callback with respect to the source Observable's teardowns.

Background: unsubscription teardown semantics

When you subscribe (with an AbortSignal) to an Observable returned by an operator, two things happen:

  1. A dependent signal is created from the signal that was passed into subscribe().
  2. The intermediate Observable is subscribed to with that dependent signal (example), and its native subscribe callback runs.
  3. The operator subscribes to its upstream Observable with the dependent signal (example). This dependent signal goes through (1) above and gets turned into a new dependent signal before subscription, of course.

So in the following code:

const source = new Observable(subscriber => ...);

const ac = new AbortController();
source.take(1).subscribe({}, {signal: ac.signal});

There are three AbortSignals involved:

  1. The parent one: ac.signal.
  2. A dependent of (1), for take()s Subscriber's Subscriber#signal. (It is purely internal and not web accessible).
  3. A dependent of (2), exposed in subscriber above.

If you call ac.abort(), then the abort signals are aborted in the order they're listed above. That's because DOM defines that a parent signal gets fully aborted before any of its dependents: https://dom.spec.whatwg.org/#abortsignal-signal-abort. Practically speaking, this means if you added an abort event listener to ac.signal and subscriber.signal, the ac.signal one would fire first, and the subscriber.signal one would fire last.

Integrating finally()

Now consider the requirement that the finally() callback needs to be called when a consumer unsubscribes from an Observable:

const ac = new AbortController();

const source = new Observable(() => {});
source
  .finally(() => console.log('finally'))
  .subscribe({}, {ac.signal});

ac.abort(); // logs `finally` to the console

The simplest way to make this happen is just to add finally()'s callback to its Subscriber's teardown list. This essentially means finally() is syntactic sugar around that operator's Subscriber's addTeardown() method. But this would break the ordering suggested in the test: https://github.com/web-platform-tests/wpt/pull/44482/files#diff-b1a6cf94fa2c551227215a7556fbc3114006fca82549ba48a27f3dafe2564f01R186. Specifically, in the following example:

const ac = new AbortController();
const source = new Observable(subscriber => {
  subscriber.addTeardown(() => console.log('teardown'));
});

source
  .finally(() => console.log('finally()'))
  .subscribe({}, {signal: ac.signal});

// Logs `finally()`, and then `teardown`. But the test expects
// `teardown` and then `finally()`.
ac.abort();

If the finally callback were part of its intermediate Subscriber's teardown list, then it would necessarily run before any of the source Observable's teardowns, since the intermediate signal (associated with finally()) aborts before its dependent signal (the one associated with the source Observable's Subscriber). Is this OK? It doesn't match RxJS's behavior, where finalize() callbacks execute after the teardown callback runs.

If we reallllly wanted to, we could probably match RxJS's behavior by building some bespoke infrastructure on Subscriber, or rejiggering around the dependencies of AbortSignals. But I'm really not sure what that would look like, or if it is even desirable at all. It would break the general precedent with AbortSignal.

I lean towards the code example / output immediately above, but I'd like to get opinions from others, to know if this is a real problem.

@benlesh
Copy link
Collaborator

benlesh commented Jul 1, 2024

Teardowns should be executed during finalization of observation. They are roughly what you'd put in a "finally" block if we had syntax for observable in JavaScript. Since the result of the finally() method is a new observable that is "downstream" from the source, I would expect the source's upstream finalization to be complete before the downstream could trigger it's finally.

In fact, once an upstream source is complete or errored, or when it's notified of abort, the downstream consumer should not be able to act until finalization is complete.

Abort notifications should be sent upstream immediately.

During abort, I'd expect the following order of operations within a given subscription:

  1. Abort any upstream subscriptions
  2. Execute teardowns in reverse order

Similarly in a situation where the subscription was errored or completed by the producer, I would expect:

  1. Abort any upstream subscriptions
  2. Execute teardowns in reverse order
  3. Notify any downstream consumers of completion (or error)

So as prior art here, we can look at generators or async generators:

With generators set up as so:

function* a() {
  try { yield 'a'; } finally { console.log('finally a'); }
}

function* b() {
  try { yield* a(); } finally { console.log('finally b'); }
}

function* c() {
  try { yield* b(); } finally { console.log('finally c'); }
}

If you create an instance of the generator and prime it:

const g = c();
g.next(); // primed { value: 'c', done: false }

And then you either next (resulting in a completion notification of { done: true }), or you return() which is signaling that you're done with the generator (similar to an abort in observable), you'll see the order in which the finally blocks are logged.

c.next(); // { done: true }

// logs
// "finally a"
// "finally b"
// "finally c"

or

c.return();

// logs
// "finally a"
// "finally b"
// "finally c"

@benlesh
Copy link
Collaborator

benlesh commented Jul 1, 2024

Other prior art: Promise.prototype.finally

Promise.resolve(1)
  .finally(() => console.log('one'))
  .finally(() => console.log('two'))
  
// Logs
// "one"
// "two"

Yeah... the observable implementation needs to abort the upstream subscription before it fires the finalizer.

Similarly, it should abort upstream subscriptions as soon as it knows it can in the case of "complete" or "error" being called in the subscriber.

@domfarolino
Copy link
Collaborator Author

When I originally filed this issue, the interaction of Observables and their internal AbortSignals was such that I had the following concern:

The simplest way to make this happen is just to add finally()'s callback to its Subscriber's teardown list. This essentially means finally() is syntactic sugar around that operator's Subscriber's addTeardown() method. But this would break the ordering suggested in the test: https://github.com/web-platform-tests/wpt/pull/44482/files#diff-b1a6cf94fa2c551227215a7556fbc3114006fca82549ba48a27f3dafe2564f01R186.

Since then, #154 has landed which both simplified and corrected the interaction of Observables with all of the internal AbortSignals. This obsoletes the concerns I had earlier above, where it was natural to implement finally() using the teardown list, but impossible to do so. The issue described the problem, and ways to possibly get around this.

None of this is necessary now that #154 has landed, so this issue can be closed and we can proceed with the very simple implementation of finally(), the last operator for now.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 21, 2025
See WICG/observable#153. This CL implements the
`finally()` operator. It registers a callback which runs on both
producer-initiated and consumer-initiated unsubscription. Its
implementation is relatively straightforward, just utilizing the
relevant subscriber's teardown list.

This CL used to be really complicated, as was the spec PR. This has all
been simplified since the factors that made it complicated (and broken)
have been landed. See:
 - WICG/observable#151 (comment)
 - WICG/observable#154
 - https://crrev.com/c/5676226

Now that the simplifying factors have been landed, this CL can proceed
with a simple implementation, which passes all of the tests.

[email protected]

Bug: 40282760
Change-Id: Ifff220ec094abd2187f35020eae85c1bca502f89
aarongable pushed a commit to chromium/chromium that referenced this issue Feb 21, 2025
See WICG/observable#153. This CL implements the
`finally()` operator. It registers a callback which runs on both
producer-initiated and consumer-initiated unsubscription. Its
implementation is relatively straightforward, just utilizing the
relevant subscriber's teardown list.

This CL used to be really complicated, as was the spec PR. This has all
been simplified since the factors that made it complicated (and broken)
have been landed. See:
 - WICG/observable#151 (comment)
 - WICG/observable#154
 - https://crrev.com/c/5676226

Now that the simplifying factors have been landed, this CL can proceed
with a simple implementation, which passes all of the tests.

[email protected]

Bug: 40282760
Change-Id: Ifff220ec094abd2187f35020eae85c1bca502f89
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5654720
Reviewed-by: Di Zhang <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1423349}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 21, 2025
See WICG/observable#153. This CL implements the
`finally()` operator. It registers a callback which runs on both
producer-initiated and consumer-initiated unsubscription. Its
implementation is relatively straightforward, just utilizing the
relevant subscriber's teardown list.

This CL used to be really complicated, as was the spec PR. This has all
been simplified since the factors that made it complicated (and broken)
have been landed. See:
 - WICG/observable#151 (comment)
 - WICG/observable#154
 - https://crrev.com/c/5676226

Now that the simplifying factors have been landed, this CL can proceed
with a simple implementation, which passes all of the tests.

[email protected]

Bug: 40282760
Change-Id: Ifff220ec094abd2187f35020eae85c1bca502f89
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5654720
Reviewed-by: Di Zhang <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1423349}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 21, 2025
See WICG/observable#153. This CL implements the
`finally()` operator. It registers a callback which runs on both
producer-initiated and consumer-initiated unsubscription. Its
implementation is relatively straightforward, just utilizing the
relevant subscriber's teardown list.

This CL used to be really complicated, as was the spec PR. This has all
been simplified since the factors that made it complicated (and broken)
have been landed. See:
 - WICG/observable#151 (comment)
 - WICG/observable#154
 - https://crrev.com/c/5676226

Now that the simplifying factors have been landed, this CL can proceed
with a simple implementation, which passes all of the tests.

[email protected]

Bug: 40282760
Change-Id: Ifff220ec094abd2187f35020eae85c1bca502f89
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5654720
Reviewed-by: Di Zhang <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1423349}
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

No branches or pull requests

2 participants