Correct IDisposable implementation for this code

Question

I have the following code

public static byte[] Compress(byte[] CompressMe)
{
    using (MemoryStream ms = new MemoryStream())
    {
        using (GZipStream gz = new GZipStream(ms, CompressionMode.Compress,true))
        {
            gz.Write(CompressMe, 0, CompressMe.Length);
            ms.Position = 0;
            byte[] Result = new byte[ms.Length];
            ms.Read(Result, 0, (int)ms.Length);
            return Result;
        }
    }
}

This works fine, but when I run code analysis on it, it comes up with the following message

CA2202 : Microsoft.Usage : Object 'ms' can be disposed more than once in 
method 'Compression.Compress(byte[])'. To avoid generating a 
System.ObjectDisposedException you should not call Dispose more than one 
time on an object.

As far as I'm concerned, when the GZipStream is Disposed, it leaves the underlying Stream (ms) open, due to the last parameter of the constructor (leaveOpen=true).

If I change my code slightly.. remove the 'using' block around the MemoryStream and change the 'leaveOpen' parameter to false..

public static byte[] Compress(byte[] CompressMe)
{
    MemoryStream ms = new MemoryStream();
    using (GZipStream gz = new GZipStream(ms, CompressionMode.Compress, false))
    {
        gz.Write(CompressMe, 0, CompressMe.Length);
        ms.Position = 0;
        byte[] Result = new byte[ms.Length];
        ms.Read(Result, 0, (int)ms.Length);
        return Result;
    }
}

This then comes up with..

CA2000 : Microsoft.Reliability : In method 'Compression.Compress(byte[])',
object 'ms' is not disposed along all exception paths. Call 
System.IDisposable.Dispose on object 'ms' before all references to 
it are out of scope.

I can't win.. (unless I'm missing something obvious) I've tried various things, like putting a try/finally around the block, and Disposing of the MemoryStream in there, but it either says that I'm disposing of it twice, or not at all !!

Answer 1

You have to go old school:

public static byte[] Compress(byte[] CompressMe)
{
    MemoryStream ms = null;
    GZipStream gz = null;
    try
    {
        ms = new MemoryStream();
        gz = new GZipStream(ms, CompressionMode.Compress, true);
        gz.Write(CompressMe, 0, CompressMe.Length);
        gz.Flush();
        return ms.ToArray();
    }
    finally
    {
        if (gz != null)
        {
            gz.Dispose();
        }
        else if (ms != null)
        {
            ms.Dispose();
        }
    }
}

Looks horrible I know, but is the only way to appease the warning.

Personally, I dislike writing code like this, so just suppress the multiple dispose warning (where applicable) on the basis that Dispose calls should be idempotent (and is in this case).

Answer 2

That is sometimes the problem with running CodeAnalysis, you sometimes you simply cannot win and you have to choose the lesser evil™.

In this situation, I believe the correct implementation is the second example. Why? According to .NET Reflector, the implementation of GZipStream.Dispose() will dispose of the the MemoryStream for you as GZipStream owns the MemoryStream.

Relevant parts of GZipStream class below:

public GZipStream(Stream stream, CompressionMode mode, bool leaveOpen)
{
    this.deflateStream = new DeflateStream(stream, mode, leaveOpen, true);
}

protected override void Dispose(bool disposing)
{
    try
    {
        if (disposing && (this.deflateStream != null))
        {
            this.deflateStream.Close();
        }
        this.deflateStream = null;
    }
    finally
    {
        base.Dispose(disposing);
    }
}

As you wouldn't want to disable the rule entirely, you can suppress for this method only using using the CodeAnalysis.SupressMessage attribute.

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability ", "CA2000:?", Justification = "MemoryStream will be disposed by the GZipStream.")]

Note: You will have fill in the full rule name (i.e. CA2000:?) as I did not know what it was from the error message you posted.

HTH,

EDIT:

@CodeInChaos:

Looking deeper at the implementation DeflateStream.Dispose I believe it still will dispose of the MemoryStream for you regardless of the leaveOpen option as it calls the base.Dispose().

EDIT Ignore the above about DeflateStream.Dispose. I was looking at the wrong implementation in Reflector. See comments for details.

Answer 3

In reality effectively calling dispose twice on the memory stream won't cause any problems, it would be easy to code against this inside the MemoryStream class and on testing Microsoft appear to have. Therefore, if you didn't want to supress the rule another alternative is to split your method in two:

    public static byte[] Compress(byte[] CompressMe)
    {
        using (MemoryStream ms = new MemoryStream())
        {
            return Compress(CompressMe, ms);
        }
    }

    public static byte[] Compress(byte[] CompressMe, MemoryStream ms)
    {
        using (GZipStream gz = new GZipStream(ms, CompressionMode.Compress, true))
        {
            gz.Write(CompressMe, 0, CompressMe.Length);
            ms.Position = 0;
            byte[] Result = new byte[ms.Length];
            ms.Read(Result, 0, (int)ms.Length);
            return Result;
        }
    }

Answer 4

Apart from the disposing issue, your code is also broken. You should close the zip stream before reading back the data.

Also there is already a ToArray() method on MemoryStream, no need to implement that yourself.

using (MemoryStream ms = new MemoryStream())
{
    using (GZipStream gz = new GZipStream(ms, CompressionMode.Compress,true))
    {
        gz.Write(CompressMe, 0, CompressMe.Length);
    }
    return ms.ToArray();
}

I'd simply suppress the warning, since it's a false positive. Code analysis is there to serve you, not the other way round.

Answer 5

From this page in the MSDN

Stream stream = null;

try
{
    stream = new FileStream("file.txt", FileMode.OpenOrCreate);
    using (StreamWriter writer = new StreamWriter(stream))
    {
        stream = null;
        // Use the writer object...
    }
}
finally
{
    if(stream != null)
        stream.Dispose();
}

It is the try...finally that is missing from your solution that causes the second message.

If this:

GZipStream gz = new GZipStream(ms, CompressionMode.Compress, false)

fails the stream will not be disposed.