Should I heed this superficially nonsensical Code Analysis warning?

Question

When I select Analyze > RUn Code Analysis on Solution in Visual Studio 2013, I get, "CA2202 Do not dispose objects multiple times Object 'fs' can be disposed more than once in method 'RoboReporterSQL.SaveReportDataToDB(string, string)'. To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object."

The indicated line of code is:

fs.Close();

And here is the code in context:

    internal static void SaveReportDataToDB(string filename, string 
RESTFilename)
    {
        if (RecordAlreadyExists(RESTFilename)) return;
        string EXCEL_FILE = "application/vnd.ms-excel";
        DateTime begDate = 
RoboReporterConstsAndUtils.GetBeginDate(RESTFilename);
        DateTime endDate = 
RoboReporterConstsAndUtils.GetEndDate(RESTFilename);

        var fs = new FileStream(filename, FileMode.Open, FileAccess.Read);
        BinaryReader br = new BinaryReader(fs);
        Byte[] bytes = br.ReadBytes((Int32)fs.Length);
        br.Close();
        fs.Close();

        using (var sqlConn = new SqlConnection(CPSConnStr))
        {
            var insertStr = "INSERT INTO ReportsGenerated (FileBaseName, 
ContentType, BinaryData, BeginDate, EndDate) " +
                             "VALUES (@FileBaseName, @ContentType, 
@BinaryData, @BeginDate, @EndDate)";

            using (var insertRptsGenerated = new SqlCommand(insertStr))
            {
                insertRptsGenerated.Connection = sqlConn;
                insertRptsGenerated.Parameters.Add("@FileBaseName", 
SqlDbType.VarChar, 100).Value = RESTFilename;
                insertRptsGenerated.Parameters.Add("@ContentType", 
SqlDbType.VarChar, 50).Value = EXCEL_FILE;
                insertRptsGenerated.Parameters.Add("@BinaryData", 
SqlDbType.Binary).Value = bytes;
                insertRptsGenerated.Parameters.Add("@BeginDate", 
SqlDbType.DateTime).Value = begDate;
                insertRptsGenerated.Parameters.Add("@EndDate", 
SqlDbType.DateTime).Value = endDate;
                sqlConn.Open();
                insertRptsGenerated.ExecuteNonQuery();
            }
        }
    }

So the warning is claiming that the FileStream is being closed twice if I call "fs.Close();"

Although I don't refute that with certainty, I question it because I don't see it being elsewhere closed.

After all, it is not in a "using" block, so how is it being closed?

The question is: should I really remove that line of code ("fs.Close();")?

Note: Resharper did not whin[g]e a whit about this - with "fs.Close();" in or out, it raises no warning flags either way.

You are closing it twice. You're closing it when closing the BinaryReader - it also closes the underlying stream. And you really should be doing the using — Allan S. Hansen, May 26 '16 at 17:57
This is discussed to death on SO and only your reputation keep the question alive... — Alexei Levenkov, May 26 '16 at 18:03
I'm wondering if the wording of the error just off in this case. If closing the `BinaryReader` might cause the `FileStream` to be disposed, then accessing the `Close` method of the disposed object might throw an `ObjectDisposedException`. — Chris Dunaway, May 26 '16 at 18:23

score 4 · Accepted Answer · answered May 26 '16 at 17:58

Frankly you shouldn't be closing those streams explicitly, you should be using using blocks.

Anyway, it gives you that warning because the stream is already closed. When readers/writers wrap a stream, closing them will close the underlying stream as well. Some of readers/writers give you the option to leave the stream open however.

In your particular case, you can kill two birds with one stone by using some of the other methods available in the File class. Consider using File.ReadAllBytes() to read in your file.

Evk · Answer 2 · 2016-05-26T18:33:21.953

3

Want to add that it's still quite useless warning. I never saw any implementation of IDisposable which would throw ObjectDisposedException when you call Dispose multiple times. Actually in documentation of IDisposable.Dispose the following is written:

If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times

However, you call Close on your stream, not Dispose, and while in this case it's the same, in general you indeed should not call any non-Dispose methods on disposed objects, so at least change Close to Dispose (or better wrap all in using).

edited May 26 '16 at 18:33

answered May 26 '16 at 18:12

Evk

98,527
8
141
191

I wonder if it could be thinking that because the `BinaryReader` was closed that the `FileStream` may be disposed. So the warning is telling that accessing the already disposed `FileStream` (fs) object could result in an `ObjectDisposedMethod`. I mean, what happens if you dispose a `FileStream` and then call it's `Close` method? – Chris Dunaway May 26 '16 at 18:20
Nothing happens. But if you try to do anything else (reading\writing etc) - then it will throw ObjectDisposedException. I use this pattern (disposing both reader and stream in two using blocks) for many years already, without any troubles. – Evk May 26 '16 at 18:24
I don't mean Disposing twice. I mean accessing the 'Close` method of an already disposed object could throw couldn't it? – Chris Dunaway May 26 '16 at 18:26
Close method just calls Dispose, that is all. So those methods are the same in terms of what they do (so you can call both any times you want, they won't throw ObjectDisposed). Actually description of this rule (https://msdn.microsoft.com/en-us/library/ms182334.aspx) says to not dispose multiple times because what I said above in my answer "might change in the future". I doubt that ever will change since there are no reasons to throw exception on multiple dispose calls. – Evk May 26 '16 at 18:27
Well on second thought I think that even if it's harmless in this case - you should indeed not call Close on disposed stream. The fact that it calls Dispose is implementation detail which you should not rely on. You can Dispose multiple times, but for Close that is not guaranteed. Missed that since I _never_ use Close method on streams. – Evk May 26 '16 at 18:30
I did a little experiment in LinqPad. Calling `fs.Close` after disposing of the `FileStream` does not raise an exception, but accessing any other property does, of course. So I'm guessing that the code analyzer sees that _something_ is being accessed on an object that might already be disposed and throws up a warning. I can't think of anything else that might be going on here. – Chris Dunaway May 26 '16 at 18:33
I just created the FileStream in a Using block and then after it was disposed, I called `Close` again and no exception occurred. Changing the call to `Close` to any other property resulted in the exception. LinqPad does not throw that warning to my knowledge. – Chris Dunaway May 26 '16 at 18:37
I mean - if you change br.Close() to br.Dispose and fs.Close to fs.Dispose() - this warning is still raised? – Evk May 26 '16 at 18:39
@ChrisDunaway: Using `Close()` isn't a great example. `Close()` methods on `IDisposable` objects are typically implemented to call `Dispose()` which is always safe. Call another method (like `Read()`) and you'll get the `ObjectDisposedException`. Analyzers at a minimum would try to raise warnings about calling other methods on objects which have been disposed, whether or not they are aware of implementation specific details shouldn't be expected. – Jeff Mercado May 26 '16 at 19:09

Should I heed this superficially nonsensical Code Analysis warning?

2 Answers2

Related