Memory leaks caused by shareReplay when shared observable is synchronous or continuous

[MaximSagan]

Bug Report

Current Behavior
Under certain common conditions, use of the shareReplay operator will cause memory leaks. This is similar to what was documented in #5034, but although that issue was marked as resolved by #5044, it can be observed in the linked example that the problem persists to a considerable degree.

Reproduction
https://stackblitz.com/edit/angular-a27w1c
https://angular-a27w1c.stackblitz.io/

1. Open up https://angular-a27w1c.stackblitz.io/ in Chrome
2. Use the checkboxes to create and destroy each of the components.
3. Once they are all destroyed, use the devtools Memory tab to take a heap snapshot, and search for the word "Component".
4. Observe that while the AsyncSingleComponent was indeed destroyed, the SyncComponent and the AsyncContinuousComponent have stuck around.

Expected behavior
Each of the three components should have their class instances garbage-collected.

Environment

• See Stackblitz above
• RxJS version: 6.6.3

[josepot]

Hi @MaximSagan,

I just spent some time looking into this. However, I haven't touched Angular since AngularJS 😬 , so forgive me if I'm missing something here.

Unless I'm missing something, I think that the only thing that seems problematic is the fact that the SyncComponent is not being destroyed.

I think that it's understandable that AsyncContinuousComponent is not being destroyed, given the fact that shareReplay(1) does not (by default) use a refcount. Therefore, I think that the AsyncContinuousComponent should be destroyed if its underlying stream was enhanced using shareReplay({bufferSize: 1, refCount: true}), instead? Again, I'm not familiar with Angular, so I could be missing something here.

I just opened #5932, which I think that should solve (at least) the issue with the memory leaks produced by streams that complete synchronously.

[MaximSagan]

Hey @josepot, I really appreciate your quick response and PR.

I think that it's understandable that AsyncContinuousComponent is not being destroyed, given the fact that shareReplay(1) does not (by default) use a refcount.

Maybe I'm missing something, but I thought the point of refCount: true was only to clear the buffer when the number of subscriptions drops to zero, not to also release the subscribers from memory. (Sorry if I'm using the wrong RxJS terminology there.) I would think that, regardless of what kind of observable is being subscribed to, AsyncContinuousComponent should be able to subscribe and then unsubscribe from that observable with confidence that nothing about that observable/subscription will retain any reference to it, preventing it from being garbage collected. I feel like that's part of the guarantee of unsubscribe(), is it not?

[josepot]

Hey @MaximSagan, sorry for confusing you. My bad, you are right: the fact that the observable produced by shareReplay(1) keeps an underlying subscription to the observable produced by timer(1000) even when it has no subscribers left, shouldn't be a reason for the component not being garbage-collected. Because the underlying subscription shouldn't hold any references towards the first subscriber of the observable produced by shareReplay(1). I wish I could find a better way to explain this 😅 . I will keep investigating, but I think that it's possible that the changes that I've proposed in my PR already address this issue... but I want to make sure that's the case.

[josepot]

Hi @MaximSagan, I've been able to test the changes of my PR against your example, and those changes seem to fix the memory leak. Could you please confirm that's the case, please?

[MaximSagan]

@josepot Works perfectly in my example! Thanks so much!

[benlesh]

Okay... so looking into this there's a few things:

1. The SyncComponent part of your example is absolutely a memory leak in RxJS 6.6.3. However that memory leak does not exist in 7.0.0-beta (master). I'm not sure what we're going to do about 6.6.3, since it's for an edge case (sharing a synchronous observable), and it's fixed in 7.
2. The the default behavior of shareReplay is the cause of the other memory leak here, but it's expected behavior (I'll explain below)
3. Retaining the entire component is a result of referencing this in your handlers. Not that there's anything wrong with that. Just calling it out, because this surprises people about uses of this.

TL;DR: the default use of shareReplay should never be used with never-ending observables.

By default, shareReplay is designed for use with "heavy" async calls that you don't want to redo. Once upon a time, this operator was called cache, but people didn't like that name. By default, shareReplay will forgo reference counting under the hood, and once initially subscribed to, will maintain that underlying subscription to the source until the source completes. This is done in order to optimistically keep fetching something in the assumption that whatever that thing is, it's resource intensive, slow, or heavy.

People have consistently been fooled by this, and there have been more than a few issues opened. At one point it was proposed that we move to a reference counted method (what you were expecting), however the community was split... 50/50 on that decision, and it would break a lot of code, so we introduced a configuration object to allow users to opt-in to the behavior you're expecting.

What you wanted to do, on that never-ending interval was shareReplay({ bufferSize: 1, refCount: true }). The refCount: true part means that it will have the behavior you're thinking it will, where if all subscribers unsubscribe, the source subscription is also unsubscribed.

I've updated your example to use [email protected] and the proper use of shareReplay here.

Further reading: We're making all of the sharing operators a lot more explicit in their configuration and unifying them at the same time in an upcoming release. Hopefully this helps get rid of some of the mistakes/confusion people are making around this.

I'm very sorry that this has cost you time and effort, and I really do appreciate all of the effort you've put in. This has been a tricky API to manage, from a community standpoint.

[MaximSagan]

@benlesh

I appreciate the comprehensive response. It's good to know that RxJS 7 solves this issue in my first example.

the default use of shareReplay should never be used with never-ending observables

This is a fair enough statement to make, but I'm not sure that people are going to listen. I often see people using shareReplay as a default solution when they have a never-ending observable that is being mapped to another value using some relatively computationally-intensive logic that they don't want to repeat.

I know that you've already reviewed the PR I made (#5934), where I identified that at a lower level, the memory leak is due to Subscriber's destination property (at least in RxJS 6), and it was deemed that the changes were unsafe. However, I wonder if you (or @cartant) might have a chance to look at @josepot's PR (#5932) that he mentioned above, where he solves the issue for shareReplay directly (without touching the Subscriber implementation as I did) by simply setting a couple closures to undefined. It seems (at least to me) fairly uncontroversial, and it fixes both memory leaks I identified above.

[JontyMC]

I am confused by this:

What you wanted to do, on that never-ending interval was shareReplay({ bufferSize: 1, refCount: true }). The refCount: true part means that it will have the behavior you're thinking it will, where if all subscribers unsubscribe, the source subscription is also unsubscribed.

This option exists in v6.6.3. Are you saying this option has a bug in v6.6.3 but works as intended in v7 or should it work as above in v6.6.3 as well? In which case why do you need to upgrade to v7 to fix this?

Regarding this comment:

I'm not sure what we're going to do about 6.6.3, since it's for an edge case (sharing a synchronous observable), and it's fixed in 7.

I agree with @MaximSagan, I don't think this is an edge case. We are using it to cache computationally-intensive logic when using aurelia-store (a redux like state store based on BehaviorSubject). I assumed shareReplay({ bufferSize: 1, refCount: true }) would unsubscribe the source.