rxjs: SafeSubscriber does not unsubscribe itself

When subscribing to Observable with Partial Observer, rxjs creates SafeSubscriber, but on unsubscribe it does not unsubscribe itself. Moreover if a closure is passed to next/error callback of partial observer, this leads to memory leaks, as consumers cannot be freed.

See the code (pure JS).

RxJS version: 5.4.1

Code to reproduce:

via gist: https://gist.github.com/czterocyty/d4ad5dea55316c9a734c64f378975f04

Expected behavior:

I think SafeSubscriber#unsubscribe should be called too upon unsubscription.

Test should pass:


  it('upon subscription, the destination SafeSubscriber should be unsubscribed too', () => {
    const observer = {
      next: function () { /*noop*/ },
      error: function (e) { throw e; }
    };

    const sub = new Subscriber(observer);

    const destination = sub['destination'];
    const destUnsubscribe = destination['unsubscribe'] = sinon.spy();
    const destUnsubscribeProtected = destination['_unsubscribe'] = sinon.spy();

    sub.unsubscribe();

    expect(sub.closed).to.eq(true);
    expect(destination.closed).to.eq(true);

    expect(destUnsubscribe).have.been.called;
    expect(destUnsubscribeProtected).have.been.called;
  });

Actual behavior:

It is not when passing partial observer into Subscriber.

Additional information:

After playing around with buttons, check JS heap and not-freed Foo instance due to handing closures in SafeSubscriber. See https://drive.google.com/open?id=0B4JH51FD7JBZcUJTTVRjRXJ2a00. There is Subscriber which is closed (good), but destination as SafeSubscriber is still not closed.

About this issue

Original URL
State: closed
Created 7 years ago
Reactions: 8
Comments: 18 (6 by maintainers)

Commits related to this issue

fix(SafeSubscriber): Propagate disposal to terminal Subscriber Ensure the internal SafeSubscriber is disposed when an Observable is subscribed with a Subscriber or Subscriber-like object. Fixes #267... — committed to trxcllnt/rxjs by trxcllnt 4 years ago
fix: chain safe subscribers to interop subscribers Closes #5469 #5311 #2675 — committed to cartant/rxjs by cartant 4 years ago
fix: chain interop/safe subscriber unsubscriptions correctly (#5472) * test: add failing interop subscriber test * fix: chain safe subscribers to interop subscribers Closes #5469 #5311 #2675 ... — committed to ReactiveX/rxjs by cartant 4 years ago

Most upvoted comments

Here ya go everybody https://github.com/ReactiveX/rxjs/pull/5311

trxcllnt on Feb 13, 2020

(For my own reference) this issue is somewhat related to this one: https://github.com/ReactiveX/rxjs/pull/5243

cartant on Feb 13, 2020

@BioPhoton FWIW looking back on my original reply, it appears that the latest version of RxJS still does not call the unsubscribe() callback when unsubscribe() is called on the subscription. i.e. nothing has changed, in regards to my particular issue. Also whether or not what I was asking for is something we want, I’m not sure anymore as I’ve lost context of why I cared.

RxJS v6: https://jsbin.com/gibozogero/1/edit?html,js,console

@trxcllnt is this the same unsubscribe issue as what you and I were discussing on Slack? I think so.

its called on error https://jsbin.com/zudivez/edit?js,console

called on complete https://jsbin.com/nazixix/edit?js,console

but not called on unsubscribe https://jsbin.com/lerilub/edit?js,console

Whether it was in fact the same as what the OP had an issue with, I’m not sure.

jayphelps on Oct 3, 2019

I am not completely sure the issue I saw is the same as this one, since I don’t understand the internal workings of rxjs well enough. However, for me the bug seems to have been introduced here: https://github.com/ReactiveX/rxjs/commit/a1778e0dc6b8c3c41d06aa48b3bd72db51873b6d#diff-c377d3777a1fedd1d4744b0bb9eb939cL170

SafeSubscriber used to patch the unsubscribe method of the PartialObserver, so that if the PartialObserver was unsubscribed from, the SafeSubscriber would unsubscribe up the chain as well. The change in that linked commit still patches unsubscribe on the context object, but that is no longer the same object as the original PartialObserver, so an unsubscribe on the original object no longer propagates.

In my case, this causes the unsubscribe on the InnerSubscriber of switchMap to not unsubscribe “up the chain”, so the TimerObservable keeps firing and the chain keeps firing requests to our server even though really nobody is subscribed anymore. Normally this would not happen, because InnerSubscriber (as a Subscriber) would cause a different code path to be taken where the unsubscription is handled properly. However, in my case the switchMap and its InnerSubscriber come from a different copy of the rxjs library than the Observable that it maps, so it does not extend the same Subscriber class and is treated as a PartialObserver. I know that this is not an ideal situation, but IMO it should not cause a difference in behavior, because from a library consumer point of view, the same interfaces are being adhered to.

I have some trouble figuring out how it should work though, because the whole approach of chaining Subscribers seems incompatible with that case. If you have a general PartialObserver of unknown implementation, I simply see no way to react to its unsubscribe, because we don’t even know if it has one.

Edit: The multiple identities of Subscriber seem to be caused by different forms of import, see #2984

Medo42 on Feb 19, 2018