Should "unsubscribe" be an optional method on Observer?

[leebyron]

For the same reasons that start is a very useful additional method to Observer, I believe that unsubscribe would also be very useful.

The compelling use case is where subscribe() is called in an unrelated part of a codebase from where the resulting Subscription object may end up, and side-effectful resources are in use, which should be cleaned up on completion.

In other words, we could make a guarantee that exactly one, and only one of error, complete, or unsubscribe on the Observer would be called before "cleanup".

Consider this case with a logging API

function subscribeWithSideEffects(observable) {
  return observable.subscribe({
    start() {
      LoggingAPI.begin()
    },
    error() {
      LoggingAPI.reportError()
    },
    complete() {
      LoggingAPI.reportSuccessfulCompletion()
    },
    // proposed addition:
    unsubscribe() {
      LoggingAPI.reportClientDidCancel()
    }
  });
}

If the returned Subscription is unsubscribed, there's no concise way to observe that at the point where subscribe was called.

A working albeit verbose solution might be:

function subscribeWithSideEffects(observable) {
  const outer = new Observable(sink => {
    const inner = observable.subscribe({
      start() {
        LoggingAPI.begin()
      },
      error() {
        LoggingAPI.reportError()
      },
      complete() {
        LoggingAPI.reportSuccessfulCompletion()
      }
    });
    return () => {
      LoggingAPI.clientDidCancel() // EDIT: comment below explains why this is still insufficient
      inner.unsubscribe()
    }
  );
  return outer.subscribe()
}

That does work, however it doesn't compose well. It also may not lend itself well to library-specific expansions of prototypal methods which rely on the Observer type.

Consider the do() method, which accepts Observer for the purpose of side-effects. This method could be the right fit for the side-effectful example of logging above, but would only be useful if it supported start (which would be expected for a library implementing ESObservable) and unsubscribe.

If this is compelling, I would suggest two changes to implement this:

First, adding unsubscribe to Observer, mirroring start:

interface Observer {
    // Receives the subscription object when `unsubscribe` is called
    unsubscribe(subscription : Subscription);
}

As well as extending %SubscriptionPrototype%.unsubscribe( ) to include:

  1. Let _subscription_ be the *this* value.
  1. If Type(_subscription_) is not Object, throw a *TypeError* exception.
  1. If _subscription_ does not have all of the internal slots of a Subscription instance, throw a *TypeError* exception.
  1. If SubscriptionClosed(_subscription_) is *true*, return *undefined*.
+ 1. Let _observer_ be the value of _subscription_'s [[Observer]] internal slot.
  1. Set _subscription_'s [[Observer]] internal slot to *undefined*.
+ 1. Let _unsubscribeMethodResult_ be GetMethod(_observer_)
+ 1. If _unsubscribeMethodResult_.[[Type]] is ~normal~, then
+   1. Let _unsubscribe_ be _unsubscribeMethodResult_.[[Value]].
+   1. If _unsubscribe_ is not *undefined*, then
+     1. Let _result_ be Call(_unsubscribe_, _observer_, « _subscription_ »).
+     1. If _result_ is an abrupt completion, perform HostReportErrors(« _result_.[[Value]] »).
  1. Return CleanupSubscription(_subscription_).

[benjamingr]

The thing is that unlike .start the consumer is the one that knows when unsubscribing happens (since they hold the return value of .subscribe and can unsubscribe).

Your above code can (probably) be written as:

function subscribeWithSideEffects(observable) {
  let f = observable.subscribe({
    start: LoggingAPI.begin,
    error: LoggingAPI.reportError,
    complete: LoggingAPI.reportSuccessfulCompletion
  });
  return () => {
    f(); // unsubscribe
    LoggingAPI.reportClientDidCancel(); // react
  };
}

[josephsavona]

@benjamingr That's a reasonable workaround in the case that you aren't returning an Observable. But it's less elegant if you want to perform side effects using do:

function doWorkWithSideEffects(...): Observable {
  return doWorkImpl(...) // Observable
    .do({
      start: LoggingAPI.begin,
      error: LoggingAPI.reportError,
      complete: LoggingAPI.reportSuccessfulCompletion,
      unsubscribe: LoggingAPI.reportClientDidCancel, // cleanup composes
    });
}

Is much clearer than a workaround such as:

function doWorkWithSideEffects(...): Observable {
  return new Observable(sink => {
    const sub = doWorkImpl(...)
      .do({
        start: LoggingAPI.begin,
        error: LoggingAPI.reportError,
        complete: LoggingAPI.reportSuccessfulCompletion,
      })
      .subscribe(sink);
    return () => {
      sub.unsubscribe();
      LoggingAPI.reportClientDidCancel();
    };
  });
}

This seems very similar to the use-cases for start: without it developers are often forced to explicitly create new Observables, because complete/error/next are not sufficient to fully compose the behavior.

[benjamingr]

This seems very similar to the use-cases for start

I'm not sure I agree, with .start you need to be able to know when it started which is information you don't actually have in some cases. With unsubscription - you control the unsubscription - it's easy to wrap it in a small helper method which would make the API nicer again.

[josephsavona]

Observable.prototype.onUnsubscribe(fn) {
  return new Observable(sink => {
    const sub = this.subscribe(sink);
    return () => {
     sub.unsubscribe();
     fn(); // <-- this actually doesn't work, as it's called on complete/error cleanup in addition to unsubscribe
    };
  });
}

At first glance I thought so too and wrote up the above onSubscribe implementation. But this actually demonstrates exactly the problem you mentioned with start - the callback returned in an Observable source is called on cleanup, not on unsubscribe. I don't see a good way to do some work only when unsubscribe is called and not on normal cleanup. Perhaps you could keep local state about whether error/complete have already fired and only call the onSubscribe logic if cleanup occurs before error/complete. I'm not sure if that would miss any cases though.

[appsforartists]

Is there a distinction between teardowns due to { complete(), error(), unsubscribe() } anywhere else in the spec? Is there good reason for there to be?

My first instinct is that from the POV of an Observable, teardown is teardown regardless of what triggered it.

[leebyron]

I came back to comment that my pre-proposed code wasn't actually correct, and @josephsavona pointed that out in his comment above.

Instead you would need to write:

function subscribeWithSideEffects(observable) {
  let didErrorOrComplete = false;
  let f = observable.subscribe({
    start: LoggingAPI.begin,
    error: () => {
      didErrorOrComplete = true;
      LoggingAPI.reportError(),
    complete: () => {
      didErrorOrComplete = true;
      LoggingAPI.reportSuccessfulCompletion();
    }
  });
  return { unsubscribe: () => {
    f.unsubscribe();
    if (!didErrorOrComplete) {
      LoggingAPI.reportClientDidCancel(); // react
    }
  }};
}

So this is in fact distinct from teardown - this is specifically about having an opportunity in the context of an Observer, to observe the unsubscription event.

[leebyron]

@appsforartists

Is there a distinction between teardowns due to { complete(), error(), unsubscribe() } anywhere else in the spec? Is there good reason for there to be?

My first instinct is that from the POV of an Observable, teardown is teardown regardless of what triggered it.

I completely agree. I do not suggest that the addition of an unsubscribe entry to the Observer interface should alter the teardown behavior in any way.

[appsforartists]

I think I get it now. I had to re-read the thread.

You want unsubscribe to be distinct from error and complete because there are already methods for error and complete. teardowns = unsubscribes + completes + errors, and Observer can presently only detect two of them; you want a method for each.

Personally, I don't have a horse in this race. The only Observer method we care about is next.

[zenparsing]

@leebyron I can't think of a reason to oppose, other than just a general desire to keep the API from getting any bigger. My primary motivation for adding start was to help deal with cases where next was called before the subscription was returned to the caller of subscribe, but it's cool that it also has other "lifecycle" uses.

Thinking this through...

Currently, the return value of the subscriber function may be either a cleanup function or a "subscription" (the structural type { unsubscribe() : void }). We allow the "subscription" type so that composition is ergonomic:

new Observable(sink => {
  return somethingElse.subscribe({
    // ...
  });
});

If we added unsubscribe to Observer, then some observers would also be subscriptions (structurally). Would that have any negative impacts?

[leebyron]

Excellent questions, @zenparsing. I don't think it should have any negative impact, because the semantics all still make sense. We just add the ability to observe these existing semantics.

In your example, when the outer Observable cleans up, the correct behavior is to unsubscribe from the inner observable. If the outer Observable is cleaning up because it completed successfully, that implies that the inner also completed successfully, and the unsubscribe would be simply ignored (all present behavior).

We can investigate this with a test:

function testSink() {
  let sink;
  const inner = new Observable(_sink => {
    sink = _sink;
    return () => console.log('cleanup');
  });

  const outer = new Observable(sink => {
    return inner.subscribe({
      start: () => console.log('inner start'),
      next: value => (sink.next(value), console.log('inner next')),
      error: error => (sink.error(error), console.log('inner error')),
      complete: () => (sink.complete(), console.log('inner complete')),
      unsubscribe: () => console.log('inner unsubscribe'),
    });
  });

  const subscription = outer.subscribe({
    start: () => console.log('outer start'),
    next: () => console.log('outer next'),
    error: () => console.log('outer error'),
    complete: () => console.log('outer complete'),
    unsubscribe: () => console.log('outer unsubscribe'),
  });

  return [sink, subscription];
}

// Test inner completes
const [sink1] = testSink();
sink1.next(1);
sink1.complete();
// outer start
// inner start
// outer next
// inner next
// outer complete
// inner complete
// cleanup


// Test outer unsubscribes
const [sink2, unsubscribe2] = testSink();
sink2.next(1);
unsubscribe2.unsubscribe();
// outer start
// inner start
// outer next
// inner next
// outer unsubscribe
// inner unsubscribe
// cleanup

[benlesh]

It's a real edge-case, in my experience, to need to be notified of unsubscription, but I can see how it could come up.

Below is a representation of how you can accomplish this in any flavor I could imagine people asking for. The shouldUnsubscribe one is a bit iffy, because it would only be effective in certain scenarios with multicast observables, but it could come up so I'm tossing it in there.

const watched = new Observable(observer => {
  const sub = source.subscribe(observer);
  return () => {
    willUnsubscribe();
    if (shouldUnsubscribe) sub.unsubscribe();
    didUnsubscribe();
  }
});

function willUnsubscribe() {
}

function shouldUnsubscribe() {
  return true;
}

function didUnsubscribe() {
}

Likewise, similar patterns can be used to manage those things for Subscription start.

All-in-all, these sorts of things would be trivial for a library like RxJS to provide.

[jhusain]

The main issue I have with this proposal is that I don't think the proposed semantics will match user's expectations. The unsubscribe method of the Subscription returned from the subscriber function currently gets called when error and complete are called. Under the circumstances, it would be reasonable for users to expect that the Observer's unsubscribe method would be called when error or complete are notified.

If the main use case is resource finalization, why not simply use Observable's finally method? Would the code's intent not be clearer?

[leebyron]

One use case is simple resource finalization, and I agree that finally() is a good prototype method for capturing that case.

A more interesting case to me is when the reason for an Observable completing is relevant to program behavior. Here's a concrete example in how Relay handles updates. In this case, we apply an optimistic change to the screen, and when we observe a new network value, we replace the optimistic change with the real network change. Knowing the difference between normal completion, error completion, and unsubscribe completion is critical to applying the right behavior.

Another example is logging/telemetry, where knowing the difference between an internal completion (the Observable completed successfully or failed with error) vs an external completion (the Observable was unsubscribed from elsewhere) is an important distinction.

As for your direct concerns, I'm not sure I follow them, it would be helpful to see some small examples.

The unsubscribe method of the Subscription returned from the subscriber function currently gets called when error and complete are called. Under the circumstances, it would be reasonable for users to expect that the Observer's unsubscribe method would be called when error or complete are notified.

I don't follow this. The unsubscribe method of the Subscription returned from the subscriber function is never called by that Observable, it is called externally. When the Observable's sink's error or complete is called, that Observable's cleanup function is called but the unsubscribe method is something exposed for a consumer to call. I would be surprised if any user would expect an Observer's unsubscribe method to be called in response to errors and completes when Observer already has error and complete methods.

One important edge case to this is Observable composition, where we have two different Observables, and an inner Observable is subscribed to, providing it's unsubscribe method as the cleanup function to an outer Observable. Again I think the distinction between internal vs external completion is helpful in understanding why when the internal Observer's complete is called, that neither the internal or external Observer's unsubscribe methods would be called, vs if the external Observerable is completed, and in cleaning up calls the inner Subscription's unsubscribe method, causing the inner Observer's unsubscribe method to be called since it was rightfully unsubscribed before it was able to internally compelte/error itself.

I wrote up some code illustrating that case a couple comments above

[leebyron]

@benlesh I think your code example again illustrates a real confusion about Observable cleanup functions. They're called after unsubscribe yes, but also after internal error or normal completion. Either better names for your functions would be willCleanup() instead of willUnsubscribe(), or you would need similar checks for whether sink.error or sink.complete had been called first, to differentiate cleanups in response to external unsubscription

[matthewwithanm]

For another concrete example of where the distinction between internal and external completion is useful, you can check out Nuclide's process spawning utilities. If the process completes, cool, so will the observable. But if you unsubscribe we actually want to kill the process.

Currently, we resort to using do() to track whether it was an internal completion and then act on that in finally().

[jhusain]

Dropping in quickly to clarify questions about my concerns. Will circle back and give a more detailed reply tomorrow. To reiterate, my concern is that the behavior of the Observer's unsubscribe method will not match user's expectations unless we invoke the Observer's unsubscribe whenever the Observable's cleanup logic executes.

I don't follow this. The unsubscribe method of the Subscription returned from the subscriber function is never called by that Observable, it is called externally. When the Observable's sink's error or complete is called, that Observable's cleanup function is called but the unsubscribe method is something exposed for a consumer to call. I would be surprised if any user would expect an Observer's unsubscribe method to be called in response to errors and completes when Observer already has error and complete methods.

See Section 3.1.1 and 3.1.2 of the spec (http://tc39.github.io/proposal-observable/#observable-prototype-properties). A Subscriber function can return a Subscription instead of a cleanup function. If the Subscriber function returns a Subscription, the Observer's cleanup function calls the Subscription's `unsubscribe` method when complete or error are called. const observable = new Observable(observer => { return { // called on observer.complete() unsubscribe() { console.log("unsubscribe") } }; });

I would be surprised if any user would expect an Observer's unsubscribe method to be called in response to errors and completes when Observer already has error and complete methods.

In order for this to be true, developers would have to expect different behavior when they invoked the Subscription's unsubscribe method vs. when the Observable invoked the Subscription's unsubscribe method. Consider the following concrete example: Observable.nextEvent(element, name) = new Observable(observer => { const handler = e => { observer.next(e); observer.complete(); }; element.addEventListener(name, handler); return { unsubscribe() { element.removeEventListener(name, handler); }; }); In the example above, the developer expects the Observable to invoke unsubscribe when the Observer's complete method is invoked. Why would a developer assume the Observer would get different notifications when the Observable invoked the unsubscribe method on the Subscription rather than the developer? I want to clarify that I'm interested in the use case, and I'd like to explore this further. The challenge is the proposed API is likely to violate user expectations.

…

-- some edits to clean up formatting and grammar --

[jhusain]

I think it could be argued that the proposed semantics would match user expectations if we were to remove the ability to return a Subscription from the Subscriber function. Unfortunately that would make Observable composition less ergonomic, and composition is a very common operation.

Putting aside finalization which I think is best served with finally, the rationale for the proposed semantics appears to be enabling applications to observe the reason for unsubscription. It's been pointed out that this use case can be supported through composition - though it's admittedly awkward.

In order to justify making Observable composition less ergonomic, we should have evidence that observing unsubscription is a very common use case. This may well be the case, but it would presumably mean that there are many projects working around the broad lack of support for subscription observation in the userland Observable libraries.

Interested to see if we can think of an approach to enabling unsubscription observation which would both match user expectations and ensure that Observable composition remains as easy as it is today.

[leebyron]

That makes a lot more sense that you were referring to Observable composition in your example of the unsubscribe method being called. I still make the claim that any confusion for this particular case exists already and that adding an ability to observe unsubscription does not change that.

In particular, your concrete example above highlights existing confusion due to the format of the cleanup function:

  return {
    unsubscribe() {
      element.removeEventListener(name, handler);
    };
  });

Since this is not returning a Subscription instance, it's really just taking advantage of the cleanup function's behavior such that it's equivalent to:

  return function () {
    element.removeEventListener(name, handler);
  };

In my opinion this is the existing confusion between cleanup and unsubscription which is why I was initially confused by your statement:

In the example above, the developer expects the Observable to invoke unsubscribe when the Observer's complete method is invoked.

Let me rephrase this as: the developer expects the Observable to invoke it's cleanup function when the Observer's complete method is invoked. This rephrasing should still capture what's happening and is a good developer expectation. Of course, that cleanup function could be invoking a composed Observable's Subscription's unsubscribe method.

I think it could be argued that the proposed semantics would match user expectations if we were to remove the ability to return a Subscription from the Subscriber function. Unfortunately that would make Observable composition less ergonomic, and composition is a very common operation.

I would not support removing this ability since I agree with you about both ergonomics and how common this is. I also do not think it would be necessary to remove this ability to match developer expectations, which I believe are already met through an existing precaution:

The critical difference from a typical object with an unsubscribe method, is that a Subscription instance's unsubscribe method has no effect if a Subscription is already closed (step 4). This is critical to matching developer expectations if allowing a Subscription instance to be returned from the Subscriber function.

In this proposal, the step of checking if the Subscription is closed needs to be performed before invoking an unsubscribe method on an Observer both to ensure that Observer method is invoked at most once, and to match developer expectations about only being invoked as a result of being externally completed.

To illustrate this behavior, I wrote a code example in a comment above which shows when an unsubscribe Observer method would be invoked.

[leebyron]

I'd like to better understand where the potential for violation of developer expectations exist so we can better test that.

Please let me know if this captures the confusion / expectation-mismatch that you see:

When composing Observables, an outer composing Observable typically unsubscribes from an inner composed Observable within its cleanup function. Because of this common pattern, we expect many developers to conflate cleanup and unsubscription such that, while they should expect an Observable's cleanup function to be called after that Observable results in error or complete, they could misinterpret that as an Observable's Subscription unsubscribing from itself, such that if we allow "unsubscribe" to be observed in an Observer, that users could incorrectly expect this Observer method to be called during cleanup (resulting from error or complete) rather than when the Observed Subscription was unsubscribed.

I empathize with this confusion, but would be sad to see it be the lone reason to turn down this proposal.

[jhusain]

Your summary of my concerns about the proposed semantics is accurate. You also make a valid point which is that there is an important difference between the Subscription-like (i.e. object with a subscribe method) and a Subscription: Subscriptions ensure the idempotence of the unsubscribe method. However it's not clear to me this distinction will lead developers to conclude that the Observer's unsubscribe method will not be called when the Subscription-like's unsubscribe method is called. The fact that these methods have the same name sends a very strong signal.

One counter argument that occurs to me is that developers may be unlikely to expect that unsubscribe will be invoked along with complete/error because Promises accept multiple callbacks and only invoke one. However Promises do not support unsubscription, so it's difficult to use Promises to draw conclusions about developers expectations.

It is interesting to consider whether developers might be less likely to assume Observer's unsubscribe was invoked along with complete/error if Observable.prototype.finally was part of the specification. I say this because I believe some developers may assume that unsubscribe is invoked alongside complete and error in order to facilitate finalization. The existence of the finally method may lead developers to believe that finally is necessary because only one of the Observer callbacks is invoked on unsubscription.

Ultimately none of these counter arguments override my concerns that the proposed semantics will violate user expectations. However I must admit I find this frustrating. I agree that the proposal is the most straightforward way of supporting the use case. There is also a satisfying completeness about the Observer receiving a notification for every possible cause of subscription termination. I've been thinking about how we might rename methods to differentiate between cleanup and unsubscription, but I have yet to come up with an API that is not significantly more complex. It's difficult to justify this complexity for a use case that appears to be limited. I want to reiterate that I am very open to ideas about how to pull apart cleanup and unsubscription in an ergonomic way.

[zenparsing]

Note that you can use the closed property of subscriptions to make the workaround a little nicer:

function withUnsubscribe(observable, onUnsubscribe) {
  return new Observable(sink => {
    let subscription = observable.subscribe(sink);
    return () => {
      if (!subscription.closed) {
        subscription.unsubscribe();
        onUnsubscribe();
      }
    };
  });
}

It seems like a do operator would be an appropriate place for this feature. A do operator could even avoid the naming issue by using onX names to differentiate their semantics, like so:

function withLogging(observable) {
  return observable.do({
    onStart() {
      LoggingAPI.begin();
    },
    onError() {
      LoggingAPI.reportError();
    },
    onComplete() {
      LoggingAPI.reportSuccessfulCompletion();
    },
    onUnsubscribe() {
      LoggingAPI.reportClientDidCancel();
    },
  });
}

[benjamingr]

To be honest I'm in general -1 on adding any changes to the observable proposal that do not directly address the TC39 concerns about EventTarget and other consumers.

Especially ones that can be added later like this one

[benlesh]

Currently, we resort to using do() to track whether it was an internal completion and then act on that in finally().

@matthewwithanm, I've started an issue so we can maybe get RxJS to address this directly. Seems straightforward enough.

[benlesh]

In a world where Observable actually returns an AbortController instead of Subscription (ala the WHATWG Proposal here), you'd get events for "unsubscribe" by default:

const abortController = observable.subscribe(observer);

abortController.signal.addEventListener('abort', handleUnsubscribe);

I'll admit, the vernacular of "abort" and "subscribe" is a little yucky, but that proposal is aiming to work with existing types, as AbortController is just a Subscription by another name.

Would that work for you, @leebyron?