Skip to content

Weak subscription to CanExecuteChange events #29837

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

Merged
merged 7 commits into from
Jun 17, 2025
Merged

Conversation

PureWeen
Copy link
Member

@PureWeen PureWeen commented Jun 5, 2025

Description of Change

This pull request introduces a mechanism for managing and cleaning up command event subscriptions when the lifetime of the ICommand instance exceeds that of the XAML element it's assigned to.

Example from the Unit Tests

var command = new CommandWithoutWeakEventHandler();

// The Button subscribes to "command.CanExecuteChanged", creating a strong reference
// from the command to the Button. If the command outlives the Button,
// this prevents the Button from being garbage collected, resulting in a memory leak.
// Our built-in Command implementation avoids this by using WeakEventManager,
// but custom ICommand implementations that do not use weak event patterns are prone to leaks.
{
	var button = new Button();
	button.Command = command;
}			

This largerly comes up with scenarios using static viewmodels or if people are using reference bindings. The key changes include the addition of a WeakCommandSubscription class, updates to the ICommandElement interface, and new unit tests to verify the functionality.

Memory Management Enhancements:

  • Added WeakCommandSubscription class: Introduced a new class to manage weak references to command subscriptions, ensuring that event handlers do not prevent garbage collection of associated objects. This includes the CommandCanExecuteSubscription inner class for handling CanExecuteChanged events. (src/Controls/src/Core/WeakCommandSubscription.cs)

  • Updated ICommandElement interface: Added a CleanupTracker property to manage the lifecycle of command subscriptions. (src/Controls/src/Core/ICommandElement.cs)

  • Integrated WeakCommandSubscription in controls: Implemented the CleanupTracker property in multiple controls (Button, ImageButton, MenuItem, RefreshView, SearchBar) to utilize the new subscription management system. [1] [2] [3] [4] [5]

Command Handling Improvements:

  • Modified CommandElement static class: Updated OnCommandChanging and OnCommandChanged methods to use the CleanupTracker for disposing of old subscriptions and creating new ones. (src/Controls/src/Core/CommandElement.cs)

Unit Testing Enhancements:

  • New tests for garbage collection: Added tests to ensure that controls with commands can be garbage collected properly and that weak event handlers are not collected prematurely. (src/Controls/tests/Core.UnitTests/ButtonUnitTest.cs)

  • Improved garbage collection helper: Enhanced the TestHelpers.Collect method to force multiple garbage collection cycles and ensure deterministic behavior during testing. (src/Controls/tests/Core.UnitTests/TestHelpers.cs)

These changes collectively improve the robustness of command handling in the library while addressing potential memory management issues.

Concerns

  • Calling Managed code from the finalizer typically isn't recommended. The only purpose of the finalizer is to cleanup CommandCanExecuteSubscription. The commandelement itself will cleanup without the finalizer. One approach we could take here would be to track these and then just clean them up every so often.

Other Notes

Issues Fixed

Fixes #16124

@PureWeen PureWeen added this to the .NET 9 SR9 milestone Jun 6, 2025
@PureWeen PureWeen added the p/0 Work that we can't release without label Jun 6, 2025
@PureWeen PureWeen moved this from Todo to Ready To Review in MAUI SDK Ongoing Jun 6, 2025
@github-project-automation github-project-automation bot moved this from Ready To Review to Changes Requested in MAUI SDK Ongoing Jun 9, 2025
@PureWeen PureWeen requested a review from mandel-macaque June 12, 2025 12:06
@PureWeen
Copy link
Member Author

/rebase

@PureWeen PureWeen marked this pull request as ready for review June 12, 2025 12:09
@PureWeen PureWeen requested a review from a team as a code owner June 12, 2025 12:09
@Transis-Pedro
Copy link

I've been testing this PR and it solved a lot of leaks.

It's a hit!

Thank you guys

@PureWeen PureWeen changed the base branch from main to inflight/current June 17, 2025 20:38
@PureWeen PureWeen merged commit 4250824 into inflight/current Jun 17, 2025
129 of 130 checks passed
@PureWeen PureWeen deleted the fix_16124 branch June 17, 2025 20:38
@github-project-automation github-project-automation bot moved this from Ready To Review to Done in MAUI SDK Ongoing Jun 17, 2025
PureWeen added a commit that referenced this pull request Jun 21, 2025
* Weak subscription to CanExecuteChange events

* - add tests

* - fix lifecycle of CanExecute subscription

* - cleanup code based on Pictos recommendations

* - add a netstandard_20 version of DependentHandle

* - add more

* - fix missing null check on textcell
PureWeen added a commit that referenced this pull request Jun 25, 2025
* Weak subscription to CanExecuteChange events

* - add tests

* - fix lifecycle of CanExecute subscription

* - cleanup code based on Pictos recommendations

* - add a netstandard_20 version of DependentHandle

* - add more

* - fix missing null check on textcell
PureWeen added a commit that referenced this pull request Jun 25, 2025
* Weak subscription to CanExecuteChange events

* - add tests

* - fix lifecycle of CanExecute subscription

* - cleanup code based on Pictos recommendations

* - add a netstandard_20 version of DependentHandle

* - add more

* - fix missing null check on textcell
github-actions bot pushed a commit that referenced this pull request Jun 26, 2025
* Weak subscription to CanExecuteChange events

* - add tests

* - fix lifecycle of CanExecute subscription

* - cleanup code based on Pictos recommendations

* - add a netstandard_20 version of DependentHandle

* - add more

* - fix missing null check on textcell
github-actions bot pushed a commit that referenced this pull request Jun 26, 2025
* Weak subscription to CanExecuteChange events

* - add tests

* - fix lifecycle of CanExecute subscription

* - cleanup code based on Pictos recommendations

* - add a netstandard_20 version of DependentHandle

* - add more

* - fix missing null check on textcell
PureWeen added a commit that referenced this pull request Jun 27, 2025
* Weak subscription to CanExecuteChange events

* - add tests

* - fix lifecycle of CanExecute subscription

* - cleanup code based on Pictos recommendations

* - add a netstandard_20 version of DependentHandle

* - add more

* - fix missing null check on textcell
github-actions bot pushed a commit that referenced this pull request Jun 27, 2025
* Weak subscription to CanExecuteChange events

* - add tests

* - fix lifecycle of CanExecute subscription

* - cleanup code based on Pictos recommendations

* - add a netstandard_20 version of DependentHandle

* - add more

* - fix missing null check on textcell
PureWeen added a commit that referenced this pull request Jun 27, 2025
* Weak subscription to CanExecuteChange events

* - add tests

* - fix lifecycle of CanExecute subscription

* - cleanup code based on Pictos recommendations

* - add a netstandard_20 version of DependentHandle

* - add more

* - fix missing null check on textcell
PureWeen added a commit that referenced this pull request Jun 28, 2025
For more information about inflight process check
https://github.com/dotnet/maui/wiki/Inflight-Branch-Process

# .NET MAUI Release Notes - Inflight/Candidate Branch

## What's Changed

### MAUI Product Fixes
* [iOS] CarouselView with CarouselViewHandler2 make app crash when
Loop="False" and user scroll to the last position - fixes #26863 by
@kubaflo in #26868
* Fixes Setting BackgroundColor to null does not actually changes
BackgroundColor - fixes #22914 and #19576 by @jgonzalez-gft in
#22917
* Fixed the picker title's color - fixes #16737 by @kubaflo in
#23075
* [android] Fallback to default icons in SearchHandler by @aheubusch in
#25067
* ScrollView's Background on iOS - fixes #24016 by @kubaflo in
#25541
* [iOS] Enabled MultiTouch Support for Handling Multi-Touch Points in
GraphicsView - fixes #29461 by @prakashKannanSf3972 in
#29895
* Optimize converters for GridLength, ColumnDefinition, and
RowDefinition - performance improvement by @emiller in
#20048
* Add defensive IsAlive check to Android ViewExtensions.OnUnloaded -
fixes #28051 by @jfversluis in #29934
* [Windows] Fixed runtime update issue for SearchBar PlaceholderColor
and BackgroundColor - fixes #29962 by @Tamilarasan-Paranthaman in
#29965
* Weak subscription to CanExecuteChange events - fixes #16124 by
@sneumaier in #29837
* [iOS, Mac] Fix for downsized image retaining original dimensions in
GraphicsView - fixes #30006 by @SyedAbdulAzeemSF4852 in
#30007
* [Android] Prevent Picker from Gaining Focus on Touch - fixes #19739,
#8546, #13503, #24862, #28121, #21704, #15394 by @bhavanesh2001 in
#29068
* Fix CV1 GridItemsLayout centering single item AND Fix Empty view not
resizing when bounds change - fixes #29595, #29634 by @albyrock87 in
#29639

### Testing
* [Testing] Feature Matrix UITest Cases for Button by @TamilarasanSF4853
in #29803
* [Testing] Feature matrix UITest Cases for BoxView Control by
@HarishKumarSF4517 in #29808
* [Testing] Enable HandlerDoesNotLeak for Button and ProgressBar by
@bhavanesh2001 in #29896
* [Testing] Add Validation Test For Issue28051 On Android by
@prakashKannanSf3972 in #30026
* [Testing] Fixed Test case failure in PR 30115 - [2025/06/23] Candidate
by @HarishKumarSF4517 in #30136

### Dependency Updates
* Bump to 1.7.250606001 of WindowsAppSDK by @sneumaier in
#29915

### Housekeeping
* [housekeeping] Update namespaces in HostApp and Shared tests projects
by @bhavanesh2001 in #29904
* Update SetterSpecificity.cs Remove Extra Line From Bad Merge by
@sneumaier in #29987
* Revert - Fixed the Label not sized correctly on Android by @Ahamed-Ali
in #30023
* Revert "Fixes Setting BackgroundColor to null does not actually
changes BackgroundColor #22914 (#22917)" by @mattleibow in
#30031
* [create-pull-request] automated change by @github-actions[bot] in
#30019
* [create-pull-request] automated change by @github-actions[bot] in
#30043
* [create-pull-request] automated change by @github-actions[bot] in
#30078
* Update Controls.TestCases.HostApp.csproj by @HarishKumarSF4517 in
#30124

## New Contributors
* @albyrock87 made their first contribution in
#29639
* @SyedAbdulAzeemSF4852 made their first contribution in
#30007
* @emiller made their first contribution in
#20048
* @jgonzalez-gft made their first contribution in
#22917
* @aheubusch made their first contribution in
#25067

**Full Changelog**:
https://github.com/dotnet/maui/compare/main..inflight/candidate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p/0 Work that we can't release without
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

RefreshView can have a memory leak with a custom ICommand
5 participants