Dispose of object more than one time error. CA2202. Is there a better way?

Question

How can I ensure the following code is disposing of all objects in a better fashion? Currently, Code Analysis is telling me

Error 45 CA2202 : Microsoft.Usage : Object 'ns' can be disposed more than once in method 'CPCommunicator.GetResults(string)'. To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object.: Lines: 64, 65

NetworkStream ns = null;
StreamWriter requestStream = null;
TextReader responseStream = null;

var results = new StringBuilder();

try
{
    ns = new NetworkStream(CreateConnection(), true);
    requestStream = new StreamWriter(ns);
    requestStream.Flush();
    responseStream = new StreamReader(ns);

    requestStream.Write(reportData);
    requestStream.Flush();
    while (responseStream.Peek() != -1)
    {
        var currentLine = responseStream.ReadLine();
        results.Append(currentLine);
        results.Append("\n");
    }
}
finally
{
    if (requestStream != null) requestStream.Close();
    if (responseStream != null) responseStream.Close();
    if (cpNetworkStream != null) cpNetworkStream.Close();
}

Since both requestStream and responseStream use ns, they both dispose of ns so in order to satisfy the code analysis warning, I have to comment out the last two close methods in the finally block. But do I really want to do this?????

Although the recommendation is definitely good practice, the documentation for `Dispose` states that "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". http://msdn.microsoft.com/en-us/library/system.idisposable.dispose.aspx — LukeH, May 10 '10 at 14:33
@LukeH: The recommendation could be considered "good practice" for exactly one reason: Ideally, an object is disposed at the end of its useful life, and disposing it a second time could imply or even cause some confusion about its real vs intended lifetime. But as you said, correct implementation of `IDisposable` requires that `Dispose` be idempotent. — cHao, Oct 30 '12 at 21:52

score 4 · Accepted Answer · answered May 10 '10 at 14:28

4

Yes, imho you really should only call it once.

Alternatively you could use the using syntax on ns, which makes the whole situation even clearer.

using (ns = new NetworkStream(CreateConnection(), true)) {
   ...
}

answered May 10 '10 at 14:28

Foxfire

5,675
21
29

right, but i think the issue comes from trying to close requestStream and responseStream. I've wrapped all three in using statements and I still get the code analysis error. after requestStream closes and disposes, calling close on responseStream tries to close ns as well and that's what CA is yelling at me. – Chris Conway May 10 '10 at 14:34
You should not wrap all three in usings but just the ns one that is in my response. – Foxfire May 10 '10 at 15:06
are you suggesting that I don't close requestStream or responseStream, only the networkStream? wrapping just ns in a using statement and closing the other two manually still gives me the error. – Chris Conway May 10 '10 at 15:29
Yes, you shouldn't close the other ones. Closing them just closes the underlying stream. And in your case you already know that this stream is guaranteed to be closed. – Foxfire May 10 '10 at 15:36
won't leaving a streamwriter or textreader open like that cause a memory leak? – Chris Conway May 11 '10 at 00:44
Not closing/disposing an object NEVER EVER causes a memory leak. All memory will still be garbage collected. In your case it doesn't matter at all. The only potential disadvantage (not happening in your case) could be that if you don't call close/dispose it might take two garbage-collections (because in the first collection only the finalizer gets called) until the memory is freed. – Foxfire May 11 '10 at 11:49
@Foxfire: Stating that not disposing an object will "never ever" cause a memory leak is a bit misleading. Not disposing will never cause *managed* memory to leak, but it's quite possible for an object to use unmanaged memory and then release that memory if/when it's disposed. – LukeH May 14 '10 at 12:12
@LukeH: Even if it uses unmanaged memory that memory will still be freed as soon as the managed object gets collected (or one collection later). The only difference is that this will happen later than if you call Close (then it will happen right away). But that memory will not be "leaked". If the developer didn't call dispose, then the GC will call it at collection time. – Foxfire May 15 '10 at 13:10
1

@Foxfire: The GC *never ever* calls `Dispose`. The GC will eventually call an object's finaliser, so it's possible for an `IDisposable` object to provide a "fallback" finaliser which cleans up if the developer forgets to dispose, but that finaliser isn't part of the `IDisposable` contract (even if it is good practice). – LukeH May 16 '10 at 07:09
@LukeH: You are right that it does not call Dispose directly. But as you wrote it calls the finalizer and the finalizer in turn calls Dispose (at least for all framework classes). So indirectly it ALWAYS calls Dispose. Moreover the GC will not call it "eventually" but it is actually guaranteed that it will be called. Just the time when it happenes is not guaranteed. – Foxfire May 16 '10 at 23:28

score 2 · Answer 2 · answered May 10 '10 at 14:39

2

I would refactor your code to be like this:

using (NetworkStream ns = new NetworkStream(CreateConnection(), true))
using (StreamWriter requestStream = new StreamWriter(ns))
using (TextReader responseStream = new StreamReader(ns))
{

    var results = new StringBuilder();

    requestStream.Flush();

    requestStream.Write(reportData);
    requestStream.Flush();
    while (responseStream.Peek() != -1)
    {
        var currentLine = responseStream.ReadLine();
        results.Append(currentLine);
        results.Append("\n");
    }
}

answered May 10 '10 at 14:39

Skywalker

412
1
8
14

while certainly cleaner, I am still getting the same message about not calling dispose on ns more than one time on an object. Error 45 CA2202 : Microsoft.Usage : Object 'ns' can be disposed more than once in method 'Communicator.GetResults(string)'. To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object. – Chris Conway May 10 '10 at 15:03
3

This answer has the same problem as the original question. – Foxfire May 10 '10 at 15:07

Dispose of object more than one time error. CA2202. Is there a better way?

2 Answers2

Linked