Generic function to handle disposing IDisposable objects.

Question

I am working on a class that deals with a lot of Sql objects - Connection, Command, DataAdapter, CommandBuilder, etc. There are multiple instances where we have code like this:

if( command != null )
{
    command.Dispose();
}

if( dataAdapter != null )
{
    dataAdapter.Dispose();
}

etc

I know this is fairly insufficient in terms of duplication, but it has started smelling. The reason why I think it smells is because in some instances the object is also set to null.

if( command != null )
{
    command.Dispose();
    command = null;
}

I would love to get rid of the duplication if possible. I have come up with this generic method to dispose of an object and set it to null.

private void DisposeObject<TDisposable>( ref TDisposable disposableObject )
    where TDisposable : class, IDisposable
{
    if( disposableObject != null )
    {
        disposableObject.Dispose();
        disposableObject = null;
    }
}

My questions are...

1. Is this generic function a bad idea?
2. Is it necessary to set the object to null?

EDIT:

I am aware of the using statement, however I cannot always use it because I have some member variables that need to persist longer than one call. For example the connection and transaction objects.

Thanks!

Answer 1

You should implement IDisposable in the class that owns these fields. See my blog post on the subject. If this doesn't work well, then the class is not following OOP principles, and needs to be refactored.

It is not necessary to set variables to null after disposing them.

Answer 2

You should consider if you can use the using statement.

using (SqlCommand command = ...)
{
    // ...
}

This ensures that Dispose is called on the command object when control leaves the scope of the using. This has a number of advantages over writing the clean up code yourself as you have done:

• It's more concise.
• The variable will never be set to null - it is declared and initialized in one statement.
• The variable goes out of scope when the object is disposed so you reduce the risk of accidentally trying to access a disposed resource.
• It is exception safe.
• If you nest using statements then the resources are naturally disposed in the correct order (the reverse order that they were created in).

Is it necessary to set the object to null?

It is not usually necessary to set variables to null when you have finished using them. The important thing is that you call Dispose when you have finished using the resource. If you use the above pattern, it is not only unnecessary to set the variable to null - it will give a compile error:

Cannot assign to 'c' because it is a 'using variable'

One thing to note is that using only works if an object is acquired and disposed in the same method call. You cannot use this pattern is if your resources need to stay alive for the duration of more than one method call. In this case you might want to make your class implement IDisposable and make sure the resources are cleaned up when your Dispose method is called. I this case you will need code like you have written. Setting variables to null is not wrong in this case but it is not important because the garbage collector will clean up the memory correctly anyway. The important thing is to make sure all the resources you own are disposed when your dispose method is called, and you are doing that.

A couple of implementation details:

• You should ensure that if your Dispose is called twice that it does not throw an exception. Your utility function handles this case correctly.
• You should ensure that the relevant methods on your object raise an ObjectDisposedException if your object has already been disposed.

Answer 3

Why don't you use using C# construct? http://msdn.microsoft.com/en-us/library/yh598w02.aspx Setting to null if not required.

Answer 4

Others have recommended the using construct, which I also recommend. However, I'd like to point out that, even if you do need a utility method, it's completely unnecessary to make it generic in the way that you've done. Simply declare your method to take an IDisposable:

private static void DisposeObject( ref IDisposable disposableObject )
{
    if( disposableObject != null )
    {
        disposableObject.Dispose();
        disposableObject = null;
    }
}

Answer 5

I assume these are fields and not local variables, hence why the using keyword doesn't make sense.

Is this generic function a bad idea?

I think it's a good idea, and I've used a similar function a few times; +1 for making it generic.

Is it necessary to set the object to null?

Technically an object should allow multiple calls to its Dispose method. (For instance, this happens if an object is resurrected during finalization.) In practice, it's up to you whether you trust the authors of these classes or whether you want to code defensively. Personally, I check for null, then set references to null afterwards.

Edit: If this code is inside your own object's Dispose method then failing to set references to null won't leak memory. Instead, it's handy as a defence against double disposal.

Answer 6

I'm going to assume that you are creating the resource in one method, disposing it in another, and using it in one or more others, making the using statement useless for you.

In which case, your method is perfectly fine.

As far the second part of your question ("Is setting it to null necessary?"), the simple answer is "No, but it's not hurting anything".

Most objects hold one resource -- memory, which the Garbage collection deals with freeing, so we don't have to worry about it. Some hold some other resource as well : a file handle, a database connection, etc. For the second category, we must implement IDisposable, to free up that other resource.

Once the Dispose method is called, both categories because the same : they are holding memory. In this case, we can just let the variable go out of scope, dropping the reference to the memory, and allowing to GC to free it eventually -- Or we can force the issue, by setting the variable to null, and explicitly dropping the reference to the memory. We still have to wait until the GC kicks in for the memory to actually be freed, and more than likely the variable will go out of scope anyway, moments after to set it to null, so in the vast majority of cases, it will have no effect at all, but in a few rare cases, it will allow the memory to be freed a few seconds earlier.

However, if your specific case, where you are checking for null to see if you should call Dispose at all, you probably should set it to null, if there is a chance you might call Dispose() twice.

Answer 7

You never need to set variables to null. The whole point of IDisposable.Dispose is to get the object into a state whereby it can hang around in memory harmlessly until the GC finalises it, so you just "dispose and forget".

I'm curious as to why you think you can't use the using statement. Creating an object in one method and disposing it in a different method is a really big code smell in my book, because you soon lose track of what's open where. Better to refactor the code like this:

using(var xxx = whatever()) {
    LotsOfProcessing(xxx);
    EvenMoreProcessing(xxx);
    NowUseItAgain(xxx);
}

I'm sure there is a standard pattern name for this, but I just call it "destroy everything you create, but nothing else".

Answer 8

If you're objects have a lot of cleanup to do, they might want to track what needs to be deleted in a separate list of disposables, and handle all of them at once. Then at teardown it doesn't need to remember everything that needs to be disposed (nor does it need to check for null, it just looks in the list).

This probably doesn't build, but for expanatory purposes, you could include a RecycleBin in your class. Then the class only needs to dispose the bin.

public class RecycleBin : IDisposable
{
    private List<IDisposable> _toDispose = new List<IDisposable>();

    public void RememberToDispose(IDisposable disposable)
    {
        _toDispose.Add(disposable);
    }

    public void Dispose()
    {
        foreach(var d in _toDispose)
            d.Dispose();

        _toDispose.Clear();
    }
}

Answer 9

Given that iDisposable does not include any standard way of determining whether an object has been disposed, I like to set things to null when I dispose of them. To be sure, disposing of an object which has already been disposed is harmless, but it's nice to be able to examine an object in a watch window and tell at a glance which fields have been disposed. It's also nice to be able to have code test to ensure that objects which should have been disposed, are (assuming the code adheres to the convention of nulling out variables when disposing of them, and at no other time).