tags:

views:

173

answers:

2

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?????

+3  A: 

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)) {
   ...
}
Foxfire
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
You should not wrap all three in usings but just the ns one that is in my response.
Foxfire
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
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
won't leaving a streamwriter or textreader open like that cause a memory leak?
Chris Conway
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
@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
@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
@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
@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
+2  A: 

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");
    }
}
Skywalker
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
This answer has the same problem as the original question.
Foxfire