Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

IDisposable created within a method and returned

I happy coded quite a project that works fine and do not manifest any oddities at runtime. So I've decided to run static code analysis tool (I'm using Visual Studio 2010). It came out that rule CA2000 is being violated, message as follows:

Warning - CA2000 : Microsoft.Reliability : In method 'Bar.getDefaultFoo()', call System.IDisposable.Dispose on object 'new Foo()' before all references to it are out of scope.

The code refered goes like this:

private static IFoo getDefaultFoo()
{
    return (Baz.canIDoIt()) ? new Foo() : null;
}

I thought myself: maybe conditional expression spoils the logic (mine or validator's). Changed to this:

private static IFoo getDefaultFoo()
{
    IFoo ret = null;
    if (Baz.canIDoIt())
    {
        retFoo = new Foo();
    }
    return ret;
}

Same thing happened again, but now the object was referred to as retFoo. I've googled, I've msdn'ed, I've stackoverflow'ed. Found this article. There are no operations I need done after creating the object. I only need to return the reference to it. However, I have tried to apply the pattern suggested in OpenPort2 example. Now code looks like this:

private static IFoo getDefaultFoo()
{
    Foo tempFoo = null;
    Foo retFoo = null;
    try
    {
        if (Baz.canIDoIt())
        {
            tempFoo = new Foo();
        }
        retFoo= tempFoo;
        tempFoo = null;
    }
    finally
    {
        if (tempFoo != null)
        {
            tempFoo.Dispose();
        }
    }
    return retFoo;
}

Same message again, but tempFoo variable is rule violator this time. So basically, code went twisted, longer, little irrational, unnecesarrily complex and does the very same, but slower.

I've also found this question, where same rule seems to attack a valid code in similar manner. And there questioner is adviced to ignore the warning. I've also read this thread and a mass of similar questions.

Is there anything I missed? Is the rule bugged / irrelevant? What should I do? Ignore? Handle in some magickal way? Maybe apply some design pattern?

Edit:

After Nicole's request, I'm submiting the whole related code in form I also tried of using.

public class DisposableFooTest
{
    public interface IFoo
    {
        void bar();
    }

    public class Foo : IFoo, IDisposable
    {
        public void bar()
        {
            Console.Out.WriteLine("Foo baring now");
        }

        public void Dispose()
        {
            // actual Dispose implementation is irrelevant, or maybe it is?
            // anyway I followed microsoft dispose pattern
            // with Dispose(bool disposing)
        }
    }

    public static class Baz
    {
        private static bool toggle = false;
        public static bool canIDoIt()
        {
            toggle ^= true;
            return toggle;
        }
    }

    private static IFoo getDefaultFoo()
    {
        IFoo result = null;
        try
        {
            if (Baz.canIDoIt())
            {
                result = new Foo();
            }

            return result;
        }
        catch
        {
            if (result != null)
            {
                (result as IDisposable).Dispose();
                // IFoo does not inherit from IDisposable, hence the cast
            }

            throw;
        }
    }

    public static void Main()
    {
        IFoo bar = getDefaultFoo();
    }
}

Analysis report contain the folowing:
`CA2000 : Microsoft.Reliability : In method 'DisposableFooTest.getDefaultFoo()', call System.IDisposable.Dispose on object 'result' before all references to it are out of scope. %%projectpath%%\DisposableFooTest.cs 44 Test

Edit2:

The following approach resolved CA2000 problem:

private static IFoo getDefaultFoo()
{
    Foo result = null;
    try
    {
        if (Baz.canIDoIt())
        {
            result = new Foo();
        }
        return result;
    }
    finally
    {
        if (result != null)
        {
            result.Dispose();
        }
    }
}

Unfortunately, I can't go that way. What is more, I'd rather expect following object-oriented principles, good practices and guidelines to simplify the code, make it readable, maintanable and extendible. I doubt anyone read it as intended: give the Foo if possible, or null otherwise.

like image 269
Krzysztof Jabłoński Avatar asked Jan 24 '13 00:01

Krzysztof Jabłoński


People also ask

Is IDisposable called automatically?

By default, the garbage collector automatically calls an object's finalizer before reclaiming its memory. However, if the Dispose method has been called, it is typically unnecessary for the garbage collector to call the disposed object's finalizer.

What does it mean when an object implements IDisposable?

Typically, types that use unmanaged resources implement the IDisposable or IAsyncDisposable interface to allow the unmanaged resources to be reclaimed. When you finish using an object that implements IDisposable, you call the object's Dispose or DisposeAsync implementation to explicitly perform cleanup.

How is IDisposable interface implemented?

For implementing the IDisposable design pattern, the class which deals with unmanaged objects directly or indirectly should implement the IDisposable interface. And implement the method Dispose declared inside of the IDisposable interface. We do not directly deal with unmanaged objects.

What is the use of IDisposable interface in C#?

IDisposable is an interface that contains a single method, Dispose(), for releasing unmanaged resources, like files, streams, database connections and so on.


1 Answers

This is a false positive warning. There is no way to return an appropriate instance of IFoo, if IFoo implements IDisposable, without the code analysis tool warning you that you're not disposing of it properly.

The code analysis doesn't analyze your intent or logic, it just tries to warn about common errors. In this case, it "looks like" you're using an IDisposable object and not calling Dispose(). Here, you're doing it by design, as you want your method to return a new instance and its acting as a form of factory method.

like image 173
Reed Copsey Avatar answered Sep 20 '22 13:09

Reed Copsey