using statement FileStream and / or StreamReader - Visual Studio 2012 Warnings

Asked 17/8, 2012 at 5:15 Answered 18/4, 2014 at 16:52

Solved c#.net dispose using visual-studio-2012

The new Visual Studio 2012 is complaining about a common code combination I have always used. I know it seems like overkill but I have done the following in my code 'just to be sure'.

using (var fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
{
    using (var sr = new StreamReader(fs))
    {
        // Code here
    }
}

Visual studio is 'warning' me that I am disposing of fs more than once. So my question is this, would the proper way to write this be:

using (var fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
{
    var sr = new StreamReader(fs);
    // do stuff here
}

Or should I do it this way (or some other variant not mentioned).

var fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);

using (var sr = new StreamReader(fs))
{
    // Code here
}

I searched several questions in StackOverflow but did not find something that addressed the best practice for this combination directly.

Thank you!

Hilliary answered 17/8, 2012 at 5:15 Comment(2)

+1: Good question, JUbbard80! :) – Cynic 17/8, 2012 at 19:16

With a double-using like this in VS2013, a FileStream and a StreamReader, there's no warning emitted. Is that because of the combination of FileStream and StreamReader? Or does VS2013 (as opposed to VS2012) recognize that this particular double-disposal is a nonissue? – Prakash 10/9, 2016 at 18:4

The following is how Microsoft recommends doing it. It is long and bulky, but safe:

FileStream fs = null;
try
{
    fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
    using (TextReader tr= new StreamReader(fs))
    {
        fs = null;
        // Code here
    }
}
finally
{
    if (fs != null)
        fs.Dispose();
}

This method will always ensure that everything is disposed that should be despite what exceptions may be thrown. For example, if the StreamReader constructor throws an exception, the FileStream would still be properly disposed.

Frankfort answered 17/8, 2012 at 5:29 Comment(8)

Oh that is handy. I didn't realize the example used in CA2202 (Microsoft Recommends link) was similar code example in my question. – Hilliary 17/8, 2012 at 5:34

Good. You probably wouldn't have added that extra comma syntax error that I just edited away, either ;). – Frankfort 17/8, 2012 at 5:44

I added a little bit more of an explanation, although I think it was pretty clear from the example and the link as of to "why". If you think an answer provides misinformation, it is appropriate to downvote it. Otherwise, just upvote your preferred answer. This answer can hardly be misinformation if it's almost verbatim what Microsoft recommends. – Frankfort 17/8, 2012 at 16:41

Upon further testing. This answer does not clear the VS2012 warning with StreamReader. Only StreamWriter. – Hilliary 17/8, 2012 at 18:9

As there is lack of consensus, I am going to go with the Microsoft recommended practice. Although I think it is overkill. This solution still raises the warning, but as hvd points out, this warning is for when IDisposable is not implemented correctly. The objects in question (FileStream, StreamReader) implement IDisposable correctly & the warning can be ignored. Although I agree with the points made by Dan, Daniel, hvd and paulsm4. My answer does not generate the warning but it does risk an undisposed FileStream if the streamreader constructor throws an exception. Thank you all for your help. – Hilliary 18/8, 2012 at 20:57

This pattern is ridiculous. Disposing objects multiple times is generally safe (I don't know a single case where it wouldn't). This code is of terrible quality and shouldn't be recommended by Microsoft. Also, it is not safe in case of an asynchronous exception just before "fs = null;". – Gat 18/8, 2012 at 22:11

@usr, so please post your answer. – Juieta 24/2, 2017 at 15:23

@Juieta no need, multiple answers exist. Just trying to warn people because many would be seduced by the accepted status and the high vote count (which is not a stupid thing to rely on as a beginner if you have no other way to decide). Don't use this code. – Gat 24/2, 2017 at 17:17

Visual studio is 'warning' me that I am disposing of fs more than once.

You are, but that is fine. The documentation for IDisposable.Dispose reads:

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.

Based on that, the warning is bogus, and my choice would be to leave the code as it is, and suppress the warning.

Toe answered 18/8, 2012 at 20:37 Comment(0)

As Dan's answer only appears to work with StreamWriter, I believe this might be the most acceptable answer. (Dan's answer will still give the disposed twice warning with StreamReader - as Daniel Hilgarth and exacerbatedexpert mentions, StreamReader disposes the filestream)

using (TextReader tr = new StreamReader(new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)))
{
    string line;
    while ((line = tr.ReadLine()) != null)
    {
        // Do work here
    }
}

This is very similar to Daniel Hilgarth's answer, modified to call dispose via the Using statement on StreamReader as it is now clear StreamReader will call dispose on FileStream (According to all the other posts, documentation referenced)

Update:

I found this post. For what it is worth. Does disposing streamreader close the stream?

Hilliary answered 17/8, 2012 at 18:24 Comment(7)

Not certain about exception handling, however. I would think any exceptions would happen in the FileStream constructor and therefore not make it to the initialization of StreamReader. So in this instance I don't have to worry about an undisposed FileStream due to an exception in the StreamReader. I could be wrong of course... – Hilliary 17/8, 2012 at 18:27

The using (f(x)) { } statement is really compiled as y = f(x); using(y) { }. So, any exceptions thrown by the initializer of the using statement will not trigger the implicit finally statement. The best way to get around this is to design constructors to be exception safe and clean-up resources after a throw, which I believe the Stream classes do. – Peroxidase 17/8, 2012 at 19:2

Like I've said, I think my example is overkill, and in the case of the StreamReader class, I think the above is totally fine, but if you were using a different class other than StreamReader, or they change the implementation in the future such that it is possible for that the constructor would throw an exception after the inner IDisposable dependency was instantiated, then the inner IDisposable would not be disposed. I still vote that the example I posted is the safest. – Frankfort 17/8, 2012 at 19:6

Dan - your example still throws the visual studio warning, which is why I removed it as the answer. It will not throw the warning with StreamWriter. Just StreamReader as streamreader disposes of the object internally and VS2012 knows that. – Hilliary 17/8, 2012 at 19:30

@JHubbard80: The code you are using now should issue another warning, something like CA2000. – Devoid 18/8, 2012 at 5:57

@DanielHilgarth According to the documentation, you are correct. It should. However I just tested it. Using both my code and the code in the CA2000 doc (which has a non escaped backslash, heh). It does not raise CA2000. Weird. I will attempt to look into it further before I mark this as answered. – Hilliary 18/8, 2012 at 20:15

@Hilliary Can I ask what was the result of your investigation? – Technics 13/11, 2013 at 16:33

Yes, the correct way would be to use your first alternative:

using (FileStream fs = new FileStream(filePath, FileMode.Open,
                                      FileAccess.Read, FileShare.ReadWrite)) 
{ 
    TextReader tr = new StreamReader(fs); 
    // do stuff here 
}

The reason is the following:
Disposing the StreamReader only disposes the FileStream so that's actually the only thing you need to dispose.

Your second option (just the inner "using") is no solution as it would leave the FileStream undisposed if there was an exception inside the constructor of the StreamReader.

Devoid answered 17/8, 2012 at 5:17 Comment(6)

I would have thought it important to dispose the StreamReader object just in case that object has items that need to be disposed as well. It may be plainly known (via reflection, other) that StreamReader only disposes of the FileStream but as an everyday similar situation practice I can't predict it. When the reader is passed a string argument, I'm guessing it creates and disposes of the filestream it creates internally itself. When passed a filestream I'd assume it doesn't dispose of the object it received as an arg as it doesn't know if the stream will continue to be used externally. – Hilliary 17/8, 2012 at 6:1

@JHubbard80: Your assumption is wrong. StreamReader does indeed dispose the Stream it was passed, that's what's causing your warning in the first place. You are correct though, using my code might be problematic if (1) the class ever changes its implementation and (2) you are switching your code base to this new version. If you see that as a problem go with the alternative provided by Dan. I would never use such bulky code in a simple scenario like this as it doesn't add any benefit. – Devoid 17/8, 2012 at 6:6

Fair enough. It does seem strange that it takes ownership of disposing an object created and passed externally, however. I know it is highly bloody unlikely I'd use it externally in this specific context, but I figure if I created it outside that I should be responsible for choosing when it should be disposed. Thanks again for your help. I'd say both your answers are both correct in their own way. Accepting an answer is going to be a coin toss. – Hilliary 17/8, 2012 at 6:21

@JHubbard80: Agreed, I would have implemented StreamReader differently, too. This weird behaviour indeed has bitten me in the past, as I was getting ObjectDisposedExceptions when accessing the stream I hadn't yet disposed after disposing the StreamReader that used it... – Devoid 17/8, 2012 at 6:23

@exacerbatedexpert: Aha. Care to elaborate? – Devoid 18/8, 2012 at 5:55

@exacerbatedexpert: You need to learn to read properly. I wrote it dispose the FileStream. The one it was passed in. The context of this question was a FileStream being passed into a StreamReader... – Devoid 18/8, 2012 at 17:7

It's because the way you used StreamReader disposes the stream when it is disposed. So, if you dispose the stream too, it's being disposed twice. Some consider this a flaw in StreamReader--but it's there none-the-less. In VS 2012 (.NET 4.5) there is an option in StreamReader to not dispose of the stream, with a new constructor: http://msdn.microsoft.com/en-us/library/gg712952

Invalidity answered 17/8, 2012 at 16:6 Comment(2)

I also recommend to use the extended constructor to avoid this massive hassle. There are so many things that can go wrong between the point where the stream is assigned to the reader and the point it knows it should dispose of the passed in stream which could prevent the stream from being disposed at all. In this case Microsoft made a mistake in allowing the StreamReader to overstep its responsibility. It probably stems from the mentality to always dispose IDisposable resources, but the default in this case should be the opposite and the extended option should be to dispose if requested. – Sidonnie 20/12, 2017 at 17:41

Where this becomes more evident is the MemoryStream object. Very likely you DON'T want it disposed of even after it is read. Mostly used in unit testing, but it does prove a point. – Sidonnie 20/12, 2017 at 17:44

Two solutions:

A) You trust Reflector or Documentation and you know *Reader and *Writer will close the underlying *Stream. But warning: it won't work in case of a thrown Exception. So it is not the recommended way:

using (TextReader tr = new StreamReader(new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)))
{
    // Code here
}

B) You ignore the warning as documentation states The object must not throw an exception if its Dispose method is called multiple times. It's the recommended way, as it's both a good practice to always use using, and safe in case of a thrown Exception:

[SuppressMessage("Microsoft.Usage", "CA2202:Do not dispose objects multiple times")]
internal void myMethod()
{
    [...]
    using (FileStream fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite))
    using (TextReader tr = new StreamReader(fs))
    {
        // Code here
    }
}

Germin answered 18/4, 2014 at 16:52 Comment(0)

Given all the nonsense this (perfectly legitimate!) question generated, this would be my preference:

FileStream fs = null;
TextReader tr= null;
try
{
    fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
    tr= new StreamReader(fs);
    // Code here
}
finally
{
    if (tr != null)
        tr.Dispose();
    if (fs != null)
        fs.Dispose();
}

The links below illustrate perfectly legal syntax. IMO, this "using" syntax is far preferable to nested "using". But I admit - it does not solve the original question:

IMHO...

Cynic answered 17/8, 2012 at 19:8 Comment(4)

@palsm4 - (edit - I just realized you acknowledge this now, but to emphasize for future readers) this question began regarding visual studio 2012 warnings. The code above would still raise this warning. For reference I copy/pasted the code above into a test program to verify the warning still occurs: "CA2202 Do not dispose objects multiple times Object 'fs' can be disposed more than once [...]. To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object." This is because VS2012 knows streamreader is disposing of the filestream – Hilliary 17/8, 2012 at 19:33

So I'm guessing your solution is "try/finally: IN; using: OUT" ;) Correct? – Cynic 17/8, 2012 at 20:15

@paulsm4: -1: Your stacked usings still don't fix the warning and insulting others doesn't give you credibility – Devoid 18/8, 2012 at 6:0

1) "stacked usings" (2 lines + 1 set of braces) are merely an option. IMO a much more pleasing option than stupid, unnecessary "nesting" (with the extra braces). 2) "nesting" and "stacking" both generate the same IL byte code. We all agree on that :) 3) To the extent it doesn't solve the warning (and I said as much), I wouldn't bother with the stupid "using" at all. I'd just do everything with a (single!) try/dispose block. – Cynic 18/8, 2012 at 16:37

Hot tags

Godot Unity Godot Help Programming Godot 4.X GUI GDScript 3D 2D Physics CSharp Godot 3.X VR XR Projects C++

Recommended topics

Hot tags