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;
    }
}

check it out, maybe it's the same stuff you have: http://stackoverflow.com/questions/3932131/how-to-get-rid-of-ca2000-warning-when-ownership-is-transferred — cookieMonster, Apr 12 '12 at 14:03
That link is interesting. I'm calling Add() on Shards, but it doesn't implement ICollection. And since Shards is not my code, I can't change it. This is the Shards signature: public class Shards : List — Bob Horn, Apr 12 '12 at 14:16
Sorry, i'm getting a bit silly at the end of the day - didn't notice you cannot change it. So, you may just suppress warning message of that method. — cookieMonster, Apr 12 '12 at 14:36

Jim · Accepted Answer · 2012-04-12T15:43:25.430

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.

This worked! Wow, what a pain for something seemingly so simple. Thanks, Jim! — Bob Horn, Apr 12 '12 at 15:44
@Bob - Trying to adhere to CA can be a pain but it is for a good reason :-) Now if only the compiler helped us out by making all this automatic, then we wouldn't have to write all this extra code. — Jim, Apr 12 '12 at 15:47
The docStores will (probably) be disposed twice if `tempSharededDocumentStore.Initialize()` throws. But that might be ok. — Torbjörn Kalin, Apr 13 '12 at 06:07

Torbjörn Kalin · Answer 2 · 2012-04-12T15:02:39.490

1

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.

edited Apr 12 '12 at 15:02

answered Apr 12 '12 at 14:15

Torbjörn Kalin

1,976
1
22
31

I tried that and now I get three violations (the two new DocumentStore lines, and the new ShardedDocumentStore line). I'd let the caller worry about disposal, but the caller is a static property on a static class: private static IDocumentStore _ravenDatabase = GetRavenDatabase(); – Bob Horn Apr 12 '12 at 14:23
I updated the code to handle exceptions. However, I wonder if the compiler can really cover all flows. I would say the ravendb API is quite crappy and it might be impossible to write code using it that doesn't raise this warning. Perhaps you need to disable the warning for this code? And regarding the static field, I think you need to find a different solution for that. – Torbjörn Kalin Apr 12 '12 at 14:34
If an exception occurs in `Shards.Add`, one cannot expect `Shards.Dispose` to take care of the `DocumentStore` that may or may not have been added. If `DocumentStore` properly implements `IDisposable.Dispose`, it should be safe to, in case of exception, call `Dispose` on any instances you've created after you call `Shards.Dispose`. On the other hand, I'd suggest using an `ok` flag along with a `try`/`finally` block. If the `finally` block for your routine executes without `ok` being set, dispose the `Shards` and `DocumentStore` objects you've created. – supercat Apr 12 '12 at 14:45
I prefer a pattern with an `ok` flag and `finally`. In C#, if your code tries to `catch` an exception and then rethrow it, and an exception occurs on one of the `DocumentStore` constructor calls, the stack trace will no longer show the line number of the constructor call which failed, but rather the line number of the rethrow. Note that if all code paths in a `try` block either throw or return a value, the compiler will recognize that code cannot run past the `finally`, so there's no need to `return` a value there. – supercat Apr 12 '12 at 15:27
Still get the CA errors for the docStore lines. Also get this error for using var: Cannot assign to an implicitly-typed local variable. – Bob Horn Apr 12 '12 at 15:33
@supercat The stack trace is overwritten if you do `throw ex` but preserved if you only do `throw` (as in the code above). @BobHorn I'd say the code is correct now, but that the compiler can't handle it. – Torbjörn Kalin Apr 13 '12 at 06:05
If `Foo` calls `Bar` on lines 23 and 24, and one of those calls throws an exception which is caught and then rethrown (using `throw`, not `throw ex`) on line 37, the stack trace will show `Foo line 37` and will have no record of whether it was the call on line 23 or 24 that failed. Try it. The information loss is generally not as severe as when using `throw ex`, but can nonetheless be annoying. – supercat Apr 13 '12 at 18:10

score 1 · Answer 3 · answered Apr 12 '12 at 16:15

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.

This is good insight on the property initializer however in this example, Bob shouldn't use "using" statements on objects he plans to return to the caller. By using the "using" statements, his new objects will be disposed before they are returned and thus not usable by the caller of this method. — Jim, Apr 12 '12 at 16:22
Thanks for the tip +1. Helped me understand why it was happening in my project. — James Fleming, Apr 20 '13 at 12:36

score 0 · Answer 4 · answered Apr 12 '12 at 14:09

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.

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

4 Answers4