WICG / observable

Observable API proposal
https://wicg.github.io/observable/
Other
546 stars 12 forks source link

Introduce `Subscriber#active` boolean #80

Closed domfarolino closed 8 months ago

domfarolino commented 8 months ago

This closes https://github.com/WICG/observable/issues/76. This PR introduces the subscriber.active boolean attribute that can be used from within an Observable to determine whether a subscription is still active. I am still personally a tiny but unsure whether this is really necessary, so I'd like to use this space to describe the semantics and how it differs from other APIs, so we can come to a conclusion on this, since @benlesh feels much more strongly about this API.

On the surface it seems that subscriber.active === !subscriber.signal.aborted[^1], however this is not quite true. The lifecycle of an Observable is the following:

  1. Active subscription
  2. subscriber.complete()/error() is called from script
  3. The subscription is no longer "active", meaning the bodies of those methods cannot re-entrantly invoke themselves or any method on the Observer dictionary for that matter.
  4. The actual corresponding Observer#complete()/error() body is run
  5. The Subscriber#signal is aborted (via an internal AbortController)
  6. subscriber.signal.aborted is true
  7. All teardown callbacks (added by an API that probably looks like subscriber.addTeardown()) will run, as part of the abort algorithms associated with subscriber.signal

The subscriber.active attribute intends to expose precisely when a subscription no longer becomes active (just before the Observer#complete()/error() body is run), which is before the subscriber.signal is finally aborted. The only observable consequence I can think of for this is inside the complete()/error() methods, where subscriber.active is already false but subscriber.signal.aborted is not yet true (since teardowns haven't run yet).

I think that subscriber.active is a more sensible and ergonomic way to express the concept of an "inactive subscription", however I only hesitate to add it because I don't love the idea of having two different ways of getting this information with slightly different semantics. I'm curious if anyone would rely on subscriber.active in a way that !subscriber.signal.aborted couldn't be used for (thus causing broken code).


Here's an I can think of where subscriber.signal.aborted is insufficient for stopping an infinite loop:

let stopPushingValues = true;

const observable = new Observable(subscriber => {
  // The subscriber's `complete()` handler fires an event that
  // forces this observable to synchronously push a firehose of
  // values to the subscriber.
  document.addEventListener('stuff', e => {
    while (!subscriber.signal.aborted) {
      if (stopPushingValues === false) {
        subscriber.next(1);
      } else {
        // In this case, the subscriber has signaled something
        // to us that it wants us to complete the subscription.
        subscriber.complete();
      }
    }
  });
});

const ac = new AbortController();
observable.subscribe({
  next: v => {
    stopPushingValues = true;
  },
  complete: () => {
    document.dispatchEvent(new Event('stuff'));
  }
});

document.dispatchEvent(new Event('stuff'));

// The sequence of events is this:
//   1. Random code fires a 'stuff' event at document
//   2. This causes the Observable to start pushing values synchronously
//      to the `next()` handler. Eventually the `next()` handler decides that
//      doesn't want anymore values, so it signals to the Observable that it
//      wants the Observable to call `complete()`[^1] to end the subscription.
//   3. Observable detects `stopPushingValues` (i.e., that the subscriber wants
//      the Observable to stop), and calls `Observer#complete()`.
//   4. Observer#complete() triggers another 'stuff' event, which re-enters the
//      event-pushing code in the subscriber callback. This starts another
//      synchronous loop that tries to push values *again* to the subscriber. But
//      this time no values can be pushed, because once `complete()` is entered ever,
//      it becomes impossible to invoke `Observer#next()/complete()/error()` methods.
//      But the `while` loop never stops!
//
//      So here's how you could get yourself into this buggy situation. After you call
//      `complete()`, it's true that `subscriber.signal.aborted` will be true. As a
//      developer, you might rely on this, by using `while (!subscriber.signal.aborted)`
//      as a condition in the while loop, so that the while loop breaks *after* you call
//      `complete()`. However there's a very subtle timing thing here that you might not
//      realize. `subscriber.signal.aborted` is only true *after* the call to `complete()`
//      is done. So if `complete()` itself re-enters the while loop, it will never terminate
//      because `subscriber.signal.aborted` is never `true` *from within* the call to
//      `complete()`.

// [^1]: Note that this is the contrived part of this example. Normally when a
// subscriber wants to be "done", it just `abort()`s the signal that was passed into
// `subscribe()`, to unsubscribe. But that doesn't cause the issue in this example,
// so we had to craft a more contrived way to trigger it.

How important is it to protect against this kind of (highly contrived!) scenario? Or are there way less contrived scenarios where subscriber.signal.aborted is a footgun, and where subscriber.active would be critical.

[^1]: Remember of course that subscriber.signal is aborted absolutely whenever a subscription is ended, not just when the subscriber unsubscribes / aborts the signal that it passed into subscribe().

benlesh commented 8 months ago

Yeah. This API in RxJS is called closed, and used fairly often, but active is a MUCH better name for it, as most checks are like if (subscriber.active) { /* do things */ }, and because it better describes what you're checking. You're checking to see if the subscriber is still "active" before you continue doing work.

domfarolino commented 8 months ago

Replaced with https://github.com/WICG/observable/pull/82 since this was merged by mistake.