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 memory leaks with events. #547

Merged
merged 12 commits into from
Mar 27, 2014
Merged

Fix memory leaks with events. #547

merged 12 commits into from
Mar 27, 2014

Conversation

jlaanstra
Copy link
Member

This PR adds a WeakEventManager class to fix all kinds of memory leak issues that the different Xaml platforms have. Since these changes could break existing applications that depend on the strong references it would be great to add this to RxUI 6 before it is released.

Problem

Solution

To fix all memory leaks related to event subscription from the UI, WeakEventManager instances have been added.

  • The CanExecuteChanged event of ReactiveCommand is now handled with a CanExecuteChangedEventManager.
  • The CollectionChang(ing|ed) events of ReactiveList are now handled with a CollectionChang(ing|ed)EventManager.
  • The PropertyChang(ing|ed) events of IReactiveObject are now handled with a PropertyChang(ing|ed)EventManager.

Implementation

The implementation is based on the WeakEventManager class in WPF and the code in http://social.msdn.microsoft.com/Forums/silverlight/en-US/34d85c3f-52ea-4adc-bb32-8297f5549042/command-binding-memory-leak?forum=silverlightbugs
It contains a ConditionalWeakTable for the Target <-> Delegate mapping to keep the reference to the event handler alive as long as the control itself is alive.
The second ConditionalWeakTable exists to make sure the event manager does not leak the source of the event. This would be the ICommand instance in the case of the CanExecuteChanged event.

TODO:

  • Add unit tests that verify there are no leaks. I already verified with test applications that these changes solve the above issues.
  • Verify that this does not break RxUI View bindings that might rely on the strong reference.

@slodge
Copy link
Contributor

slodge commented Mar 26, 2014

Be really careful with this on WP - it seems to use new hooks into ICommand.CanExecuteChanged - which means that a weak event manager on WP can't be used for CanExecuteChanged

For info including repro see MvvmCross/MvvmCross#623 - with our "fix" on MvvmCross/MvvmCross@f1ebeb5 - we use strong CanExecuteChanged event subscription for WP only

@jlaanstra
Copy link
Member Author

Be really careful with this on WP - it seems to use new hooks into ICommand.CanExecuteChanged - which means that a weak event manager on WP can't be used for CanExecuteChanged

I have seen that issue and verified that this change does not have the same problem and works on WP8. I believe this has to do with the fact that I store the EventHandler in a ConditionalWeakTable, which keeps a reference to the EventHandler alive as long as the Button also lives.

Conflicts:
	ReactiveUI.Platforms/Cocoa/ReactiveCollectionViewSource.cs
@anaisbetts
Copy link
Member

This PR makes RaiseAndSetIfChanged internal, that Seems Weird. And by 'weird' I mean 'breaks a lot of code'

@jlaanstra
Copy link
Member Author

This PR makes RaiseAndSetIfChanged internal, that Seems Weird. And by 'weird' I mean 'breaks a lot of code'

It is Weird. Totally missed that this was also defined in the same class. Let me fix that.

@jlaanstra
Copy link
Member Author

@paulcbetts thanks for the style fixes. I was planning on doing them :)

@anaisbetts
Copy link
Member

This is looking good, excited to get this into GitHub for Windows - lemme verify it one more time and we'll get it merged

anaisbetts pushed a commit that referenced this pull request Mar 27, 2014
@anaisbetts anaisbetts merged commit 2b37d54 into rxui6-master Mar 27, 2014
@anaisbetts anaisbetts deleted the weak-references branch March 27, 2014 20:01
@anaisbetts
Copy link
Member

Thanks @jlaanstra! This is really great.

@jlaanstra
Copy link
Member Author

This is looking good, excited to get this into GitHub for Windows

I agree. This solves lots of memory issues which are hard to find.

I also pushed some tests to master since this was already merged.

@anaisbetts
Copy link
Member

I also pushed some tests to master since this was already merged.

Sorry about that

@patroza
Copy link
Contributor

patroza commented Jul 12, 2014

I am afraid this is causing issues with WPF's PropertyChangedWeakEventManager (and any other 'client' that implements WeakEvent'ing)

#661 (comment)

@bratsche
Copy link
Contributor

@jlaanstra Why does ReactiveList need to use the event manager here? It seems like it should only be needed in the controls and view controllers. Other containers like ObservableCollection don't do this, so I'm trying to understand why ReactiveList needs to.

This seems to be breaking Xamarin.Forms ListViews that bind to ReactiveList since Xamarin.Forms already uses an event manager.

#806

@lock lock bot locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants