views:

309

answers:

4

Apologies for the rather verbose and long-winded post, but this problem's been perplexing me for a few weeks now so I'm posting as much information as I can in order to get this resolved quickly.

We have a WPF UserControl which is being loaded by a 3rd party app. The 3rd party app is a presentation application which loads and unloads controls on a schedule defined by an XML file which is downloaded from a server.

Our control, when it is loaded into the application makes a web request to a web service and uses the data from the response to display some information. We're using an MVVM architecture for the control. The entry point of the control is a method that is implementing an interface exposed by the main app and this is where the control's configuration is set up. This is also where I set the DataContext of our control to our MainViewModel.

The MainViewModel has two other view models as properties and the main UserControl has two child controls. Depending on the data received from the web service, the main UserControl decides which child control to display, e.g. if there is a HTTP error or the data received is not valid, then display child control A, otherwise display child control B. As you'd expect, these two child controls bind two separate view models each of which is a property of MainViewModel.

Now child control B (which is displayed when the data is valid) has a RefreshService property/field. RefreshService is an object that is responsible for updating the model in a number of ways and contains 4 System.Timers.Timers;

  • a _modelRefreshTimer
  • a _viewRefreshTimer
  • a _pageSwitchTimer
  • a _retryFeedRetrievalOnErrorTimer (this is only enabled when something goes wrong with retrieving data).

I should mention at this point that there are two types of data; the first changes every minute, the second changes every few hours. The controls' configuration decides which type we are using/displaying.

  1. If data is of the first type then we update the model quite frequently (every 30 seconds) using the _modelRefreshTimer's events.

  2. If the data is of the second type then we update the model after a longer interval. However, the view still needs to be refreshed every 30 seconds as stale data needs to be removed from the view (hence the _viewRefreshTimer).

The control also paginates the data so we can see more than we can fit on the display area. This works by breaking the data up into Lists and switching the CurrentPage (which is a List) property of the view model to the right List. This is done by handling the _pageSwitchTimer's Elapsed event.

Now the problem

My problem is that the control, when removed from the visual tree doesn't dispose of it's timers. This was first noticed when we started getting an unusually high number of requests on the web server end very soon after deploying this control and found that requests were being made at least once a second! We found that the timers were living on and not stopping hours after the control had been removed from view and that the more timers there were the more requests piled up at the web server.

My first solution was to implement IDisposable for the RefreshService and do some clean up when the control's UnLoaded event was fired. Within the RefreshServices Dispose method I've set Enabled to false for all the timers, then used the Stop() method on all of them. I've then called Dispose() too and set them to null. None of this worked.

After some reading around I found that event handlers may hold references to Timers and prevent them from being disposed and collected. After some more reading and researching I found that the best way around this was to use the Weak Event Pattern. Using this blog and this blog I've managed to work around the shortcomings in the Weak Event pattern.

However, none of this solves the problem. Timers are still not being disabled or stopped (let alone disposed) and web requests are continuing to build up. Mem Profiler tells me that "This type has N instances that are directly rooted by a delegate. This can indicate the delegate has not been properly removed" (where N is the number of instances). As far as I can tell though, all listeners of the Elapsed event for the timers are being removed during the cleanup so I can't understand why the timers continue to run.

Thanks for reading. Eagerly awaiting your suggestions/comments/solutions (if you got this far :-p)

A: 

Call timer.Elapsed -= new ElapsedEventHandler(); when your control is disposed of to manually detach the handler.

MagicWishMonkey
I assume you mean `timer.Elapsed -= MyEventHandler`? Or do I need to create a new ElapsedEventHandler to detach another event handler? I did try manually detaching the event handler using -= originally before I started messing with the Weak Event pattern, but it didn't make any difference.
alimbada
+3  A: 

have you tried using the DispatchTimer instead of System.Timers.Timer?

If a Timer is used in a WPF application, it is worth noting that the Timer runs on a different thread then the user interface (UI) thread. In order to access objects on the user interface (UI) thread, it is necessary to post the operation onto the Dispatcher of the user interface (UI) thread using Invoke or BeginInvoke. Reasons for using a DispatcherTimer opposed to a Timer are that the DispatcherTimer runs on the same thread as the Dispatcher and a DispatcherPriority can be set. ( from this blog )

Muad'Dib
Oh wow. I can't believe I didn't think of this. I've changed all my Timers to DispatcherTimers and I'm profiling now. I can't see any leaks so far. I'm going to leave it running for a while and see that it's fixed for certain and report back. Cheers. :)
alimbada
I'm glad to help!
Muad'Dib
Okay, so using DispatcherTimer fixes the issue with the timers elapsing too often so web requests are being made at the right intervals. However, it doesn't fix the memory leak. The DispatcherTimers are still not being disposed of. However, the leak isn't massive. The app starts up using around 40MB and after 18 hours, it's double that. The machines this app runs on reboots every 24 hours, so I'm not too worried about the leak and I'm accepting this as the answer. However, if anyone has suggestions on fixing it, I'd love to hear them.
alimbada
+1  A: 

It sounds to me like the timer signals are being queued faster than they are processed. For example, this would happen if each elapse event takes 2 seconds to process while the timer elapses every second. This would result in a backlog of "raise Elapse event now" signals being scheduled on the .NET thread pool.

Such a backlog would then continue to cause elapse events even after you stop the timer. From the System.Timers.Timer documentation:

Even if SynchronizingObject is true, Elapsed events can occur after the Dispose or Stop method has been called or after the Enabled() property has been set to false, because the signal to raise the Elapsed event is always queued for execution on a thread pool thread. One way to resolve this race condition is to set a flag that tells the event handler for the Elapsed event to ignore subsequent events.

To avoid a backlog of unprocessed timer signals building up in the thread pool, you can set AutoReset to false. That way the timer disables itself at each elapse. You can then set Enabled back to true after handling the elapse event. That way no additional elapse events will be scheduled until the last one has been handled.

Wim Coenen
A: 

I could not comment the question for some reason... I have the same problem myself. You said "I found that event handlers may hold references to Timers and prevent them from being disposed and collected". Could you give a link to any such resource? I noticed this behavior only with System.Timers.Timer, but not with System.Threading.Timer...

Konstantin
One more thing - it seems like this is a leak from System.Timers.Timer, because it actually just wraps System.Threading.Timer. I even tried to set a different callback method to it's private field "callback". The effect of it remaining in memory has disappeared. I was not doing any serious memory profiling on that though...
Konstantin