12

MSDN's example pattern for implementing a Dispose() method depicts setting the reference to a disposed managed resource to null (_resource = null), but does so outside the if (disposing) block:

protected virtual void Dispose(bool disposing)
{
    // If you need thread safety, use a lock around these 
    // operations, as well as in your methods that use the resource.
    if (!_disposed)
    {
        if (disposing) {
            if (_resource != null)
                _resource.Dispose();
                Console.WriteLine("Object disposed.");
        }

        // Indicate that the instance has been disposed.
        _resource = null;
        _disposed = true;   
    }
}

Shouldn't _resource = null be placed inside this code block? If a call to Dispose(false) is made then _resource will be null and unable to be subsequently disposed! ??

Of course, Dispose(false) is only called (in practice) by the runtime during finalization. But if _resource wasn't previously disposed, what is the need to set it to null at this point when the object (including the _resource member field) is about to be garbage collected?


[end of original question]

Follow up:

After much reading, it appears setting the reference to null is not necessary, but may be a good idea for "heavy" member objects if you have reason to believe the containing class (the one being disposed) might not be garbage collected soon.

Know that object disposal is no assurance that the object has been "released" by consuming code. The disposed object might be kept around (in a collection or otherwise) for various purposes, or just in error. I can imagine an application that uses objects from a collection then disposes of them, but keeps them in the collection for a later process to perform removal and log final state (or something like that... who knows...)

Conclusions:

  1. Setting references to "heavy" member objects to null releases them for garbage collection even if the disposed object is not released.
  2. It is overkill to clear references for all objects.
  3. Hence, placement of the _resource = null statement (the original question) is not important for two reasons: (A) Using it at all is only something to think about after reading the above; (B) In the MSDN example, it executes for both Dispose(true) and Dispose(false), but the latter only occurs when the object is finalized and just about to be garbage collected anyway!

Thus, my preference will be to place _resource = null inside the most inner if block:

if (disposing) {
    if (_resource != null) {
        _resource.Dispose();
        _resource = null;
    }
}

This keeps all the _resource code together. Further thoughts, anyone?

More reading:

Community
  • 1
  • 1
Kevin P. Rice
  • 5,550
  • 4
  • 33
  • 39
  • 1
    It serves little purpose, but does little harm... Actually, I'd be more annoyed by a: the writing to `Console`, and b: writing to `Console` even if it is already disposed and we didn't dispose it (see missing braces on the `if`). – Marc Gravell Jul 20 '11 at 05:00
  • Haha! The `Console.Writeline()` is sample code. Would Trace.Writeline() please you better? Also, there is no missing brace--it's on the end of the `if` line (not StyleCop compliant). – Kevin P. Rice Jul 20 '11 at 05:17
  • look again; it writes "Object disposed." even if `_resource` was null and it therefore didn't call `.Dispose()`. I mean the *inner-most* `if`. – Marc Gravell Jul 20 '11 at 05:19
  • A related question here indicates there is no point in setting `_resource = null` (see Igor Zevaka's answer at: http://stackoverflow.com/questions/2926869/c-do-you-need-to-dispose-of-objects-and-set-them-to-null). – Kevin P. Rice Jul 20 '11 at 05:35
  • @Marc Ah, yes! You are correct. However, the "Object" disposed of is `this` not `_resource` so I submit to you that the code is correct as written. – Kevin P. Rice Jul 20 '11 at 05:39

1 Answers1

7

Setting it to null is removing the reference or pointer to the location in the heap. This lets the GC go through and remove anything that has no references to it without it having to do much guesswork. _resource would be some internal use object that needs to have its references cleaned up so lets say you have a Socket internal to your close you would close/dispose of that socket or any other persistent resource. Once it's disposed you set it to null and remove all references (should remove all references) so that GC can do it's job fine. The 2nd half of my answer is an example so it kind of repeats some things but I hope you get the idea.

Setting it to null twice isn't a big deal as there's no effect. Disposing should be true at the time of cleaning up any resources and it's only false once it is (as you said) about to be garbage collected anyway.

Jesus Ramos
  • 22,940
  • 10
  • 58
  • 88
  • Perhaps it doesn't make much difference where it goes? By placing `_resource = null` right after `_resource.Dispose()` the code is easier to read/maintain because these related items are in one place (imagine many items are disposed with many `= null`). – Kevin P. Rice Jul 20 '11 at 04:57
  • Also, `_resource` is a managed resource (probably not a socket). The MSDN pattern puts only managed object disposal inside the `if (disposing)` block. I'm only trying to understand if there is a reason `_resource = null` is not right below `_resource.Dispose()`. – Kevin P. Rice Jul 20 '11 at 05:01
  • Sockets could be managed within your code as being constantly open for some reason (same as a WebService connector or HttpListener) which must be closed properly or you can have a memory leak or some other kind of resource leak (for isntance a port that you used is no longer accessible). Most items don't require managing so you can simply just set them to null and the GC will pick them up on one of its trips. – Jesus Ramos Jul 20 '11 at 05:14
  • Chosen best answer (not thorough in regards to answer sought, but helpful). Thanks. – Kevin P. Rice Jul 20 '11 at 23:09
  • 1
    I would agree with you that it should be placed within the if statement. In most cases I would recommend just implementing only a Dispose() and not bother with that (as you see in one of the articles you posted) this avoids that ambiguity you were talking about. – Jesus Ramos Jul 20 '11 at 23:13
  • 3
    Wow! Epiphany! You just made me realize if there is no finalizer (and there shouldn't be in most cases) that there is no need for `Dispose(bool)`! I've been so focused on "the pattern" I didn't realized `IDisposable` only includes `Dispose()`. Any `disposed` bool flag can be implemented within `Dispose()` and there is no reason to have a `Dispose(bool)` method as well! Thank you! Winner of most valuable comment! – Kevin P. Rice Jul 22 '11 at 02:45
  • Yeah usually there's a Dispose(bool) that by default calls Dispose(true) I believe but there's no point to use it sometimes. Answer and comment MVP I can live with that :P – Jesus Ramos Jul 22 '11 at 02:47
  • 4
    In the full pattern (http://msdn.microsoft.com/en-us/library/b1yfkh5e.aspx) Dispose() calls Dispose(true) signaling disposal of all resources. The finalizer calls Dispose(false) to signal disposal of ONLY unmanaged resources (managed references are NOT SAFE to use by the time finalization occurs). It is also recommended NOT to use a finalizer if there are no unmanaged resources. THEREFORE, if no unmanaged resources exist, you don't need a finalizer OR Dispose(bool); only Dispose() is necessary. [Best Answers usually don't give you what you want, they force you to learn more!] – Kevin P. Rice Jul 22 '11 at 03:14
  • Yeah, it's an odd pattern and kind of confusing at times because it looks like dispose should be a finalizer but it really isn't. – Jesus Ramos Jul 22 '11 at 03:16