9

The following code is used to stitch together existing PDFs [As an aside we are using TallComponents to do the actual stitching, in case you were wondering what PDFUtility is]:

PDFUtility.Document docFinal = new PDFUtility.Document();
PDFUtility.Document docToAdd = null;
byte[] combinedFile;

foreach (byte[] content in fileContents)
{
    MemoryStream fileContentStream = new MemoryStream(content);
    docToAdd = new PDFUtility.Document(fileContentStream);
    docFinal.Pages.AddRange(docToAdd.Pages.CloneToArray());
}
using (MemoryStream stream = new MemoryStream())
{
    docFinal.Write(stream);
    combinedFile = stream.ToArray();
}

The glaring problem with this code is this command:

MemoryStream fileContentStream = new MemoryStream(content);

The memory stream fileContentStream is not getting disposed, potentially (I believe) holding onto resoures longer than needed.

The obvious solution would be to wrap the creation of the MemoryStream in a using block. The code would then look like this:

PDFUtility.Document docFinal = new PDFUtility.Document();
PDFUtility.Document docToAdd = null;
byte[] combinedFile;

foreach (byte[] content in fileContents)
{
    using (MemoryStream stream = new MemoryStream())
    {
        MemoryStream fileContentStream = new MemoryStream(content);
        docToAdd = new PDFUtility.Document(fileContentStream);
        docFinal.Pages.AddRange(docToAdd.Pages.CloneToArray());
    }
}
using (MemoryStream stream = new MemoryStream())
{
    docFinal.Write(stream);
    combinedFile = stream.ToArray();
}

The use of the using block in the above code causes the code to fail on this line (because the streams were previously disposed):

docFinal.Write(stream);

One possible solution would be to keep track of all the MemoryStream instances and dispose of them after they are done being used. This is the code for that:

PDFUtility.Document docFinal = new PDFUtility.Document();
PDFUtility.Document docToAdd = byte[] combinedFile;
List<MemoryStream> streams = new List<MemoryStream>();
foreach (byte[] content in fileContents)
{
    MemoryStream fileContentStream = new MemoryStream(content);
    streams.Add(fileContentStream); //EACH INSTANCE OF A STREAM IS TRACKED
    docToAdd = new PDFUtility.Document(fileContentStream);
    docFinal.Pages.AddRange(docToAdd.Pages.CloneToArray());
}
using (MemoryStream stream = new MemoryStream())
{
    docFinal.Write(stream);
    combinedFile = stream.ToArray();
}
streams.ForEach(s => s.Dispose()); //DISPOSE OF ALL STREAMS HERE

The code above works. I am simply delaying the Dispose until after the the final document is written out.

However, this doesn't seem like the "best" solution. Is there any way to implement using blocks (and thus guaranteeing the objects are properly disposed?

user990423
  • 1,397
  • 2
  • 12
  • 32
Shai Cohen
  • 6,074
  • 4
  • 31
  • 54
  • Is there any kind of a `docFInal.Flush` method you can call before exiting the loop? – Tony Vitabile Nov 06 '15 at 21:55
  • See [MemoryStream must be explicitely be disposed?](http://stackoverflow.com/questions/4195746/memorystream-must-be-explicitely-be-disposed). – MicroVirus Nov 06 '15 at 21:57
  • 1
    And one more (seems to be more active) http://stackoverflow.com/questions/234059/is-a-memory-leak-created-if-a-memorystream-in-net-is-not-closed – shurik Nov 06 '15 at 21:58
  • @TonyVitabile: No, unfortunately there is not. – Shai Cohen Nov 06 '15 at 21:58
  • 1
    Put into a class which inherits IDisposible and then dispose the class. – jdweng Nov 06 '15 at 21:59
  • @jdweng: Maybe I'm missing something, but how would that differ from just using/disposing the MemoryStream directly? – Shai Cohen Nov 06 '15 at 22:03
  • In the custom dispose function you can delete dependent objects in an orderly fashion so you don't get a memory leak which may not occur using the default dispose method. – jdweng Nov 06 '15 at 22:07
  • Your solution of storing the objects in a list is fine. That said, you don't need to dispose `MemoryStream`. [Its `Dispose()` implementation](http://referencesource.microsoft.com/#mscorlib/system/io/memorystream.cs,0b83d17ca69bf8ea) does nothing useful; it just marks the object as disposed so that it behaves consistently with other stream types. There doesn't really seem to be a _problem statement_ here. If you have a specific, answerable problem, [please clarify your question](http://stackoverflow.com/help/how-to-ask). – Peter Duniho Nov 06 '15 at 22:19
  • Doesn't the `PDFUtility.Document` dispose of its streams when it's done? Does it have a `Dispose` method itself? If so, you could maybe use that. – MicroVirus Nov 06 '15 at 22:21
  • Whats wrong with `using (MemoryStream stream = new MemoryStream(content))` – D. Ben Knoble Nov 06 '15 at 23:22

1 Answers1

7

using blocks are not much more than syntactic sugar for try-finally blocks.

Depending on how the using block is used, you end up with two types of try-finally blocks.

Case 1:

// This code ...
using( var thing = new Thing() ) {
    thing.DoOperation();
}

// ... turns into this scoped try-finally:
{
    var thing = new Thing();
    try {
        thing.DoOperation();
    }
    finally {
        thing.Dispose();
        thing = null;
    }
}

Case two:

// This code ...
var thing = new Thing();
using( thing ) {
    thing.DoOperation();
}

// ... turns into this code
var thing = new Thing();
try {
    thing.DoOperation();
}
finally {
    thing.Dispose();
    // Note the lack of a null assignment.
}

With this knowledge, you can modify your third solution to use a finally block to ensure that your MemoryStream objects are always cleaned up.

PDFUtility.Document docFinal = new PDFUtility.Document();
PDFUtility.Document docToAdd = byte[] combinedFile;
List<MemoryStream> streams = new List<MemoryStream>();

try 
{
    foreach (byte[] content in fileContents)
    {
        MemoryStream fileContentStream = new MemoryStream(content);
        streams.Add(fileContentStream); //EACH INSTANCE OF A STREAM IS TRACKED
        docToAdd = new PDFUtility.Document(fileContentStream);
        docFinal.Pages.AddRange(docToAdd.Pages.CloneToArray());
    }
    using (MemoryStream stream = new MemoryStream())
    {
        docFinal.Write(stream);
        combinedFile = stream.ToArray();
    }

}
finally 
{
    streams.ForEach(s => s.Dispose()); //DISPOSE OF ALL STREAMS HERE
}
antiduh
  • 11,853
  • 4
  • 43
  • 66