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

igx-select memory leak #13923

Closed
pmoleri opened this issue Feb 16, 2024 · 6 comments · Fixed by #13926
Closed

igx-select memory leak #13923

pmoleri opened this issue Feb 16, 2024 · 6 comments · Fixed by #13926
Assignees
Labels
🐛 bug Any issue that describes a bug 🔥 severity: high select Select component ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged.

Comments

@pmoleri
Copy link

pmoleri commented Feb 16, 2024

Description

igx-select leaks itself and can make leak other components such as Grid.
related: #11672

  • igniteui-angular version: 17.0.13
  • browser: all

Steps to reproduce (Chrome)

  1. Add a igx-select (see attached stackblitz)

  2. set it up with an ngIf so it can be destroyed and re-created as many times as you want

  3. open the select

  4. destroy without closing the dropdown

  5. re-create it and repeat

  6. Open Dev Tools / Memory tab (Chrome)

  7. Take a memory snapshot (if there's more than one process it's important to pick the correct one)

  8. Search for IgxSelectItemComponent (or "Detached HTMLElement" if you're using production build).
    -> Here you can confirm that there are many leaked elements
    image

  9. Search for IgxSelectionAPIService, select the instance and hover it to inspect its contents
    -> There are entries holding those elements:
    image

  10. Right click to store it as a global var (temp1 in this example)

  11. In the console run temp1.selection.clear()

  12. Grab a new snapshot and search again
    -> Now only the Service and Module are there.
    image

Result

There are memory leaks as verified in the steps

Expected result

No memory leaks

Attachments

Stackbliz: https://stackblitz.com/edit/hr9mt8?file=src%2Fapp%2Fselect-sample-1%2Fselect-sample-1.component.html,src%2Fapp%2Fselect-sample-1%2Fselect-sample-1.component.ts

@pmoleri pmoleri added 🐛 bug Any issue that describes a bug 🆕 status: new labels Feb 16, 2024
@pmoleri
Copy link
Author

pmoleri commented Feb 16, 2024

Analysis of the issue:

There's a root (singleton) service keeping track of selected items for any select/dropdown out there.
image

Even though the DropDown attempts to clear on destroy
image

The Select overrides it and doesn't call the super, producing the leak.
image

Even if fixing the above would prevent the issue, I believe having a root service is adding unnecessary complexity and pitfalls. You can have a per-instance service:
image

One more note. Currently the cleanup, even if it was correct, it's keeping 2 empty Set() for every select/dropdown that has ever been created. So that, even if minor, it's another leak and wrong.

@jsakamoto
Copy link

@pmoleri Please remember that kind of memory leak issues may happen not only in the "igx-select" but also in "igx-grid," "igx-chip," "igx-data-chart," etc.
See also: https://discord.com/channels/836634487483269200/836636712292581456/1205547850411544637

@Lipata
Copy link
Member

Lipata commented Feb 19, 2024

@pmoleri, @jsakamoto this is the fix that will remove the memory leak: https://github.com/IgniteUI/igniteui-angular/pull/13926/files#diff-2b12c6b9ff35bdd97b9a62948ada434845879e91b45cdc33415f98260a2ffa9bL554. @pmoleri thank you for all the suggestions! We will apply the one above and in a separate PR will make the selection service imported on a component level instead of on a root level, which will also remove the need to delete the empty sets that are currently left on each selection.

@jsakamotoIGJP
Copy link

Hi @Lipata, thank you for your quick response!

By the way, I have a question regarding the memory leak issue that was brought up on the Discord server (see also here). Will the other components mentioned by the user, such as igx-grid and igx-chip, be fixed soon? Or should I log them as separate issues?

Thank you for your help!

@Lipata
Copy link
Member

Lipata commented Feb 23, 2024

@pmoleri in addition to my previous comment - we removed the select memory leak and also now the selection service is deleting the set when the selection is empty instead of setting it as an empty set. Providing the selection service on a component level instead of on a root, will not happen, as per #13938 (comment):

Clarification on the rejected status - after discussing today again, we're actually of the opinion this change might not be desirable. The root-provided service should only be included when actually needed and tree-shaken otherwise and, once the related leak was resolved, its overhead (empty map more/less) is quite minimal. On the other hand, providing it per component (sandboxing it) will create instance per component and there's a real concern with repeated/templated Selects/Combos for example and more dropdowns around the view UI we can end up with a hefty number of instances, which could be a worse tradeoff.

@jsakamotoIGJP
Copy link

jsakamotoIGJP commented Mar 18, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Any issue that describes a bug 🔥 severity: high select Select component ✅ status: resolved Applies to issues that have pending PRs resolving them, or PRs that have already merged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants