IDisposable and CA2000 warning during VS2010 Code Analysis

Question

I need some advice here, I hope somebody can help me. I have the following class structure (simplified):

public class Bar: IDisposable {...}

public abstract class FooBase: IDisposable
{
    Bar bar;
    bool disposed;

    internal FooBase(Bar bar)
    {
        this.bar=bar;
    }

    public void Dispose()
    {
         Dispose(true);
         GC.SupressFinalize(this);

    }

    protected void Dispose(bool disposing)
    {
         if (!this.disposed)
         {
             if (disposing)
             {
                 this.bar.Dispose();
             }

             this.disposed = true;
         }
    }
}

public FooA: Foo {...}
public FooB: Foo {...}

public static class FooProvider
{
    public static FooA GetFooA()
    {
       Bar bar = new Bar();
       ...
       return new FooA(bar);
    }

    public static FooB GetFooB()
    {
        Bar bar = new Bar();
        ...
        return new FooB(bar);
    }

    ...
}

When I run Code Analysis on this, I get Warnings CA2000 on all 'CreateFooX()' methods of the FooProvider class. This warning gives the following message:

"Microsoft. Reliability: In method 'FooProvider.GetFooX()', call System.IDisposable.Dispose on object 'bar' before all references to it are out of scope."

Microsoft recommends to never suppress this warning but I'm not really sure its warning about a real problem in the code. True that 'bar' is not disposed before going out of scope in whatever 'CreateFooX()' method we consider but a reference to it lives on in the 'FooX' object which eventually will get disposed and will in turn take care of disposing 'bar'.

Am I understanding something wrong about how the Dispose pattern should work and I have some fundamental flaw in my code or should I just suppress this warning?

EDIT

Due to some comments I tried modifying the factory methods to the following:

public static class FooProvider
{
    public static FooA GetFooA()
    {
       Bar bar = null;

       try
       {
           bar =  new Bar();
           ...
           return new FooA(bar);
       }
       catch
       {
           if (bar != null) bar.Dispose();
           throw;
       }
    }

    ...
}

But I still get the same warning. I guess its just a false positive and I'm safe ingoring it.

Thanks for any advice.

Answer 1

This is a typical false positive on Code Analysis' part. It really cannot understand the intrinsic situation of your code, so it throws a general answer at it. Proceed with caution, but whenever you verify that you have a false positive, you can safely ignore it.

Answer 2

That is not a false positive. What if an exception is thrown after the Bar is created but before it is passed to the Foo constructor? I see several code paths where one or more objects might not be disposed of.

Answer 3

Your disposable pattern seems a little off to me. I don't think you should be calling bar.Dispose in the FooBase class. For safety of the objects you are disposing and being able to safly call Dispose multiple time I would recoment this approach.

  private bool _disposed;
  public void Dispose()
  {
     Dispose( true );
     GC.SuppressFinalize( this );
  }

  protected virtual void Dispose( bool disposing )
  {
     if ( disposing )
     {
        if ( !_disposed )
        {
           if ( Bar != null )
           {
              Bar.Dispose();
           }

           _disposed = true;
        }
     }
  }

As for the error, I think that this should take care of the static analysis warning. I implemented your code as follows in a test project, enabled all static analysis warnings without having an issue with warnings.

public class Bar : IDisposable
{
  private bool _disposed;
  public void Dispose()
  {
     Dispose( true );
     GC.SuppressFinalize( this );
  }

  protected virtual void Dispose( bool disposing )
  {
     if ( disposing )
     {
        if ( !_disposed )
        {
           _disposed = true;
        }
     }
  }
}

public abstract class FooBase : IDisposable
{
  public Bar Bar
  {
     get;
     set;
  }

  internal FooBase( Bar bar )
  {
     Bar = bar;
  }

  private bool _disposed;
  public void Dispose()
  {
     Dispose( true );
     GC.SuppressFinalize( this );
  }

  protected virtual void Dispose( bool disposing )
  {
     if ( disposing )
     {
        if ( !_disposed )
        {
           if ( Bar != null )
           {
              Bar.Dispose();
           }

           _disposed = true;
        }
     }
  }
}

public class FooA : FooBase
{
  public FooA( Bar bar )
     : base( bar )
  {
  }
}

public static class FooProvider
{
  public static FooA GetFooA()
  {
     Bar bar;
     using ( bar = new Bar() )
     {
        return new FooA( bar );
     }
  }
}

[TestClass]
public class UnitTest1
{
  [TestMethod]
  public void StaticAnalysisTest()
  {
     Assert.IsNotNull( FooProvider.GetFooA().Bar );
  }
}

I hope this is helpful.

Answer 4

At least part of this problem isn't really a false positive, even if it's not necessarily a very useful issue detection. To fix the remaining problem your edited version, you need to open the try block immediately after the bar assignment, not before it. e.g.:

Bar bar = new Bar();
try
{
    ///...            
    return new FooA(bar);
}
catch
{
    bar.Dispose();
    throw;
}

Unfortunately, after you make this change, you'll still get a CA2000 violation, which will probably be a false positive. That's because the rule doesn't check to see if you're placing bar into the state of the newly created FooA. If it is going into state in the FooA, you can safely create a suppression for the violation. However, if it's not going into state in FooA, you should dispose it in a finally clause instead of a catch clause.