tags:

views:

441

answers:

5

I am setting up a subscription service to send reports to various people in our company on a schedule. I plan to email the reports, the reporting system I am using is able to export as PDF stream (rather than writing temp files). Most people will receive more than one report so I am trying to attach them all to one email doing something like

List<Stream> reports = new List<Stream>();
//looping code for each users set of reports
Stream stream = ReportSource.ReportDocument.ExportToStream(PortableDocFormat)
reports.Add(stream);
stream.Flush();  //unsure
stream.Close();  //unsure
//end looping code

SmtpClient smtpClient = new SmtpClient(host, port);
MailMessage message = new MailMessage(from, to, subject, body);

foreach (Stream report in reports)
{
    message.Attachments.Add(new Attachment(report, "application/pdf"));
}                
smtpClient.Send(message);

What I am unsure about is should I be flushing and closing the stream just after adding it to the list will this be ok? Or do I need to loop the List afterwards to flush and dispose? I am trying to avoid any memory leak that is possible.

A: 

There is no harm whatsoever in doing Flush()/Close(). If you want to be absolutely sure, then you should do a using statement:

using (Stream stream = ReportSource.ReportDocument.ExportToStream(PortableDocFormat))
{
    reports.Add(stream);
    stream.Flush();  //unsure
}

This way, exceptions will not affect your code.

Mark Bertenshaw
+1  A: 

Depends on if the streams are used later on when the attachment is created. I assume they are which means you'll want to dispose the streams at the end.

Remember to try-finally this. Otherwise they wont be disposed if an exception occurs.

Quibblesome
I was just leaving the code simple for the example, know to do try finallys and all that.
PeteT
+10  A: 

Why not create a StreamCollection class that implements IDisposable:

public class StreamCollection : Collection<Stream>, IDisposable { }

In the Dispose method of that class, you could loop through all of the streams and properly Close/Dispose of each stream. Then your code would look like:

using (var reports = new StreamCollection())
{
   //looping code for each users set of reports
   reports.Add(ReportSource.ReportDocument.ExportToStream(PortableDocFormat));
   //end looping codeSmtpClient 

   smtpClient = new SmtpClient(host, port);
   MailMessage message = new MailMessage(from, to, subject, body);

   foreach (Stream report in reports)
   {    
      message.Attachments.Add(new Attachment(report, "application/pdf"));
   }

   smtpClient.Send(message);
}
mkedobbs
pwetty! +1. 15chars
Quibblesome
You could make StreamCollection generic, then it can be used for any IDisposable object
thecoop
Like that approach will use it
PeteT
+1  A: 

I do not see the logic in closing the streams right after adding it to the list. Based on the code you have provided it appears the references to those streams are being used in other places. If the streams are closed then what good do they serve?

Brian Gideon
A: 

You could create a DisposableList that you can wrap in a using statement:

public class DisposableList<T> : List<T>, IDisposable where T : IDisposable {

    // any constructors you need...

    public void Dispose() {
        foreach (T obj in this) {
            obj.Dispose();
        }
    }
}
thecoop