Question

I'm writing a method that resets a logging system. I need to get an instance of a CsvFileLogWriter (a custom class) and pass it to the reset method. CsvFileLogWriter is disposable so I get a CA2000 warning tell me:

Warning 2   CA2000 : Microsoft.Reliability : In method 'Logger.InitializeCsvLogger
(string)', call System.IDisposable.Dispose on object 'tempWriter'
 before all references to it are out of scope.

I've followed the instructions relating to CA2000 and I end up with the following method. However, I still get the CA2000 warning.

public static void InitializeCsvLogger(string path)
{
    ILogWriter tempWriter = null;

    try
    {
        tempWriter = new CsvFileLogWriter(path);
        ResetWriter(tempWriter);
        tempWriter = null;
    }
    finally
    {
        if (tempWriter != null)
            tempWriter.Dispose();
    }
}

Can someone please spot my mistake?

EDIT

I do not wish to dispose of the writer that is reference by tempWriter - this is not a temporary object, just a temporary reference. I only dispose of it if there is a failure within the try block (so tempWriter never gets set to null and the if-statement in the finally block clears up the resources.) I do not want tempWriter disposing of unless this failure occurs - the object itself must remain in use after being set in a property by ResetWriter(tempWriter). This is as per the CA2000 rules - see http://msdn.microsoft.com/en-us/library/ms182289.aspx?queryresult=true

For clarification, here is what ResetWriter does - Writer is a static property. The method disposes the old writer and sets the new one.

private static void ResetWriter(ILogWriter newWriter)
{
    if (Writer != null)
        Writer.Dispose();
    Writer = newWriter;
}

EDIT

I think as SLaks stated that it's a false positive. If I take the contents of ResetWriter and put them in place of the call to ResetWriter (essentially reversing an Extract Method refactoring) the CA2000 goes away.

Or in other words, the following does not give the CA2000 warning:

public static void InitializeCsvLogger(string path)
{
    ILogWriter tempWriter = null;

    try
    {
        tempWriter = new CsvFileLogWriter(path);
        if (Writer != null)
            Writer.Dispose();
        Writer = tempWriter;
        tempWriter = null;
    }
    finally
    {
        if (tempWriter != null)
            tempWriter.Dispose();
    }
}
Was it helpful?

Solution

This warning is a false positive.

The Code Analysis engine doesn't realize that ResetWriter needs the writer to stay alive, so it wants you to dispose it in all circumstances.

You should suppress the warning.

OTHER TIPS

When you assign null to tempWriter:

tempWriter = null;

tempWriter no longer refers to the object you created. Therefore, there's no way for you to Dispose the object.

You should really use a using block in this case instead:

using(var tempWriter = new CsvFileLogWriter(path))
{
    ResetWriter(tempWriter);
}

By doing that, you no longer have to worry about calling Dispose (or setting the reference to null).

By writing tempWriter = null you're preventing it from getting disposed, since the finally block only runs after that.

You should use the using statement instead.

This answer is correct, but contradicts your actual intentions.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top