Proper Way to Dispose: object not disposed along all exception paths

Question

I get this message for line 84 and line 85 (the two, stacked using lines):

CA2000 : Microsoft.Reliability : In method 'RavenDataAccess.GetRavenDatabase()', object '<>g_initLocal9' is not disposed along all exception paths. Call System.IDisposable.Dispose on object '<>g_initLocal9' before all references to it are out of scope.

DocumentStore implements IDisposable.

Why? How else can I dispose the DocumentStore objects? They're created in a using block, and I dispose of them in my catch block. How should this be fixed?

private static IDocumentStore GetRavenDatabase()
{
    Shards shards = new Shards();

    try
    {
        using (DocumentStore docStore1 = new DocumentStore { Url = ConfigurationManager.AppSettings["RavenShard1"] })  // Line 84
        using (DocumentStore docStore2 = new DocumentStore { Url = ConfigurationManager.AppSettings["RavenShard2"] })  // Line 85
        {
            shards.Add(docStore1);
            shards.Add(docStore2);
        }

        using (ShardedDocumentStore documentStore = new ShardedDocumentStore(new ShardStrategy(), shards))
        {
            documentStore.Initialize();

            IndexCreation.CreateIndexes(typeof(RavenDataAccess).Assembly, documentStore);

            return documentStore;
        }
    }
    catch
    {
        shards.ForEach(docStore => docStore.Dispose());

        throw;
    }
}

Answer 1

You have to ensure that you dispose all your newly created Disposable objects along any possible exception path. See below:

private static IDocumentStore GetRavenDatabase()
{
    Shards shards = new Shards();
    DocumentStore docStore1 = null;
    DocumentStore docStore2 = null;

    ShardedDocumentStore shardedDocumentStore = null;
    ShardedDocumentStore tempShardedDocumentStore = null;

    try
    {
        docStore1 = new DocumentStore();
        docStore1.Url = ConfigurationManager.AppSettings["RavenShard1"];
        docStore2 = new DocumentStore();
        docStore2.Url = ConfigurationManager.AppSettings["RavenShard2"];

        shards.Add(docStore1);
        shards.Add(docStore2);

        tempShardedDocumentStore = new ShardedDocumentStore(new ShardStrategy(), shards);
        tempShardedDocumentStore.Initialize();

        IndexCreation.CreateIndexes(typeof(RavenDataAccess).Assembly, tempShardedDocumentStore);

        docStore1 = null;
        docStore2 = null;

        shardedDocumentStore = tempShardedDocumentStore;
        tempShardedDocumentStore = null;

        return shardedDocumentStore;
    }
    finally
    {
        if (tempShardedDocumentStore != null) { tempShardedDocumentStore.Dispose(); }
        if (docStore1 != null) { docStore1.Dispose(); }
        if (docStore2 != null) { docStore2.Dispose(); }
    }
}

CA seems to have a problem with the inline property initializers but if you break them out this should work. Key thing is to ensure that no matter where an exception is thrown in the try block, all your new objects that can be disposed are cleaned up.

By setting the temp references you no longer need to null (docStore1, docStore2, and tempShardedDocumentStore) just prior to returning, you can check in the finally block to see if they in fact were set to null, if not, an exception occurred somewhere and you can dispose of them before execution leaves this method.

Note docStore1 and docStore2 are temporary references as they are added to the Shards collection.

Answer 2

First of all, the shards you pass into new ShardedDocumentStore() contains disposed docStore1 and docStore2. This will most likely cause problems.

Also, in the catch statement you dispose the docStores that might already be disposed.

Finally, the ShardedDocumentStore that you returned is being disposed (by using) when you return it, probably making it unusable for the caller.

Also, I had a quick look at the ShardedDocumentStore (at GitHub) and I'd say that it takes care of the disposal of its docStores. That is, you shouldn't handle it.

Change your code to this:

private static IDocumentStore GetRavenDatabase()
{
    ShardedDocumentStore documentStore = null;
    var docStore1 = null;
    var docStore2 = null;

    try
    {
        Shards shards = new Shards();
        docStore1 = new DocumentStore { Url = ConfigurationManager.AppSettings["RavenShard1"] };
        shards.Add(docStore1);
        docStore2 = new DocumentStore { Url = ConfigurationManager.AppSettings["RavenShard2"] };
        shards.Add(docStore2);

        documentStore = new ShardedDocumentStore(new ShardStrategy(), shards);
        documentStore.Initialize();

        IndexCreation.CreateIndexes(typeof(RavenDataAccess).Assembly, documentStore);

        return documentStore;
    }
    catch
    {
        if (documentStore != null)
        {
            documentStore.Dispose();
        }
        else
        {
            if (docStore2 != null) docStore2.Dispose();
            if (docStore1 != null) docStore1.Dispose();
        }
        throw;
    }
}

...and let the caller of GetRavenDatabase() handle disposal of the returned IDocumentStore.

Answer 3

Here's why object initializers in a using statement result in the CA warning:

Your code that looks like this:

using (DocumentStore docStore2 = new DocumentStore { Url = ConfigurationManager.AppSettings["RavenShard2"] })  // Line 85
{
   ...
}

... essentialy becomes this, due to the way object initializers work:

DocumentStore foo = new DocumentStore;
foo.Url = ConfigurationManager.AppSettings["RavenShard2"];
using(DocumentStore docStore2 = foo)
{
   ...
}

So as you can see, the initialization of the DocumentStore now happens outside of the using{} block, so if the line that sets temp.Url throws an exception, your DocumentStore will not be disposed.

There are a number of workarounds, such as passing the parameters in to the object's constructor, setting the properties inside of the using statement instead of using object initializers, or using try/finally blocks.

Answer 4

Considering that CA2000: Dispose objects before losing scope documentation states (part from it):

Nesting constructors that are protected only by one exception handler. For example:

using (StreamReader sr = new StreamReader(new FileStream("C:\myfile.txt", FileMode.Create))) { ... }

causes CA2000 to occur because a failure in the construction of the StreamReader object can result in the FileStream object never being closed.

and considering that I don't see in the code provided any disposable object allocation other then DocumentStore itself, I would suppose that this is a bug of compiler.