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

Preventing deadlock on AppDomain.Unload by removing superfluous finalization logic which uses Dispatcher. #1584

Merged
merged 2 commits into from
Oct 17, 2014

Conversation

BrunoJuchli
Copy link
Contributor

The FlipView and the MetroTabItem contain finalizers which don't clean-up unmanaged resources. All they do is deregister a local handler from a local event, which is quite superfluous. What's more, is that the deregistration was done through the dispatcher. This can lead to a deadlock when unloading an AppDomain (btw., the deadlock is resolved. After a while AppDomain.Unload(..) throws an exception stating that it can't unload the domain).

The FlipView finalizer was originally introduced by commit b5aac16.
As far as i can tell the finalizer is not related to the issue which was handled by said commit Fixed a bug where the FlipView was unloaded by the TabControl but it`s inner events weren't reattached when the Tabitem was switched back

…ter the finalizer is executed neither will the event occur ever again nor can the handler hinder garbage collection.

Furthermore the usage of the Dispatcher can proof dangerous if used when doing AppDomain.Unload (possible deadlock?!)
@BrunoJuchli
Copy link
Contributor Author

I've also looked at all other finalizers in the project. They all clean up unamanged resources except for NumericUpDown.cs. There the finalizer is doing

DataObject.RemovePastingHandler(_valueTextBox, OnValueTextBoxPaste);

(Note: The Pasting handler is attached in the OnApplyTemplate method)
Couldn't that be moved to the/an Unloaded event handler instead of the finalizer?
The finalizer is for freeing unmanaged resources.

@flagbug
Copy link
Member

flagbug commented Oct 17, 2014

Yeah, you're probably right, no idea why there's stuff in the finalizers

@flagbug flagbug added this to the v1.0 milestone Oct 17, 2014
@BrunoJuchli
Copy link
Contributor Author

To be honest i don't know WPF very well. I'm a bit uncomfortable with manually regression testing NumericUpDown and that's why I've refrained from altering it, too. Shall i create an issue for that, so that someone can look into whether RemovePastingHandler should go into an Unloaded event handler or some other place? I could just do it and create a pull request, but no warranties that it's still working, then ;-)

@flagbug
Copy link
Member

flagbug commented Oct 17, 2014

Yeah, can you create an issue so that we can keep track of the NumericUpDown thing?
You're also certainly welcome to create a pull request for reviewing, we don't have to merge it 😄

I'll merge this PR right now

flagbug added a commit that referenced this pull request Oct 17, 2014
Preventing deadlock on AppDomain.Unload by removing superfluous finalization logic which uses Dispatcher.
@flagbug flagbug merged commit eb39871 into MahApps:master Oct 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants