shareReplay memory leak

[gogakoreli]

Bug Report

Current Behavior
After component gets destroyed, its reference stays alive, because the fact that I subscribe to bigger scoped Observable (for example route.params) and switch it (via switchMap) to the Observable piped with shareReplay operator.

After taking heap snapshots via chrome devtools, I noticed that reference retainers mentioned shareReplay operator. If I use publishReplay and refCount instead of shareReplay problem is fixed, that's why I think shareReplay has an issue.

Please have a look at code on GitHub and screen shots for better understanding the issue. (Code explains issue better than description). You can see code sample below.

Event after reviewing #3336, issue still remains with shareReplay.

Reproduction

• Repo link: Please have a look at GitHub repo
• StackBlitz: Same repo source code imported from GitHub Here

export class PersonComponent implements OnInit, OnDestroy {

  public person: any;

  private unsubscribe$ = new Subject();

  constructor(private api: ApiService, private route: ActivatedRoute) { }

  ngOnInit() {
    this.route.params.pipe(
      switchMap(_ => this.api.find()),
      takeUntil(this.unsubscribe$),
    ).subscribe({
      next: (person) => {
        this.person = person;
        console.log(person);
      },
      complete: () => console.log('subscription completed'),
    });
  }

  ngOnDestroy() {
    this.unsubscribe$.next();
    this.unsubscribe$.complete();
    console.log('person component destroyed')
  }
}

@Injectable({ providedIn: 'root' })
export class ApiService {
  private cache = {};

  constructor(private http: HttpClient) {
  }

  public find(id = 1): Observable<any> {
    let res = this.cache[id];
    if (!res) {
      res = this.http.get(`https://swapi.co/api/people/${id}`).pipe(
        shareReplay({ bufferSize: 1, refCount: true }),
      )
      this.cache[id] = res;
    }
    return res;
  }
}

Expected behavior
After person component gets destroyed (for example, via navigation) it should be picked up by garbage collector and no reference of it should exist in the application.

Environment

• Runtime: Chrome latest
• RxJS version: 6

Possible Solution
To fix the issue instead of shareReplay use

 publishReplay(1),
 refCount(),

Additional context/Screenshots (Heap Snapshots)

This is what happens when I open the application on the home page, navigate to person page and then back to home page (I click garbage collection several times and take heap snapshot). Person component got destroyed but reference still lives in the heap. Under retainers you can spot shareReplay operator.

---

This is what happens when I navigate between home and person components several times and ending on person component. 2 references of it exists in the application.

---

None of these issues exist when using publishReplay(1), refCount().

[cartant]

I suppose the source parameter could be nulled here. Perhaps you might like to try making that change (locally) and reporting back?

[gogakoreli]

After playing with what you suggested these are my observations:

1. source observable, which is piped through shareReplay, completes and shareReplayOperation executes following: isComplete = true; subject.complete(); here
2. due to completion this bit of code gets executed (which is destruction logic of shareReplay as I understand):

this.add(() => {
  refCount--;
  innerSub.unsubscribe();
  if (subscription && !isComplete && useRefCount && refCount === 0) {
    subscription.unsubscribe();
    subscription = undefined;
    subject = undefined;
  }
});

3. because the fact that isComplete = true; if case is not executed, have a look at && !isComplete inside if condition.
4. I updated if condition to have isComplete instead of !isComplete and memory leak has disappeared after inspecting heap snapshots.
5. Lastly, I am not sure why !isComplete is inside if condition. To be honest, I am not sure which one is correct.
6. btw, I left source as it is, without setting it to be undefined after step 5 and it worked.

[cartant]

I suggested the wrong place. Try setting source to undefined after this line:

rxjs/src/internal/operators/shareReplay.ts

Line 105 in ffb4b0b

subject.complete();

And set subscription to undefined too, I guess.

[cartant]

... I am not sure why !isComplete is inside if condition

There's an explanation here: https://ncjamieson.com/whats-changed-with-sharereplay/

[gogakoreli]

I suggested the wrong place. Try setting source to undefined after this line:

rxjs/src/internal/operators/shareReplay.ts

Line 105 in ffb4b0b

subject.complete();

And set subscription to undefined too, I guess.

Thanks that helped.
Setting subscription to undefined helped to free up memory leak, observing on heap snapshots.

[cartant]

Do you want to submit a PR for the change?

[cartant]

Actually, if it's the subscription that has to be set to undefined to prevent the leak, I think the issue is more fundamental. I think that the _unsubscribe property within Subscription should be nulled after these lines:

rxjs/src/internal/Subscription.ts

Lines 63 to 66 in b713d39

	this._parentOrParents = null;
	// null out _subscriptions first so any child subscriptions that attempt
	// to remove themselves from this subscription will noop
	this._subscriptions = null;

E.g.:

this._parentOrParents = null;
// null out _subscriptions first so any child subscriptions that attempt
// to remove themselves from this subscription will noop
this._subscriptions = null;
this._unsubscribe = null;

Can you verify whether or not that solves the problem - with the changes made to the shareReplay implementation removed?

[gogakoreli]

By the way, setting this._unsubscribe = null; didn't help. I am still observing the source code to find solution.

[cartant]

No worries. I'll build your repro project and will have a poke around on the weekend.

Thinking about it again, the subscription will be the actual subscriber to the source, so having to null it as well isn't that surprising. Subscription probably doesn't need to change and the fix is most likely what you had in this comment.

Feel free to open a PR with the fix. I'll verify it once I build your repro, etc. Thanks for finding, reporting and continuing to investigate this. It's appreciated!

[lincolnthree]

I hit this issue recently. Wondering if this is still being considered, or if there are recommended workarounds? Sorry for the un-educated question. Still trying to get my head around all the rxjs paradigm.

[gogakoreli]

The workaround for now is:

publishReplay(1),
refCount(),