2

I'm using IDisposable along with CompositeDisposable to clean up my Reactive Extension (Rx) Observers in my ViewModels.

What's the proper implementation for IDisposable in my ViewModels in this scenario? Typically, I wouldn't implement a finalizer since there aren't unmanaged objects, but it seems that the finalizer might be useful to catch any missed calls to Dispose and thus ensure that my Observers are always disposed. (My Observable is instantiated for the lifetime of the application, while the ViewModels are not.)

Thanks!

Update:

Some clarification: The need for the call to Dispose is a little different in this case. The Observer in Reactive Extensions uses Dispose to unsubscribe itself from the Observable. In my case, I have a long-lived Observer with many short-lived Observers and thus the Observers won't be GC unless explicitly Disposed due to the Observable->Observer reference. I'm completely new to Reactive Extensions, so I may be mistaken in my understanding.

HolySamosa
  • 9,011
  • 14
  • 69
  • 102

3 Answers3

1

You shouldn't use the finalizer to call Dispose() (or really any method) on other managed objects. Those objects might have already been finalized (since there is no guarantee on the order of finalization: http://msdn.microsoft.com/en-us/library/system.object.finalize(v=vs.100).aspx

Since those objects are themselves are IDisposable, then their finalizer (if they have one) will take care of any work that needs to be done if Dispose() wasn't called on them (assumming they are implemented correctly).

Matt Smith
  • 17,026
  • 7
  • 53
  • 103
  • 1
    The situation is a bit more complicated than that, as I understand it. A holds a reference to B, B holds a reference to C, and C holds a reference back to B. Disposing of A would call a method of B that tells C to release its reference of B. Forgetting to dispose of A means B will never get garbage collected as long as C cannot be. –  Aug 22 '12 at 17:26
  • And calling `Dispose()` more than once should be safe. Better call it one time too many than not at all. – H H Aug 22 '12 at 17:28
  • @hvd, So are you suggesting that in the finalizer of A, it call the method on B that tells C to release its reference of B? That could be error prone since: You don't know the state of B or C (they may be finalized or they may still be in use) and since the finalizer is run on an unspecified thread you now have to introduce thread safety into class B's usage of the reference to C. – Matt Smith Aug 22 '12 at 18:31
  • @HenkHolterman, with proper usage of Dispose, missing a call to Dispose should be fine (for program correctness), because the finalizer should take of what it missed. See http://stackoverflow.com/questions/2101524/is-it-abusive-to-use-idisposable-and-using-as-a-means-for-getting-scoped-beha. – Matt Smith Aug 22 '12 at 18:45
  • @MattSmith I know that will take a whole lot more work, probably too much, to get right. But not doing anything about it isn't an appealing option either. I don't have an answer. –  Aug 22 '12 at 18:49
  • @hvd, I'm not suggesting doing nothing. In the situation you describe (with A, B, and C), I would suggest that the work of disconnecting B from C not be done in the Disposal method of A, but rather just some regular method that is required to be called when you are done using A. Or, if you'd like to do it in Dispose() to allow the use of a using statement, then document to the client of your class that failure to call Dispose() will cause a bug. So, in essense, I'm suggesting that the fix should be on the client side: add in the missing calls to Dispose(). – Matt Smith Aug 22 '12 at 19:00
  • 1
    Indeed, and the question already asks about that: "but it seems that the finalizer might be useful to catch any missed calls to Dispose". Perhaps having a debug-only finalizer that does `Debug.Assert(disposed);` would be a good compromise? –  Aug 22 '12 at 19:27
  • @MattSmith - missing a call to Dispose is not always lethal, just under some conditions (mostly out of your control). That makes that it is always a severe error, never 'fine'. – H H Aug 22 '12 at 19:28
  • @HenkHolterman, regarding "always a severe error, never 'fine'", that's not true. There are cases where you should call Dispose() if it is convienent for you--if it is not convienent just let the finalizer handle it. Prime example: http://stackoverflow.com/questions/3734280/is-it-considered-acceptable-to-not-call-dispose-on-a-tpl-task-object. The intended usage of Dispose is that missing a call to dispose is *never* lethal, but rather just delays the disposal of some finite resource. – Matt Smith Aug 22 '12 at 19:58
  • @hvd: Agreed, using it to catch missed cases of Dispose seems useful. I assumed the OP intended to use it as a safe-guard in production code. – Matt Smith Aug 22 '12 at 20:00
  • @HenkHolterman, Of course--Anyone can write a bad IDisposable class that doesn't follow the correct pattern. – Matt Smith Aug 22 '12 at 20:08
  • @HenkHolterman I don't know of any standard library classes that require disposal for correctness, do you have an example? Sure, they might take longer to dispose of memory, handles, etc. But eventually they will release them. – Matt Smith Aug 22 '12 at 20:13
  • 2
    If the finaliser on the contained objects didn't fix the issue, then the finaliser on the containing object is hardely going to. It's one thing to add a finaliser to catch that it should be called in debug mode, another to just slow down finalisation. – Jon Hanna Aug 22 '12 at 20:15
  • Thanks for the comments, guys! My situation is similar to what @hvd describes in the first comment. Also, the Observer in Reactive Extensions uses Dispose to unsubscribe itself from the Observable. In my case, I have a long-lived Observer with many short-lived Observers and thus (as I understand it) the Observers won't be GC unless explicitly Disposed due to the Observable->Observer reference. – HolySamosa Aug 22 '12 at 21:16
  • 1
    @HenkHolterman, I understand your point, now. Thanks. I thought you were arguing that missing the call to Dispose() would never give up the resources--but your just saying that missing the call increases the delay of giving up the resources and during that time period the program could fail. Good point. – Matt Smith Aug 23 '12 at 13:47
1

You don't need to Dispose Rx IDisposables unless you're explicitly wanting to unsubscribe earlier than normal. Just leak them. They don't represent unmanaged memory, so the CLR will GC them just like any other object. And never implement a finalizer on these objects either.

You also don't have to worry about reference loops, the CLR's GC is smart enough to detect them (just like if you had someA.B = someB; and someB.A = someA)

Ana Betts
  • 73,868
  • 16
  • 141
  • 209
  • I know that I could ignore the call to 'Observer.Dispose' is the Observer and Observable had a similar lifetime, however in my scenario, the Observable persists for the life of the application and the Observers are frequently created and destroyed. In this case, won't the Observable hold references to the Orphaned Observer, preventing garbage collection? (I feel like I read this somewhere). – HolySamosa Aug 22 '12 at 21:05
1

You shouldn't use finalizers to call Dispose() in order to unsubscribe from Rx subscriptions. Finalizers only get called when the object is about to be garbage collected and garbage collection only happens when the object is no longer able to be referenced. If there are current subscriptions then it has references and it won't be garbage collected.

You must call .Dispose() yourself - explicitly - in order to clean up your subscriptions.

Enigmativity
  • 113,464
  • 11
  • 89
  • 172