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

[Windows] Fix memory leak with GroupedItemTemplateCollection #23386

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Foda
Copy link
Member

@Foda Foda commented Jul 1, 2024

Description of Change

Fix missing event cleanup in GroupedItemTemplateCollection to match the cleanup pattern in ObservableItemTemplateCollection. This fixed a minor memory leak with grouped item collections on Windows

Issues Fixed

Fixes #22954

@Foda Foda added platform/windows 🪟 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Jul 1, 2024
@Foda Foda requested a review from a team as a code owner July 1, 2024 16:48
@Foda Foda requested a review from mattleibow July 1, 2024 16:48
@Foda
Copy link
Member Author

Foda commented Jul 10, 2024

/rebase

@Foda
Copy link
Member Author

Foda commented Aug 19, 2024

/rebase

@Foda
Copy link
Member Author

Foda commented Aug 22, 2024

/azp

Copy link

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@Foda
Copy link
Member Author

Foda commented Aug 22, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

readonly WeakNotifyCollectionChangedProxy _proxy = new();
bool _disposedValue;

~GroupedItemTemplateCollection() => _proxy.Unsubscribe();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Dispose you do a null check with ? operator... Wouldn't make sense to do it here as well?

@Foda
Copy link
Member Author

Foda commented Sep 23, 2024

/rebase

1 similar comment
@jsuarezruiz
Copy link
Contributor

/rebase

Comment on lines +333 to +335
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a helper method that does it more robustly, is there not?

This one

public static async Task<bool> WaitForCollect(this WeakReference reference)
, I believe.

@Foda
Copy link
Member Author

Foda commented Nov 14, 2024

/rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView platform/windows 🪟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Windows] GroupedItemTemplateCollection CollectionChanged event not unsubscribed, causes memory leak.
5 participants