Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(positioning): prevent memory leak in positioning service #6724

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

AurelienRichez
Copy link

While investigating performance issues on a page with a lot of tooltips, I stumbled on a memory leak in ngx-bootstrap.

Reproducer

Small reproducer creating a bunch of tooltip:
https://stackblitz.com/edit/stackblitz-starters-w3my1g3z?file=src%2Fmain.ts

steps:

  1. Open the devtools memory pane.
  2. important: make sure that you are looking at the preview iframe (ie the VM instance named stackblitz[something]--4200--[something].webcontainer.io). Otherwise you will only see the memory usage for the stackblitz editor itself. Alternatively you can open the preview in another tab.
  3. take a snapshot of the memory
  4. click the Show/Hide button a few times in the web page
  5. take another snapshot of the memory
  6. We can see that the memory rises up significantly even if we go back to the "hidden" state where no tooltip is rendered (to be certain of this you can also trigger a garbage collection directly in the chrome devtools, or under about:memory in firefox)

result:

Screenshot 2025-01-07 at 15 47 37

Explanation

PositioningService never unsubscribes from triggerEvent$. It would be mostly harmless if there was only one instance of the service but we get one instance of PositioningService per tooltip because it is explicitly in the directive providers.

providers: [
ComponentLoaderFactory, PositioningService
]
})
export class TooltipDirective implements OnInit, OnDestroy {

I settled on simply adding takeUntilDestroyed in the service since it is less likely to have some unforeseen impact in other components compared to force the service to be a singleton.

After the change and using the same scenario the memory stays roughly equivalent between the two snapshots.

Screenshot 2025-01-07 at 16 33 42

note:

I'm not sure if having several instances is intentional or not: since we have {providedIn: 'root'} I initially thought that it was a mistake and it should be a singleton, but the service also expose setOptions which makes it mandatory to have several instances if we want to have different options per component (I think #5706 is an attempt to fix that).

PR Checklist

Before creating new PR, please take a look at checklist below to make sure that you've done everything that needs to be done before we can merge it.

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated tests.
  • added/updated API documentation.
  • added/updated demos.

PositioningService is listening to events in the constructor but it
never got cleaned up on destroy. Since we can have many instances of
that service, it could pile up and use more and more memory.
@AurelienRichez
Copy link
Author

@lexasq I reckon you are the go-to person for review? I tried giving as much details as possible for future reference. The PR description is a bit long but I kept the code change minimal.

Tell me if you need anything else from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant