4

I have the following code, which uses a stream to open and modify an Open XML document, and then save the new binary representation of that stream:

MemoryStream stream = null;
try
{
    stream = new MemoryStream();
    stream.Write(this.GetBinaryRepresentation(), 0, this.GetBinaryRepresentation().Length);

    using (WordprocessingDocument document = WordprocessingDocument.Open(stream, true))
    {
        OfficeDocument.ModifyDocument(document);
        this.SetBinaryRepresentation(stream.ToArray());
        stream = null;
    }
}
finally
{
    if (stream != null)
    {
        stream.Dispose();
    }
}

I had originally used two using blocks (one for the MemoryStream and the second for the WordprocessingDocument), but received warning CA2202: "Object 'stream' can be disposed more than once in method..." Per the MSDN article, I modified the code to above (converting the outer using to a try), but I am still receiving this warning.

I'm unsure of how I can structure this method to ensure that Dispose is called exactly once on the stream. I would prefer not to simply suppress this warning since the MSDN article states that you shouldn't rely on Dispose being safely callable multiple times.

Andrew Keller
  • 3,198
  • 5
  • 36
  • 51
  • 1
    Just a comment: How will you call `Dispose` on the stream when you null the reference in the using block? – Brian Rasmussen Jun 25 '12 at 15:39
  • @BrianRasmussen - consider stream.Write throwing an exception. In this case stream is not set to null and disposed in the finally block. – Henrik Jun 25 '12 at 15:43
  • @Henrik: Sure, but in the successful case, I can't see how `Dispose` is called. – Brian Rasmussen Jun 25 '12 at 15:47
  • The stream is disposed of by the using statement around document. When it goes out of scope, document.Dispose() will be called, which itself disposes of any underlying resources, including the stream that was passed in. – Jon Senchyna Jun 25 '12 at 15:56
  • @JonSenchyna Ahh, I didn't know the WordprocessingDocument class, so I couldn't tell if it handled the stream as well. Thanks for clarifying. – Brian Rasmussen Jun 25 '12 at 16:07

5 Answers5

6

Disposing of an object multiple times should always be safe. From the documentation for Dispose:

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.

That being said, a using statement is definitely the way to go here. The only reason you'd receive that method was if you were explicitly disposing of the object, which would not be required, as the using statement should always dispose the object exactly once.

Reed Copsey
  • 554,122
  • 78
  • 1,158
  • 1,373
  • Thanks, but according to the documentation on the Code Analysis rule, you're not supposed to rely on that: "Do not suppress a warning from this rule. Even if Dispose for the object is known to be safely callable multiple times, the implementation might change in the future." – Andrew Keller Jun 25 '12 at 15:49
  • 2
    @Andrew Which I think is ridiculous, as the contract says it should always be safe. That being said, there's no reason to dispose of it manually - just use the using statement and nothing else. – Reed Copsey Jun 25 '12 at 16:03
  • 1
    There's no way for them to enforce the contract, since IDisposable is an interface and they don't have control over every implementation of it. I do agree that it should at least always be true for Framework code. – Jon Senchyna Jun 25 '12 at 16:15
  • 3
    @JonSenchyna The thing is - anything that doesn't follow the contract is a bug - plain and simple. Writing all of your code to avoid 3rd party bugs, rather than fixing the bug, is something that is way overkill IMO. I'd rather trust the contract, and report violations of the contract than worry about avoiding a theoretical problem – Reed Copsey Jun 25 '12 at 16:42
3

The stream may still be disposed twice if an exception is thrown in the using block before stream is set to null. Try this:

MemoryStream stream = null;
MemoryStream streamToDispose = null;
try
{
    streamToDispose = stream = new MemoryStream();
    stream.Write(this.GetBinaryRepresentation(), 0, this.GetBinaryRepresentation().Length);

    using (WordprocessingDocument document = WordprocessingDocument.Open(stream, true))
    {
        streamToDispose = null;
        OfficeDocument.ModifyDocument(document);
        this.SetBinaryRepresentation(stream.ToArray());
    }
}
finally
{
    if (streamToDispose != null)
    {
        streamToDispose.Dispose();
    }
}
Henrik
  • 23,186
  • 6
  • 42
  • 92
  • You could also simply get stream.ToArray() beforehand, and then set the original stream to null as soon as you enter the using block. Setting a reference to null oes not change the state of the underlying object that is used by yoru WordprocessingDocument. – Jon Senchyna Jun 25 '12 at 16:17
3

The reason that the example from the MSDN article did not work for you is that they set the stream to null as soon as they enter the using block, whereas you use the stream inside your using block and set the stream to null after. If an exception is thrown before your stream = null statement, stream would be disposed of as the using block is exited, and then again in your finally block.

Unfortunately, since you need to access your stream after document has updated it, I don't see a clean way to use their example of setting stream = null within your using statement to avoid the multiple Dispose() calls. An alternative would be to you could declare both stream and document outside of the try block, and then clean both of them up inside your finally, like so:

MemoryStream stream = null;
WordprocessingDocument document = null;
try
{
    stream = new MemoryStream();
    stream.Write(this.GetBinaryRepresentation(), 0, this.GetBinaryRepresentation().Length);

    document = WordprocessingDocument.Open(stream, true));

    OfficeDocument.ModifyDocument(document);
    this.SetBinaryRepresentation(stream.ToArray()); 
}
finally
{
    if( document != null)
    {
        document.Dispose();
    }
    // Catch the case where an error occurred before document was defined.
    else
    {
        stream.Dispose();
    }
}
Jon Senchyna
  • 7,867
  • 2
  • 26
  • 46
  • I don't think this can work, the OfficeDocument.ModifyDocument method is changing the stream, and according to the MemoryStream.ToArray() documentation (http://msdn.microsoft.com/en-us/library/system.io.memorystream.toarray.aspx) the ToArray returns a copy of the array at that point in time, so changes to the stream afterwords would not be reflected in binaryRepresentation. I did give this a try anyway but the unit tests failed bc BinaryRepresentation still contained the original document. – Andrew Keller Jun 25 '12 at 16:32
  • Ah, you are correct. I didn't read the contents of your using statement closely enough. – Jon Senchyna Jun 25 '12 at 17:15
  • I updated my answer to use my original suggestion of a single try/finally block, rather than a try/finally with a nested using block. That should take care of your issues without having to use clever manipulation of references. – Jon Senchyna Jun 25 '12 at 17:27
2

The using statement disposes the object - so essentially you are calling dispose twice

Charleh
  • 13,749
  • 3
  • 37
  • 57
  • Yeah I was thinking that maybe WordProcessingDocument was wrapping the dispose for you but maybe it's not...! – Charleh Jun 25 '12 at 15:58
0

When your code leaves the using block around the WordProcessingDocument, it will call dispose.

using (WordprocessingDocument document = WordprocessingDocument.Open(stream, true))

Since the WordProcessingDocument takes an instance of stream in its constructor, it will call dispose on that stream instance when WordProcessingDocument.Dispose is called. You then enter the finally block where you call stream.Dispose() - you have now called Dispose() on the stream instance twice.

RobertMS
  • 1,153
  • 11
  • 17