HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

MS CA2202: Object can be disposed more then once

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
oncecanmoreca2202disposedthenobject

Problem

When running Code Analysis on my project I get the following warning:


CA2202 : Microsoft.Usage : Object 'fs' can be disposed more than once in method

I don't see the problem, and the code compiles fine and produces the expected results in all test cases.
If I remove the fs.Dispose(); statement then Code Analysis instead produces this warning:


object 'fs' is not disposed along all exception paths. Call System.IDisposable.Dispose on object 'fs' before all references to it are out of scope.

I am much grateful if you can review my code so that I can learn the proper way of disposing resources.

private string ReadFileContent(string filename, Encoding encoding, string category, FileArchive archiveConfig)
    {
        FileStream fs = null;
        StreamReader sr = null;
        string fileContent = string.Empty;

        _logger.Debug("Reading file content of \"" + category + "\" file \"" + filename + "\"");

        try
        {
            fs = new FileStream(filename, FileMode.Open, FileAccess.ReadWrite, FileShare.None, 64 * 1024, FileOptions.SequentialScan);
            fs.Position = 0;
            sr = new StreamReader(fs, encoding, true);
            fileContent = sr.ReadToEnd();
        }
        catch (Exception)
        {
            ArchiveFile(filename, archiveConfig.ErrorFolder, category, _logger);
            throw;
        }
        finally
        {
            if (sr != null)
                sr.Dispose();
            if (fs != null)
                fs.Dispose(); 
        }

        return fileContent;
    }

Solution

As @amon mentioned, the correct way of referencing IDisposable Objects in C# is the using-statement.

In general you then have the try-catch block around the using statement to catch eventual initialization errors. This means your Code would look somewhat like this:

String fileContent = String.Empty;
_logger.Debug("Debug-String");

try{
   using(FileStream fs = new Filestream(filename, FileMode.Open, FileAccess.ReadWrite,
     FileShare.None, 64 * 1024, FileOptions.SequentialScan){
      fs.Position = 0;
      using (StreamReader sr = new Streamreader(fs, encoding, true){
         fileContent = sr.ReadToEnd();
      }
   }
}
catch (Exception){
   ArchiveFile(filename, archiveConfig.ErrorFolder, category, _logger);
   throw;
}

return fileContent;


This also allows you to declare variables only when used.

Finally a small note on filename... you are using camelCase in all variables but that. Be consistent in your naming schema

Code Snippets

String fileContent = String.Empty;
_logger.Debug("Debug-String");

try{
   using(FileStream fs = new Filestream(filename, FileMode.Open, FileAccess.ReadWrite,
     FileShare.None, 64 * 1024, FileOptions.SequentialScan){
      fs.Position = 0;
      using (StreamReader sr = new Streamreader(fs, encoding, true){
         fileContent = sr.ReadToEnd();
      }
   }
}
catch (Exception){
   ArchiveFile(filename, archiveConfig.ErrorFolder, category, _logger);
   throw;
}

return fileContent;

Context

StackExchange Code Review Q#44245, answer score: 4

Revisions (0)

No revisions yet.