fix: subscription leakage in ReactiveCommand #1218
Conversation
Remove the unneeded subscription and dispose the right one.
|
Suggested reviewer(s): @kentcb |
|
By the way in addition to the proposed change to fix my issue I've disposed the command when View was deactivated. Without disposing the ViewModel was leaking as before. |
|
Yep, I'm not sure there's a good reason for the dual-subscription here. Tracing back through the history, initially the disposable was created with some callback logic. Then it was moved out into en empty subscription as part of introducing more functionality, before it became the current It really only makes sense to have one subscription here (it seems important). I'll let @kentcb confirm this but i'm 馃憤 on this change... |
|
@shiftkey thank you for review! By the way commits referenced by you contain only one subscription. So the second one was added later. |
|
The subscription which wasn't disposed was added here. |
|
I've committed the test to assure that |
|
I've checked that the previous version is failing this test. |
|
@kentcb ping |
|
@Hinidu am aware, and planning to look at this next week when I'm no longer on holidays. |
|
@kentcb Thanks! Our holidays in Russia will start tomorrow but I will try to respond if you will have any questions. |
|
Definitely an oops, this one. Thanks for chasing it down @Hinidu and for submitting a great PR 馃憤 |
ghuntley
changed the title
|
Thanks for your patience @Hinidu. This has been merged in and is now available from our MyGet alpha channel. We are looking at doing a new release in a couple days time and this will be included within - it will either be |
Hi! I've described my issue today on the slack channel but nobody replies to me so I found the problem myself 馃槃 That's my original message:
[SlackMessage]
Hello everyone! I have a problem: the
ViewModelis leaking if I use any property of theModelinsidecanExecuteargument of anyReactiveCommand. TheModellives for the entire life of the application butViewModelshould be removed when I close its tab. I use RxUI 7.0. The example:If I replace
canDeepCopyvalue byObservable.Return(true)ViewModelis not leaking anymore. I even tried to disposeDeepCopycommand but it didn't give any results. By the way I have another command in thisViewModelwhich doesn't use theModelinside itscanExecuteand this command doesn't cause the leakage of theViewModel.My application is written in WPF.
I can't find any information whether I should dispose reactive commands or not.
https://docs.reactiveui.net/en/design-guidelines/use-this-on-left-of-whenany.html Here I found that there can be the problems if lifetime of the object on which we call
WhenAnyis longer than lifetime of our object. But there is no info about whether it cause the problems if I use such long-life dependency inside the expression argument ofWhenAny.[/SlackMessage]
The actual problem was the leakage of subscription to
canExecute. There are two subscriptions and I don't know why do we need the second one so I just removed it. But the first one which do the actual work is not disposed so I fixed it. I've tested these changes in my project and everything works fine now.