New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 馃憤 |
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
ViewModel
is leaking if I use any property of theModel
insidecanExecute
argument of anyReactiveCommand
. TheModel
lives for the entire life of the application butViewModel
should be removed when I close its tab. I use RxUI 7.0. The example:If I replace
canDeepCopy
value byObservable.Return(true)
ViewModel
is not leaking anymore. I even tried to disposeDeepCopy
command but it didn't give any results. By the way I have another command in thisViewModel
which doesn't use theModel
inside itscanExecute
and 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
WhenAny
is 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.