The Code Analysis-friendly way to dispose of objects

Question

As part of our Visual Studio 2010 (primarly C# 4.0) development standards, we have Code Analysis turned on. As I am reviewing recently submitted code for a new project, I am seeing a ton of

CA2000 : Microsoft.Reliability: In method 'XYZ', object 'ABC' is not disposed along all exception paths. Call System.IDisposable.Dispose on object 'ABC' before all references to it are out of scope.

warnings. The problem is that nothing I do seems to eliminate the warnings - and I've spent hours scouring the web and trying everything I can.

First, let me be clear that I am not talking about putting a simple using block in to properly dispose of a local variable - that's not an issue. In my case, these warnings are appearing when the object is either returned by the method or assigned to another object within the method.

Here is a code sample that contains four such warnings:

public void MainMethod()
{
    var object1 = CreateFirstObject();    // Warning here
    var object2 = CreateSecondObject();   // Warning here

    SomeCollectionProperty.Add(object1);
    SomeCollectionProperty.Add(object2);
}

private SomeObject CreateFirstObject()
{
    var theObject = new SomeObject()      // Warning here
    {
        FirstProperty = "some value",
        // ...
    };

    return theObject;
}

private SomeOtherObject CreateSecondObject()
{
    var theObject = new SomeOtherObject() // Warning here
    {
        FirstProperty = "a different value",
        // ...
    };

    return theObject;
}

I've commented the lines where the warnings occur.

I've tried refactoring both Create methods as described in the MSDN article (here) but the warnings still appear.

UPDATE I should note that both SomeObject and SomeOtherObject implement IDisposable.

Also, while object initializers may be a component of the problem, keep in mind that the initializers are isolated to the two private methods and have nothing to do with the warnings in MainMethod.

Can anyone show me how to properly implement these methods to eliminate the CA2000 warnings?

Answer 1

The problem that is being detected by CA2000 in this case is that a disposable instance may be "orphaned" if an exception occurs before it is passed out of the method. For example, a "correct" implementation of CreateFirstObject would look something like the following:

private SomeObject CreateFirstObject()
{
    var theObject = new SomeObject();
    try
    {
        theObject.FirstProperty = "some value";
    }
    catch
    {
        theObject.Dispose();
        throw;
    }

    return theObject;
}

---

Given what you have described concerning your desired behaviour for MainMethod, its "correct" implementation might look something like this:

public void MainMethod()
{
    var object1 = CreateFirstObject();
    try
    {
        SomeCollectionProperty.Add(object1);

        var object2 = CreateSecondObject();
        try
        {
            SomeCollectionProperty.Add(object2);
        }
        catch
        {
            object2.Dispose();
            throw;
        }
    }
    catch
    {
        object1.Dispose();
        SomeCollectionProperty.Remove(object1); // Not supposed to throw if item does not exist in collection.

        throw;
    }
}

Answer 2

One way to get rid of the warning is to suppress it in code:

[SuppressMessage(
    "Microsoft.Reliability",
    "CA2000:DisposeObjectsBeforeLosingScope",
    Justification = "Factory method")]

But this is not a real solution of the problem.

A solution is described here: How to get rid of CA2000 warning when ownership is transferred?

In the mentioned link it is basically stated to add the object to a collection implementing ICollection<T>, but I haven't tested that.

Answer 3

What happens if you wrap the returned objects in main in a using block, or implement a finally to dispose of the objects?

Do the SomeOtherObjects need to implement IDisposable?

Answer 4

What is needed is to implement a pattern similar to a "using" block, but disabling the disposal of the object in the scenario where it will be returned successfully. The approach offered by Nicole Calinoiu is reasonable, though I prefer to avoid catching exceptions that are simply going to bubble up. My preferred expression of the code, given the constraints of the C# language, would be to use an InitializedSuccessfully flag, and then have a "finally" block which takes care of the disposal if InitializedSuccessfully has not been called.

If a class is going to contain many IDIsposable objects, and the set of such objects will be fixed once construction is complete, it may be useful to define an IDisposable manager class which will hold a list of IDisposable objects. Have the constructors of your class accept a DisposableManager object as a parameter, and place all of the objects it constructs into the list generated thereby (it may be helpful for your class to have an instance method:

T regDisposable<T>RegDispose(T newThing) where T:IDisposable
{
  myDisposableManager.Add(newThing);
  return newThing;
}

To use it, after myDisposableManager had been initialized, simply say something like: var someDisposableField = RegDispose(new someDisposableType());. Such a pattern would offer two big benefits:

1. All the main class would have to do in its Dispose implementation would be `if (myDisposableManager != null) myDisposableManager.Dispose();` The act of setting an IDisposable object in the constructor (using RegDispose) would also provide for its cleanup.
2. The code calling the constructor for the main object could, if the constructor throws an exception, call the Dispose method on the DisposableManager object it had created and passed in. This would ensure timely cleanup of the partially-created object, something which is otherwise all but impossible.

In vb, it's possible for a base-class constructor to expose a constructor parameter as a field which is available to field initializers. Thus, one could nicely use the RegDispose pattern in field initializers as well as in the explicit constructor. In C# that is not possible. It is possible to use [threadstatic] fields for that purpose, but some caution would be required to ensure that any such fields that get set also get unset. A constructor gets called from within something like a threadpool thread could generate a memory leak. Also, threadstatic fields cannot be accessed nearly as efficiently as normal ones, and I don't know any way in C# to avoid having to re-fetch the thread-static field many times--once for each IDisposable object registered.

Answer 5

The pattern we use and that clears the warning most of the time is

DisposableObject retVal;
DisposableObject tempVal;

try{
  tempVal = new DisposableObject();
  tempVal.DoStuff();
  retVal = tempVal;
  tempVal = null;
} finally{
  if (tempVal != null) { tempVal.Dispose();} //could also be writtent tempVal?.Dispose();
}

return retVal;

Unfortunately, there are still some cases where the warning won't go so we then delete the warning locally with a justification saying the disposable is covered with pattern.

This is very briefly mentioned on Microsoft Documentation.