SafeSubscriber does not unsubscribe itself

[czterocyty]

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.

[jayphelps]

@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

[martin-the-geek]

Any work being done on this. Just noticed a memory leak in my angular application because of this.

[volser]

Any news? I have the same issue (

[Medo42]

I just spent several hours of debugging until identifying this issue. It caused our application to never unsubscribe from an observable that fired a request every few seconds, so as the user kept using the application the frequency of requests would go up and up.

So +1 on this issue, please give this more priority.

[Medo42]

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: a1778e0#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

[cghislai]

Still happening with recent versions of rxjs 5. this is rather severe in angular applications as component instances are not garbage collected if they leak subscriptions.

[GuichiZhao]

+1

[nanoTitan]

+1

[perfilyev]

+1

[BioPhoton]

@czterocyty and @jayphelps.
IMHO this issue is already solved in the latest version.
If I'm right, please any of u 2 could close the issue.
Otherwise please ignore my comment.

[jayphelps]

@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.

[startswithaj]

This is still happening.

[cartant]

(For my own reference) this issue is somewhat related to this one: #5243

[trxcllnt]

Here ya go everybody #5311

[trxcllnt]

@Medo42 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.

Yeah that's a problem. My fix in #5311 should address this if the unknown Subscriber-like (or otherly-imported Subscriber) is the terminal Subscriber in the chain.

We could add more robust checks than instanceof Subscriber to determine if the Subscriber-like should be treated like one of our own (checking for add, remove, unsubscribe, etc. methods), but I worry that could fail in other ways.

[Medo42]

Thank you very much for looking into this.

I'm very hazy on the details at this point and haven't even used rxjs in a while, so sadly I can't be of much help. I still wonder why rxjs uses this "subscriber" model internally though, instead of more "naively" implementing the observable/observer interfaces.

[trxcllnt]

At the time the approach we took was based on throughput benchmarks. Some things are better now with turbofan, but in 2015/16 dynamic/duck-typed object property lookups were expensive.

[cartant]

Closed by #5472 IIRC