content(rxjs): avoid memory leaks

[thekiba]

Use publishReplay and refCount instead of shareReplay to avoid memory leaks.
See ReactiveX/rxjs#3336

[KwintenP]

Hi,

It actually depends on the use case that you have. If you have an http Observable that will just give you one value, than you can perfectly use this.

If you'd want to change this to something that reflects some nuance on when to use which one, I'm definitely up for it. You can read about it in this article that I wrote: https://blog.strongbrew.io/share-replay-issue/

[thekiba]

@KwintenP
It doesn't matter because:
— uses the Observable interface, hoping for its completion makes the code fragile;
— subscription cannot be canceled, it is likely will be critical side effects.

The application should work stably, and not fall with every unexpected change.

[d3lm]

There's an open PR that adds an option to shareReplay but it is still open and not merged yet. I think I would agree that in most cases you'd want to unsubscribe from the source when the ref count goes from 1 to 0. But as Kwinten said, that really depends on the use case and to be on the safe side it's a good idea to use publishReplay and refCount. As long as there is no option to enable this behavior we should advocate the safer option here.

[d3lm]

The PR was merged. shareReplay now accepts a config object. More specifically, the option refCount can be used to control the behavior. If you set the following config { refCount: true } then the subscription to the source is reference counted. By default it's not. We should definitely mention this.

[KwintenP]

I'll update this asap with the refCount and a link to the publishReplay(1).refCount()

[d3lm]

@thekiba Would you like to change this PR to the latest API of shareReplay to avoid possible memory leaks with shareReplay(1). Now that this PR was merged, I think we can change it.

[thekiba]

@d3lm Thanks for, I can.

But what do you think about that? ReactiveX/rxjs#4530
shareReplay still has different behavior with publishReplay and refCount.

[d3lm]

Thanks for changing your PR. I think that what's discussed in the issue you mentioned is for this checklist item negligible because shareReplay, with the new config option, doesn't cause memory leaks anymore. The point of discussion in that issue is that it still doesn't behave the same way as publishReplay().refCount() but that is by design, to make shareReplay and other variants such as share retryable as opposed to the solution with publishReplay.

I think we can merge this one.

@KwintenP What do you say?

[KwintenP]

SGTM. Thanks for your addition!