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

Fixes BindableLayout BindingContext inheritance #17290

Merged

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Sep 9, 2023

Description of Change

Usually the BindingContext is automatically inherited via Element.SetParent using SetChildInheritedBindingContext.
When an element is removed from a Layout, the Element.SetParent takes care of clearing BindingContext automatically.

		void SetParent(Element value) {
			// ..
			object context = value?.BindingContext;
			if (value != null)
			{
				value.SetChildInheritedBindingContext(this, context);
			}
			else
			{
				SetInheritedBindingContext(this, null);
			}

We can see that setting the BindingContext to null is an explicit behavior when the parent (value) is set to null (a.k.a. child removed).

BindableLayout sets the BindingContext manually on each created child, so it is its responsibility to clear the BindingContext once the item is removed.

Not clearing the BindingContext might cause unwanted side-effects and leaks if the child view attached to some of its events (i.e. PropertyChanged).

// inside child view class
private MyViewModel _current;
protected override void OnBindingContextChanged() {
    if (_current != null) { 
        // I see no other way to unsubscribe to that event
        // Even if the event is backed by a WeakEventManager there's a risk
        // that an event is invoke before GC finishes cleaning up resources
        _current.MyEvent -= MyHandler;
        _current = null;
    }


    if (BindingContext != null) {
        // Subscribing to an event is totally legit here
        _current = BindingContext;
        _current.MyEvent += MyHandler;
    }
}

On top of that, the view created by EmptyViewTemplate should not have the BindingContext set manually, because it matches the parent one: we should rely on the automatic inheritance.

Issues Fixed

Issue #10904 does not represent a leak itself, but in a more complex scenario where a child attaches to some events on the BindingContext it would create issues.
Issue #19142

@albyrock87 albyrock87 requested a review from a team as a code owner September 9, 2023 10:15
@ghost ghost added the community ✨ Community Contribution label Sep 9, 2023
@ghost
Copy link

ghost commented Sep 9, 2023

Hey there @albyrock87! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@PureWeen PureWeen requested review from jonathanpeppers and removed request for jsuarezruiz September 9, 2023 19:58
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

I'm not following how clearing the BindingContext on children solves a memory leak. Usually leaks are MAUI views that live forever -- not model objects. If a model object lives forever, it means there is a MAUI view that lives forever -- that is the actual problem to be solved.

Looking at:

#10904

We should be able to write a test with BindableLayout as mentioned here:

https://github.com/dotnet/maui/wiki/Memory-Leaks#writing-tests

If we can show an issue with GC.Collect() and WeakReference that will prove what is going wrong here.

@albyrock87
Copy link
Contributor Author

@jonathanpeppers I will add more unit tests to proof what is described in the issue, but besides that, the problem here is also a functional one.

As I've explained, the binding context needs to be cleared the same way it happens with the automatic inheritance.

The unit test I've added show exactly why the current implemention is wrong: because the removed view is still attached to the binding context.

@jfversluis jfversluis added legacy-area-perf Startup / Runtime performance s/pr-needs-author-input PR needs an update from the author memory-leak 💦 Memory usage grows / objects live forever (sub: perf) labels Sep 11, 2023
@ghost
Copy link

ghost commented Sep 11, 2023

Hi @albyrock87. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time.

@samhouts samhouts added this to the .NET 8 GA milestone Sep 11, 2023
@albyrock87 albyrock87 force-pushed the bindable-layout-bindingcontext-leaks branch from 4042d31 to 3c3042b Compare September 14, 2023 08:58
@albyrock87
Copy link
Contributor Author

albyrock87 commented Sep 14, 2023

@jonathanpeppers some updates for you:

  • FlexLayout Memory leak #10904 specific report can be closed because the issue's code is not waiting for pending finalizers (I've added a comment there)
  • I've updated the unit test regarding the EmptyViewTemplate which is now named EmptyViewTemplateContentInheritsLayoutBindingContext and it proofs that the empty view should inherit the BindingContext of the parent even when it changes
  • I've updated the other two unit tests related to leaks with GC execution in order to show that there's no leak (names have been changed too) => you were right, the leak is on the child view and not on the BindingContext.

All the three unit tests are failing on main branch.

Edit: the amend-commit below are just to rename things

@albyrock87 albyrock87 force-pushed the bindable-layout-bindingcontext-leaks branch 2 times, most recently from 1d01752 to 1761325 Compare September 14, 2023 09:10
@albyrock87 albyrock87 changed the title Fixes BindableLayout leaks on BindingContext Fixes BindableLayout leaks and EmptyViewTemplate issues Sep 14, 2023
Comment on lines 398 to 399
var triggeredCount = 0;
var itemTemplate = new DataTemplate(() => new MyViewModelBoundComponent(onTextChangedCallback: () => triggeredCount++));
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove triggeredCount and onTextChangedCallback in these new tests? Nothing asserts against that value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The presence of that variable is what causes the leak, but this doesn't mean that the problem resides in the unit tests.
I guess it all depends on what the MAUI team says.

Comment on lines 412 to 413
// The component should be gone
Assert.Equal(0, MyViewModelBoundComponent.InstanceCount);
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead create a List<WeakReference> that contains each MyViewModelBoundComponent object, and assert IsAlive is false? Then you wouldn't need a manual InstanceCount.

Example:

Assert.False(viewReference.IsAlive, $"{type} should not be alive!");

Comment on lines 657 to 660
if (_myViewModel != null)
{
_myViewModel.PropertyChanged += OnBindingContextFixturePropertyChanged;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this line the actual cause of the leak? You've created a control and viewmodel where the viewmodel has a strong reference to the control. Then at the top of the test you have a viewmodel that lives the lifetime of the test:

var myViewModel = new MyViewModel();

You would need to enclose myViewModel in a scope, so the GC can collect it?

I'm still not sure there is an actual memory leak here, besides the one created in this test. Clearing the BindingContext just makes the event unsubscribe in this custom control and remove the strong reference. I would consider not designing a custom control this way if the viewmodel is known to live longer than the control.

Maybe someone on the MAUI team can comment what the behavior is supposed to be in regards to BindingContext. Should it always get cleared when children are unparented? @PureWeen @mattleibow @jsuarezruiz?

Copy link
Contributor Author

@albyrock87 albyrock87 Sep 14, 2023

Choose a reason for hiding this comment

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

I think the point here is:
What should a View do if it needs to subscribe on a BindingContext property to do some action when something changes there?
How can it unsubscribe properly in the exact moment the view is no more needed?
The view cannot rely on the GC because that might happen at later moment in time, and something can happen in the mean time which would trigger an action on a removed view.
There are things out there on commercial libraries which rely on this behavior of clearing the context, and they work properly only when used outside of BindableLayout.

Copy link
Member

Choose a reason for hiding this comment

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

It might be correct to clear the BindingContext (MAUI team can comment), but these tests have simply created a control that leaks unless its BindingContext is cleared. It might be better to use WeakEventManager or other solutions to avoid the problem entirely.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might make sense. Is the WeakEventManager instantly reacting to the un-reference of the child view?

Anyway for example, ObservableObject in community toolkit does not use weak event manager.

https://github.com/CommunityToolkit/dotnet/blob/main/src/CommunityToolkit.Mvvm/ComponentModel/ObservableObject.cs#L34

I'm just saying this kind of situations happen in complex applications, and we need a clear way to handle them.
I felt that relaying on the way binding context inheritance works was the way.

@samhouts samhouts modified the milestones: .NET 8 GA, Under Consideration Sep 19, 2023
@samhouts samhouts modified the milestones: Under Consideration, .NET 8 GA Sep 28, 2023
@PureWeen PureWeen modified the milestones: .NET 8 GA, .NET 8 SR1 Sep 28, 2023
@albyrock87 albyrock87 force-pushed the bindable-layout-bindingcontext-leaks branch from 1761325 to 5799552 Compare September 29, 2023 08:28
@albyrock87 albyrock87 changed the title Fixes BindableLayout leaks and EmptyViewTemplate issues Fixes BindableLayout BindingContext inheritance Sep 29, 2023
@albyrock87
Copy link
Contributor Author

@jonathanpeppers I see no response from the team, so I've investigated the code myself and it appears to me that clearing the context when the parent is unset is an explicit behavior in Element.cs (see more on PR description).

I've updated the title, description and commit to reflect that this PR is only fixing a wrong behavior.
There are no leaks directly caused by this, unless the developers chooses to subscribe to BindingContext events in child view.

@albyrock87 albyrock87 force-pushed the bindable-layout-bindingcontext-leaks branch from 5799552 to 48a443f Compare November 3, 2023 18:25
- EmptyViewTemplate should inherit BindingContext even when it changes
- BindingContext must be cleared on removed children to:
  - Behave like standard BindingContext inheritance
  - Avoid unwanted side-effects
  - Avoid leaking views in memory when they listen on BindingContext events
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Nov 21, 2023
@albyrock87
Copy link
Contributor Author

This PR is waiting since September 29, it is not stale.

Now that .NET8 GA is done, may I ask for a final review? Thanks
@samhouts @PureWeen

Copy link
Contributor

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

shouldn't you use SetInheritedBindingContext instead of setting the child BindingContext ?

@albyrock87
Copy link
Contributor Author

@StephaneDelcroix that'd be a nice idea, but it would cause BindingContext changing two times:

  1. var view = (View)dataTemplate.CreateContent() => context null
  2. layout.Add(view) => context = inherited from parent
  3. BindableObject.SetInheritedBindingContext(view, item); => context = item

And the problem is that 3 will happen after the element is already in the visual tree.

I'd like to do 3 right after 1, but layout.Add would override the value I've just set.

@Redth Redth removed this from the .NET 8 SR1 milestone Nov 30, 2023
@albyrock87
Copy link
Contributor Author

Any updates on this?

@mattleibow mattleibow removed their request for review February 2, 2024 17:37
@mattleibow
Copy link
Member

@PureWeen I am wondering if this should be using the visual childrent feature you added? I see you opened the OG issue, so not sure if you had thoughts?

@samhouts samhouts removed the stale Indicates a stale issue/pr and will be closed soon label Feb 6, 2024
@albyrock87
Copy link
Contributor Author

@PureWeen any chance to include this in the next 8 SR?

@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label May 10, 2024
@PureWeen
Copy link
Member

/rebase

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

This behavior now mirrors any other control we have that utilizes an ItemSource. CV and LV both also use this pattern when items are added/removed

@PureWeen PureWeen enabled auto-merge (squash) May 16, 2024 15:43
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

@StephaneDelcroix that'd be a nice idea, but it would cause BindingContext changing two times:

  1. var view = (View)dataTemplate.CreateContent() => context null
  2. layout.Add(view) => context = inherited from parent
  3. BindableObject.SetInheritedBindingContext(view, item); => context = item

And the problem is that 3 will happen after the element is already in the visual tree.

I'd like to do 3 right after 1, but layout.Add would override the value I've just set.

Chatted with @StephaneDelcroix some about this. We're going to see about adding an API that will achieve this goal. I did some local tests and I really like how this all works if we can set the item as the Inherited Binding Context.

That basically lets us remove all the code that clears out the BC as well because removing the parent will just clear it out.

I feel like this is also an API we can extend out to CV/LV/etc..

@albyrock87
Copy link
Contributor Author

@PureWeen should I decline the PR then and simply wait for the new API?
I see changes requested but I don't get what I have to do in this situation :)

@PureWeen PureWeen added the area-xaml XAML, CSS, Triggers, Behaviors label May 31, 2024
@PureWeen PureWeen merged commit 7b2fafb into dotnet:main Jun 1, 2024
49 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors community ✨ Community Contribution fixed-in-8.0.60 fixed-in-9.0.0-preview.5.24307.10 legacy-area-perf Startup / Runtime performance memory-leak 💦 Memory usage grows / objects live forever (sub: perf) t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants