Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

How to fix a CA2000 IDisposable C# compiler warning, when using a global cache

CA2000 is a warning regarding the IDisposable interface:

CA2000 : Microsoft.Reliability : In method 'ImportProcessor.GetContext(string)', call System.IDisposable.Dispose on object 'c' before all references to it are out of scope.

My method is used to store a cache of context like so:

public class RegionContext : IDisposable { /* Implement Dispose() here */ } 

private Dictionary<string, RegionContext> contextCache = new ..... ();

public RegionContext GetContext(string regionCode)
{
    RegionContext rc = null;

    if (!this.contextCache.TryGetValue(regionCode.ToUpper(), out rc))
    {
        rc = new RegionContext(regionCode);
        this.contextCache.Add(regionCode.ToUpper(), rc);
    }

    return rc;
}

Where would you use the using() statement that fixes this compiler warning?

My outer class actually does iterate and dispose of the contents in the contextCache in its own implementation. Shall I suppress it, or is there a way to correctly get rid of this warning?

like image 721
Dominic Zukiewicz Avatar asked Jul 13 '11 19:07

Dominic Zukiewicz


2 Answers

This CA2000 warning comes up any time you have a return value that's IDisposable and don't handle the case where the method throws an exception. In that instance, the caller won't get a valid instance of your object, so it has no way to dispose it. Therefore you have to.

I assume that you won't want to dispose the object if you pull it out of your cache successfully. In that case, you would need to do something like this to make sure the object you might create locally gets disposed in all cases:

public RegionContext GetContext(string regionCode)
{
    RegionContext temp = null;
    RegionContext rc = null;

    try
    {
        if (!this.contextCache.TryGetValue(regionCode.ToUpper(), out rc))
        {
            temp = new RegionContext(regionCode);
            this.contextCache.Add(regionCode.ToUpper(), temp);

            rc = temp;
            temp = null;
        }

        return rc;
    }
    finally 
    {
        if ( temp != null ) 
        {
             temp.Dispose();
        }
    }
}
like image 128
Michael Edenfield Avatar answered Nov 10 '22 08:11

Michael Edenfield


What CA2000 is complaining about here is that the variable could be "orphaned" in an undisposed state if there's an exception while attempting to add it to the cache. To address the problem thoroughly, you could add a try/catch as follows (the newContext variable is used only so that CA2000 can detect the fix):

public RegionContext GetContext(string regionCode)
{
    RegionContext rc = null;
    if (!this.contextCache.TryGetValue(regionCode.ToUpper(), out rc))
    {
        RegionContext newContext = new RegionContext(regionCode);
        try
        {
            this.contextCache.Add(regionCode.ToUpper(), newContext);
        }
        catch
        {
            newContext.Dispose();
            throw;
        }

        rc = newContext;
    }

    return rc;
}

Personally, I find this sort of thing to be somewhat ridiculous overkill in most cases, but ymmv...

like image 6
Nicole Calinoiu Avatar answered Nov 10 '22 07:11

Nicole Calinoiu