Memory Leak in BusyAnimation for CslaLight

cds posted on Friday, October 09, 2009

Hi Rocky/all

I've spent the day trying to find a memory leak in my CslaLight application (currently using 3.7.0). I've spent time taking memory dumps and running these through windbg (a serious learning curve!) to isolate the problem.

Basically I have a number of screens in my application which are selected into view by the user clicking on a button. The Page is instantiated and set as the child of a content control on the main page.

What was happening was that the previous page was being disconnected from the visual tree, so this should have caused the GC to release its memory. But the memory kept growing.

In order to test this, I put a destructor method on my page class to do a Debug.WriteLine of a message when it got GCed. This was never executing, meaning that my page was staying in memory for ever.

By taking a dump and looking at the heap in windbg I discovered that the DispatcherTimer in the BusyAnimation class was holding a reference to the page via its event handler. This reference was causing the page not to be collected.

For proof of this, I commented out the CSLA PropertyStatus control (which internally uses a BusyAnimation) on my page and then it got collected fine and my debug message got written from the destructor.

Now, I've looked at the code in the CslaLight BusyAnimation class and it looks fine to me. The DispatcherTimer is contructed and held in a private member of the BusyAnimation class so, in theory, it should be eligible for collection when the BusyAnimation is dereferenced, etc. But it doesn't do this.

I decided to try some experiments with the CSLA source code - I commented out the creating of the DispatcherTimer in the BusyAnimation class and then the GC worked fine.

But, not wanting to lose the benefits of using the PropertyStatus control, etc. I decided the only way to fix this was to add a public method to the BusyAnimation class which I manually call when I am closing my page - this calls StopTimer() and sets the _timer to null. To achieve this, I use the VisualTreeHelper to find all BusyAnimation controls on my page and call the StopTimer() method on them. When I do this, the page gets GCed.

So, why is this happening? The only thing I can think of is that the DispatcherTimer is somehow special and that the event handler, presumeably because it executes on another thread, is somehow maintained as a global reference and having the _timer reference go out of scope is not enough to cause it to be GCed.

Anyway, as I outlined above, I have a solution for now, but obviously it's not ideal.

Is anybody else seeing this problem, or am I seriously out of whack? :)

Craig

RockfordLhotka replied on Friday, October 09, 2009

That is certainly unexpected!

The first possibility that comes to mind is that BusyAnimation is handling an event on the timer. This means that the timer has a reference to the BA control.

Suppose that Microsoft implemented the timer as a singleton. If that were true, there'd be one timer for the whole app, and all its event references would come from that one instance of the timer. And that timer would never go away.

If that's true, then any code handing a timer event would have to release the event (or tell the timer to release the event) or it would never get GCed.

cds replied on Monday, October 12, 2009

Hi Rocky

Yes, it certainly is unexpected. I want to get to the bottom of this though.

Over the weekend I tried building a very simple test of this with just a simple child control with a DispatcherTimer running on it - and it worked fine - i.e. I could detact the control and remove my references to it and it would be GCed.

So, obviuosly it's something more subtle. I will persevere though - unfortunately I've been dragged off onto another project for a few days.

Craig

pondosinat replied on Tuesday, October 13, 2009

This thread is enlightening. I've been dealing with a memory leak issue for months on a SL app that I have put off investigating. The app starts out great but gets slower and slower as more forms get opened. The app uses the BusyAnimation control heavily, so there may be a correlation to this issue.

I was dreading the process of converting the app to WPF and going through the memory profiling process. Now maybe I can forgo that by seeing if this fix addresses the leak. If I have time I'll run some tests and post my findings...

cds replied on Tuesday, October 13, 2009

From what I leaned about WinDbg last week (much of which is quickly evaporating from my mind, sadly!) you can debug SL apps without having to convert to WPF. You can take a dump of IE then .load sos (from the SL runtime) and dump the heap and trace things that way.

It's fairly laborious though :(

Craig

pondosinat replied on Tuesday, October 13, 2009

Thanks for the tip! I thought WPF was the only way. I think I'll try your workaround first to see if I can get away with a quick fix. Either way, that's useful to know for the future.

RockfordLhotka replied on Tuesday, October 13, 2009

cds:

By taking a dump and looking at the heap in windbg I discovered that the DispatcherTimer in the BusyAnimation class was holding a reference to the page via its event handler. This reference was causing the page not to be collected.

Being precise here, are you really saying that the DispatchTimer has a reference to the page? Or does it have a reference to the BusyAnimation, which has a reference to the PropertyStatus, which has a reference to the page?

Also, in normal usage you'd expect that IsBusy would be set to false, which would set IsRunning to false, which would call StopTimer() - is this not happening?

cds replied on Wednesday, October 14, 2009

Yes, to be precise, it was a more tortuous path, and from memory that sounds about right. I don't have the time to look at it in detail at the moment (not the least because I'm about to head off to bed!).

I don't believe the timer was in a running state though - it's just that calling StopTimer disconnects the event handler and sets the reference to null. So, I agree by your logic the timer shouldn't have been running, and shouldn't have had the event handler connected.

I do want to experiment with this some more, but at this stage I won't have time to do so until next week.

Cheers...

Craig

cds replied on Monday, October 19, 2009

I've had a little more time to look at this again, and I've found there definitely is a problem with DispatcherTimer in general i.e it's not something specific to CSLA, but it's just that the BusyAnimation makes heavy use of DispatcherTimer.

I've built an extremely simple project - I have a page containing a ContentPresenter and I can dynamically load a UserControl with a DispatcherTimer on it as the Content of the ContentPresenter. And I can unload it at will with a button click.

So, I have:

ContentPresenter -> ChildControl - dynamically created.

The ChildControl has a DispatcherTimer which I can start.

When I unload the ChildControl by ContentPresenter.Content = null the DispatcherTimer keeps on firing its Tick event. I have no references to the ChildControl, so I expect it to be GC'ed, but it never does, and the DispatcherTimer is just a non-static reference on the ChildControl.

I have button on my page that calls GC.Collect(); GC.WaitForPendingFinalizers();

So, I'm forcing a GC, but still the timer keeps on ticking. And I suspect that because the DispatcherTimer is alive, then the ChildControl is alive too, which is the main source of my memory leak.

Finally, if I manually stop the timer, then the GC does happen.

So, I have to think about what to do about this...

sergeyb replied on Monday, October 19, 2009

BusyAnimation does stop the timer when IsRunning set to false though. Sounds a bit strange that this will still cause an issue...

Sergey Barskiy
Principal Consultant
office: 678.405.0687 | mobile: 404.388.1899

Microsoft Worldwide Partner of the Year | Custom Development Solutions, Technical Innovation


-----Original Message-----
From: cds [mailto:cslanet@lhotka.net]
Sent: Monday, October 19, 2009 6:08 AM
To: Sergey Barskiy
Subject: Re: [CSLA .NET] Memory Leak in BusyAnimation for CslaLight

I've had a little more time to look at this again, and I've found there definitely is a problem with DispatcherTimer in general i.e it's not something specific to CSLA, but it's just that the BusyAnimation makes heavy use of DispatcherTimer.

I've built an extremely simple project - I have a page containing a ContentPresenter and I can dynamically load a UserControl with a DispatcherTimer on it as the Content of the ContentPresenter. And I can unload it at will with a button click.

So, I have:

ContentPresenter -> ChildControl - dynamically created.

The ChildControl has a DispatcherTimer which I can start.

When I unload the ChildControl by ContentPresenter.Content = null the DispatcherTimer keeps on firing its Tick event. I have no references to the ChildControl, so I expect it to be GC'ed, but it never does, and the DispatcherTimer is just a non-static reference on the ChildControl.

I have button on my page that calls GC.Collect(); GC.WaitForPendingFinalizers();

So, I'm forcing a GC, but still the timer keeps on ticking. And I suspect that because the DispatcherTimer is alive, then the ChildControl is alive too, which is the main source of my memory leak.

Finally, if I manually stop the timer, then the GC does happen.

So, I have to think about what to do about this...

RockfordLhotka replied on Monday, October 19, 2009

That's the odd thing Sergey, especially given that the workaround noted
earlier in this thread was to loop through all controls on the form to find
any BusyAnimation controls and explicitly call the StopTimer() method.

It seems like there must be some edge case (though a common one?) where
IsRunning doesn't get set to false, thus leaving the timer active.

pondosinat replied on Monday, October 19, 2009

I also was able to confirm that removing BusyAnimation from my project resolved some noticeable memory leaking. I was referencing it in various forms and search results type controls in my app. I feel I was using it in a pretty typical way though - just binding IsRunning to the IsBusy property on my CslaDataProvider controls. If I get time I'll see if I can troubleshoot for a possible fix.

As an aside, I'm glad this was brought up as it also prompted me to identify another memory leak problem in an AgDataGrid control I'm using...

cds replied on Monday, October 19, 2009

Snap!

I'm also using the AgDataGrid control on a couple of my pages and found there's a memory leak in it too - from memory it attaches to a JavaScript event to do with the mouse wheel, and if you don't disconnect that when you close the page then the page stays in memory.

cds replied on Monday, October 19, 2009

Hi Rocky/Sergy

You're right in that when the timer is stopped in the BusyAnimation control it does detach the timer.

But, I've found that the timer starts running in various places - specifically the StepInterval dependency property calls StartTimer() when it's changed, which seems like a bug to me. (The timer starts running, and keeps running...)

I need to play with the code some more - I've hacked around with the BusyAnimation code so much now.

I think the work-around I posted earlier isn't a total solution though - at least it doesn't completely solve my memory leak (which is BAD - running my app intensively for 1/2 hour or so easily eats up 500MB!)

From the experiments I did last night, I think the solution might be to listen for the LayoutUpdated event, then check the visual tree to see if it's rooted (i.e. is the ultimate parent of the visual tree that the BusyAnimation control is on Application.Current.RootVisual - if not, stop the timer and disconnect the reference to it.) Anyway, I'm going to experiment with this some more and see if that's the solution.

sergeyb replied on Thursday, October 22, 2009

I think I may have found a culprit. Busy animation starts the timer in Loaded event. I think it should never do this, instead just go to normal state there.
Craig,
Could you replace
Loaded += (o, e) =>
{
ArrangeParts();
StartTimer();
};

with
Loaded += (o, e) =>
{
ArrangeParts();
GoToState(true);
};

And see if your memory leaks go away?

Thank you.

Sergey Barskiy
Principal Consultant
office: 678.405.0687 | mobile: 404.388.1899

Microsoft Worldwide Partner of the Year | Custom Development Solutions, Technical Innovation

-----Original Message-----
From: cds [mailto:cslanet@lhotka.net]
Sent: Monday, October 19, 2009 2:56 PM
To: Sergey Barskiy
Subject: Re: [CSLA .NET] RE: Memory Leak in BusyAnimation for CslaLight

Hi Rocky/Sergy

You're right in that when the timer is stopped in the BusyAnimation control it does detach the timer.

But, I've found that the timer starts running in various places - specifically the StepInterval dependency property calls StartTimer() when it's changed, which seems like a bug to me. (The timer starts running, and keeps running...)

I need to play with the code some more - I've hacked around with the BusyAnimation code so much now.

I think the work-around I posted earlier isn't a total solution though - at least it doesn't completely solve my memory leak (which is BAD - running my app intensively for 1/2 hour or so easily eats up 500MB!)

>From the experiments I did last night, I think the solution might be to listen for the LayoutUpdated event, then check the visual tree to see if it's rooted (i.e. is the ultimate parent of the visual tree that the BusyAnimation control is on Application.Current.RootVisual - if not, stop the timer and disconnect the reference to it.) Anyway, I'm going to experiment with this some more and see if that's the solution.