Dialog problem in windowmanager

Oct 3, 2010 at 1:45 PM
Edited Oct 3, 2010 at 1:46 PM

Hello,

 

I looked a the showdialog in this thread  http://caliburnmicro.codeplex.com/Thread/View.aspx?ThreadId=225718

This seems to work OK, but if i use it more than once it cracshes on the follwing code (recursive loop) in the windowmanager.cs

deactivatable.Deactivated += (s, e) =>
                {
                  if (e.WasClosed && !deactivatingFromView)
                  {
                    deactivateFromVM = true;
                    actuallyClosing = true;
                    view.Close();
                    actuallyClosing = false;
                    deactivateFromVM = false;
                  }
                };

I changed the code to an event procedure and it seems ok

                deactivatable.Deactivated += deactivatable_Deactivated;

:
:

        void deactivatable_Deactivated(object sender, DeactivationEventArgs e)
        {
          ((IDeactivate) sender).Deactivated -= deactivatable_Deactivated;

          if (e.WasClosed && !deactivatingFromView)
          {
            deactivateFromVM = true;
            actuallyClosing = true;
            view.Close();
            actuallyClosing = false;
            deactivateFromVM = false;
          }

        }

Does this mean the code in windowmanger has a memory leak.

 

Coordinator
Oct 3, 2010 at 2:52 PM

Is it possible for you to send me a solution that demonstrates the problem? I would really appreciate that. I'd like to have a look at it before I apply this fix. Please send it to robertheisenberg at hotmail dot com  Thanks!

Oct 3, 2010 at 4:14 PM
Edited Oct 3, 2010 at 6:00 PM

I think I just hit that one too. Pull my fork. There should be a Test Dialog button. Press it once and press Ok/Cancel => OK, press it again and the next Ok/Cancel will get a StackOverflow.

/Christoffer

Coordinator
Oct 4, 2010 at 5:27 PM

I added a ticket to remind me to look into this later this week. Can you both confirm that the code above fixes the problem?

Oct 4, 2010 at 7:31 PM

Yes, i've enhanced my windowmanager (it has of course some consequeces for variable scoping)

My concern is: do all events coded in this way lead to this behavior and thus memory leaks...

Oct 4, 2010 at 7:49 PM
Edited Oct 4, 2010 at 7:58 PM

Hi,

I've found out the issue. I forgot to tell MEF to see it as Non.Shared thus giving me the same vm. Added

 [PartCreationPolicy(System.ComponentModel.Composition.CreationPolicy.NonShared)]

Now it works like a charm! But if you need a singleton then it's a different story...

Br Christoffer

Oct 4, 2010 at 9:56 PM

Well my first impression would be that it should be a singleton.

If it is nonshared there is a new instance all the time, so you will not notice is but the issue of a memory leak remains.

There are numerous threads about memory leaks. Also mentioned here. And my concerns are: does it have to do with the lamba expressions

But that seems hard to believe. It must be something else, maybe in conjunction with MEF.

Oct 6, 2010 at 10:44 AM

Defining the VM as NonShared should not be the solution. As jkattestaart said, you are probably avoiding the problem getting a new instance every time.

I think that in this case there is no memory leak, tho. A WindowManager reference is stored inside the deactivable when the event is hooked, not the opposite. This means that the deactivable cannot leak (on the contrary, the WindowManager could leak if it's lifetime is shorter than the deactivable). Have a look at this article for more information on the subject.

I could be wrong, but I think the issue is that singletons (or VMs that are created and then shared) get the events registered multiple times, since events are never un-registered. The event procedure jkattestaart suggested fix this, provided that the Deactivate method is called. A safer way would be to use both his fix and a un-register/register approach, like this:

 

deactivatable.Deactivated -= deactivatable_Deactivated;
deactivatable.Deactivated += deactivatable_Deactivated;

//Event handler
void deactivatable_Deactivated(object sender, DeactivationEventArgs e)
{
        ((IDeactivate) sender).Deactivated -= deactivatable_Deactivated;

        if (e.WasClosed && !deactivatingFromView)
        {
                deactivateFromVM = true;
                actuallyClosing = true;
                view.Close();
                actuallyClosing = false;
                deactivateFromVM = false;
        }
}

Note that un-registering the event is perfectly safe even if no event handler has been previously hooked (just like removing an object from a list that does not contain it).

I suppose that the same trick should be used everytime there is no way to determine if the hooking has been already performed (e.g. both view.Closing and view.Closed) in the WindowManager.cs file.

 

On a last note, it is possible to use lambdas and achieve event un-registering in the handler:

 

Window window = new Window();
EventHandler closeHandler = null;
closeHandler = (obj, args) => { ((Window)obj).Closed -= closeHandler; };
window.Closed += closeHandler;

 

Using Re-Sharper there will be a warning, regarding the fact that the lambda is accessing to a modified closure, but it is normal, since it's thanks to such modification that the above code works as intended. Nevertheless, the event handler way is definitely clearer, cleaner and thus preferable, IMHO.

 

Coordinator
Oct 6, 2010 at 12:18 PM

Thanks BladeWise I'm pretty sure your evaluation is correct. I'm going to try to get these fixes in soon.

Oct 6, 2010 at 6:40 PM

Agreed.

Coordinator
Oct 7, 2010 at 2:01 PM

I did some refactoring and checked it in b4366b357d16 Let me know if this fixes the issues you were having.

Oct 8, 2010 at 2:32 PM

Rob , for me it fixes the issues. Thx