ReactiveCommand can lead to memory leaks!

[StanislawSwierc]

Hi Paul,
I noticed a place where you can have memory leaks and tried to fix it. I thought that implementing IDisposable will solve the problem. Unfortunately it didn't help. I assume it's something related to your caching (momoization).

You can find Leak project in my branch:
git@github.com:StanislawSwierc/ReactiveUI.git

[anaisbetts]

Does this repro if your canExecute isn't a Timer? I suspect a circular reference is going on with the timer, which will constantly have a reference because it queues a WorkItem. Since the Timer will only go away when the ReactiveCommand is GC'd and the ReactiveCommand has an indirect reference to the Timer, it stays around forever. Try with canExecute = Observable.Return(true).

[StanislawSwierc]

With canExecute = Observable.Return(true) I can also see the leak.

[anaisbetts]

This isn't a bug - the reason the memory leaks is because your while(true) {} is in the OnStartup, so we never actually pump messages, or more importantly, run the Dispatcher queue items. Every instance of MainWindowViewModel ends up queuing a dispatcher item, which just sits there in the queue (which then has a ref to the ViewModel).

Change the code to this and it doesn't repro (the CPU is pinged at 100%, but the memory usage is constant):
https://gist.github.com/1011601

That's a super-obscure reason why the leak is happening, thanks for reporting it anyways!

[StanislawSwierc]

Thanks Paul for finding out what the real cause was. However this does not eliminate my first concern related to ReactiveCommand. I created this Leak project just to highlight the problem of not disposing subscription to observable which is passed in constructor. Some IObservables like Observable.Timer<> require you to remove subscription. Otherwise they are active all the time.
I think ReactiveCommand should either implement IObserver or IDisposable. I've chosen the latter and implemented it in my branch at:
https://github.com/StanislawSwierc/ReactiveUI/blob/fb4ebe856fc85f0ab5ba8235872d920880f9394b/ReactiveUI.Xaml/ReactiveCommand.cs

Maybe you could choose the option you think fits better and implement it. It's bed practice to ignore Disposable pattern.

[anaisbetts]

"It's bed practice to ignore Disposable pattern."

So in general, you're right - however, Rx is a little different; they're not using IDisposable the same way that the Fx folks use it. The IDisposable returned is just a token - some way to say "I want to unsubscribe" that happens to have some extra language support. The Rx IDisposables don't implement a Finalizer for example. You don't have to Dispose every Subscription when using Rx, nor do you have to keep the IDisposable at all when you're a client (and since the object doesn't actually represent an unmanaged resource, the GC will still handle everything).

The one exception to this is when you're implementing a method that returns IObservable, or an operator. In that case, you want to disconnect your "parent" (i.e. your source Observable) as soon as possible - you can see this in action at https://github.com/xpaulbettsx/ReactiveUI/blob/master/ReactiveUI/ObservedChangedMixin.cs#L159

http://stackoverflow.com/questions/5227823/keeping-references-to-idisposable-when-using-the-reactive-extensions-for-net gives more info about this as well as http://blog.aggregatedintelligence.com/2010/10/idisposable-and-finalizers.html

And philosophically speaking, many of the Observables you're constructing in RxUI are kind of the equivalent of Bindings, you don't want them to detach early (with the exception of things in ItemsControls).

[StanislawSwierc]

Thanks for pointing me to some useful materials. Now I understand that I don't need to save IDisposable I get from Subscribe methods. Nevertheless, there is slight difference between 'don't need' and 'can't'. Imagine a situation when you periodically poll a database/service to check if you can execute some operation. You would probably use Observable.Timer. Unfortunately this Observable gets attached to Scheduler and if you don't remove subscription (Dispose) it won't be garbage collected. Moreover, it will hold a reference to your ReactiveCommand and its object graph.

This for example results in memory leak:
new ReactiveCommand(Observable.Timer(TimeSpan.Zero, TimeSpan.FromMinutes(1)).Select(l => true));

I don't claim that you have to dispose everything but it would be nice if ReactiveUI users had choice.

[anaisbetts]

"I don't claim that you have to dispose everything but it would be nice if ReactiveUI users had choice."

I agree - RxUI was written as I myself was learning Rx, so many pllaces don't have the best development practices. I'm going to make a pass through the code to fix these kinds of bugs. Great discussion!