DispatcherTimer should implement the IDisposable interface

[odrouin]

We've learned the hard way that if Stop() is not called on DispatcherTimer instances, once they are started, they continue to live forever, even if nothing references the instance anymore.

After looking at the implementation of DispatcherTimer, we noticed that when calling Start(), a reference to the DispatcherTimer is added to the Dispatcher.CurrentDispatcher (which is static) and the reference is only removed once the Stop() method is called.
Now knowing that Stop() must always be called in order to avoid memory leaks, shouldn't the class implement IDisposable and make sure to call Stop() in the Dispose()? This would indicate the consumer of the class that it needs to be disposed for resources to be released.

Same way System.Threading.Timer implements IDisposable, I think DispatcherTimer should as well.

[miloush]

That would be break anyone relying on the timer running without keeping a reference to it.

[miloush]

It is also by design and documented:

A DispatcherTimer will keep an object alive whenever the object's methods are bound to the timer.

Edit: My bad, this refers to the event handlers I believe.

[odrouin]

That would be break anyone relying on the timer running without keeping a reference to it.

Well, this wouldn't break it right? As long as they don't call Dispose() on it, it would keep the same behavior as before.
Actually, I may be thinking of 2 possible consequences:
1- Breaking change: if the DispatcherTimer was kept in a collection of items and that at some point, Dispose() is called on every IDisposable items of the collection. The suggested change would indeed change the behavior of the code.
2- I guess some analysis rules may be triggered after this change for some users, but I see this as a good thing as it would make them think twice if they really want to keep the DispatcherTimer running even after it is not referenced anymore.

But you are right, because of the possibility of breaking existing code, I guess this is something that is not possible to change.

[miloush]

I didn't looked for numbers but I would imagine a lot of people create the DispatcherTimer in the constructor/on load, start it and not store it anywhere. We have run into the same issue in .NET Gadgeteer, where users would start our timer this way and then it magically stopped working when it got GCed.

I also think System.Timers.Timer does not get collected.

Your scenario seems more unusual to me. You created a timer, run it and then expected it to stop at some undefined point in the future? What is the use case?

[ThomasGoulet73]

It seems like when you start a DispatcherTimer, it is added to Dispatcher. The dispatcher won't get GCed so the DispatcherTimer won't get GCed either. The solution could be to only store weak reference of DispatcherTimer in Dispatcher but this could break existing code.

[miloush]

We could certainly document it though.

[lindexi]

Reference: #11022

[trebahl]

Your scenario seems more unusual to me. You created a timer, run it and then expected it to stop at some undefined point in the future? What is the use case?

I don't think there was such a conscious expectation from the OP, just an oversight due to the lack of IDisposable that would have drawn their attention. You create your timer where you need it, forget to manage its lifetime, then consumers (e.g. a test suite) create lots of instance of your component and you're dead. It's a very easy mistake to make; otherwise c++ wouldn't have been created.

I also think System.Timers.Timer does not get collected.

True but anyway it would only be relevant if you added a finalizer calling dispose, which is a horrifying idea as it wouldn't be called on the right thread. So I fail to see how any code would be broken (apart from code relying on the fact that casting a timer to idisposable should fail but this is https://xkcd.com/1172/ territory).