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

Transient views registered with DialogServiceViews #122

Closed
peter-durrant opened this issue Aug 25, 2020 · 7 comments
Closed

Transient views registered with DialogServiceViews #122

peter-durrant opened this issue Aug 25, 2020 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@peter-durrant
Copy link

Transient views registered with DialogServiceViews
I have a fixed length list of items to display where new data is added to the list and the oldest is removed. Each item is represented by a view model and a corresponding view. Each item can launch a dialog showing a larger version of the same data. The view for each item is registered DialogServiceViews.IsRegistered="True" and the DialogService is accessible by each view model so that a dialog can be shown if required.

When an item is removed from the list as new data arrives, the event handler that was added in DialogServiceViews.Register - owner.Closed += OwnerClosed; - is not unsubscribed.

We have identified that this causes a memory leak.

In our case, the owner window is the main application window. This may stay active for long periods of time - possibly months or years - so we need to avoid an ever growing list of OwnerClosed subscriptions.

(When an item is unloaded we have considered calling DialogServiceViews.SetIsRegistered(this, false); from the item view but this won't fix the issue.)

Expected behavior
Only necessary invocations should be maintained.

Demo project
Demo code available at https://github.com/peter-durrant/MvvmDialogsOwnerClosed

Desktop:

  • OS: Windows 10 (Windows 7 SP1 - Windows 10 supported)
  • .NET version: 4.6.1
@peter-durrant peter-durrant added the bug Something isn't working label Aug 25, 2020
@github-actions
Copy link
Contributor

Hi there and welcome to this repository!

A maintainer will be with you shortly, but first and foremost I would like to thank you for taking the time to report this issue. Quality is of the highest priority for us, and we would never release anything with known defects. We aim to do our best but unfortunately you are here because you encountered something we didn't expect. Lets see if we can figure out what went wrong and provide a remedy for it.

@FantasticFiasco
Copy link
Owner

Hi @peter-durrant!

Thanks for providing a sample application. I'll clone it and se what's what. I'll get back to you when I know more about the possible solutions.

Thanks for reporting this!

@FantasticFiasco
Copy link
Owner

I might have found the issue, given I understood your problem. Could you please update your sample to use v7.1.1-beta.1 and tell me whether this version works for you.

Another note. I don't think you are required to manually unregister the view in ItemView.xaml.cs. Could you try removing this code and see whether you experience any memory leaks? My guess is that the views will stay in memory a short time after being removed from the list, but when the garbage collector kicks in they'll be removed.

@peter-durrant
Copy link
Author

Thank you for the speedy response.

Testing with my demo application demonstrated the fix. We have now updated one of our applications and have performed some preliminary memory profiling which has been successful too.

We are performing a memory test overnight tonight just to measure the full impact of the change.

@FantasticFiasco
Copy link
Owner

Perfect! Let me know of the results, and if they are positive I'll officially release the fix.

@peter-durrant
Copy link
Author

Our memory test has completed and we're not seeing a repeat of the memory leak. Great work, thanks!

FantasticFiasco added a commit that referenced this issue Aug 27, 2020
Fix memory leak where Window.Closed events never where un-registered.

Closes #122
@FantasticFiasco
Copy link
Owner

v7.1.1 has now been published to nuget.org. Thank you so much for the sample you provided, it really is the reason as to why this issue was fixed so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants