0

I try to develop an application in C# and have some concerns with MailMessage object:

it implements IDisposable interface, so I use it within using statement. So it implicitly calls Dispose method after. Now, using that object I need to add attachments, which I have converted to byte[] object and I add them as stream. Here's part of code to have better view:

using(MailMessage message = new MailMessage("john.smith@gmail.com"){
    MemoryStream stream;
    //here I pass byte array to the stream and create an attachemnt
    message.Attachments.Add(new Attachment(stream, "name.xyz"));

    using(SmtpClient client = new SmtpClient("server.com", port))
    {
        // send message
    }
}

Now, I have one resource unmanaged: Stream object. I can't close it (so can't call Dispose method) right after setting attachments, because I'd get an error when sending message, because it uses the stream while sending.

So, I need to get rid of it later, which I do after sending. That's the code in second using:

try
{
    client.Send(messgae);
}
finally
{
    if(stream != null)
        stream.Dispose();
}

Now the question: Dispose method of MailMesssage frees all resources used by that object. My Stream object is one of the resources, isn't it? So, when using(MailMessage... terminates it should manage also my Stream object, shouldn't it? So I wouldn't need to dispose of my Stream object manually.

EDIT:

Suggested approach:

using(MailMessage message = new MailMessage("john.smith@gmail.com"){
    using(MemoryStream stream = ...)
    {
        //here I pass byte array to the stream and create an attachemnt
        message.Attachments.Add(new Attachment(stream, "name.xyz"));

        using(SmtpClient client = new SmtpClient("server.com", port))
        {
            // send message
        }
    }
}

But the questions stays: MailMessage uses this Stream - so, do we still need to manage Stream on our own?

Michał Turczyn
  • 32,028
  • 14
  • 47
  • 69
  • you need to use using for MemoryStream too, Steam isn't part of MailMessage – Sandip Bantawa Oct 18 '17 at 05:49
  • You may have a read on https://stackoverflow.com/questions/1065168/does-disposing-streamreader-close-the-stream. I´m not sure if that applies to `MailMessage` also but according to what you wrote I suspect it does. Also here´s another article: https://stackoverflow.com/questions/5252271/stream-closed-error-when-adding-attachment-to-mailmessage – MakePeaceGreatAgain Oct 18 '17 at 05:54
  • 1
    The `Dispose` implementation for `MemoryStream` basically does nothing. Is is completely safe not to `Dispose` of `MemoryStream` instances. – spender Oct 18 '17 at 05:54
  • 3
    @spender While that is probably true (didn't bother to check) it means that developers should get to know the **implementation** of whatever classes they use. Not only is it impossible, it goes against the encapsulation principle. Given an interface, you shouldn't be bothered to get yourself familiar with the concrete implementation. So even if the specific implementation of Dispose in MemoryStream does nothing (at the moment, implementation details like that might change in the future!), I would still not recommend not to dispose it. If something implements IDisposable it should be disposed. – Zohar Peled Oct 18 '17 at 05:58
  • @ZoharPeled Agreed. It's a bad habit and bad example not to dispose, especially when dev#2 comes in and replaces the MemoryStream with a FileStream (for example). However... it is safe to say that the implementation of MemoryStream will never change as it will never need to rely on unmanaged resources, as is the case for most other streams. – spender Oct 18 '17 at 06:04

3 Answers3

3

Why not dispose the stream after the message is sent?

using(MailMessage message = new MailMessage("john.smith@gmail.com"))
{
    using(var stream = new MemoryStream())
    {
        //here I pass byte array to the stream and create an attachemnt
        message.Attachments.Add(new Attachment(stream, "name.xyz"));

        using(SmtpClient client = new SmtpClient("server.com", port))
        {
        // send message
        }
    }
}
Zohar Peled
  • 79,642
  • 10
  • 69
  • 121
  • So, I guess answer to my question is no... :) – Michał Turczyn Oct 18 '17 at 05:57
  • I don't know and I can't be bothered to check. Not only is it an implementation detail, it also moves the responsibility of disposing your stream to another class. Anyway, as I wrote in my comment to spender, Whenever you are dealing with an IDisposable, you should be disposing it, either by explicitly call Dispose or with the using statement. That guarantees proper disposing of your instance even in future .Net versions. – Zohar Peled Oct 18 '17 at 06:02
  • Ok, accepted answer is exactly what I needed. Can you give me your opinion on that? I think it will be crucial to my future habits :) – Michał Turczyn Oct 18 '17 at 06:04
  • 1
    Already did. IMHO, disposing of an `IDisposable` should be expressed in your code, either by calling `Dispose` explicitly or with the using statement. Trusting the implementation of classes you didn't build and have no control over is a naive approach. Again, this is my opinion only, and I might be wrong about it, However, I say better safe then sorry. – Zohar Peled Oct 18 '17 at 06:05
2

Try this:

using(MailMessage message = new MailMessage("john.smith@gmail.com")
using(MemoryStream stream = new MemoryStream())
using(SmtpClient client = new SmtpClient("server.com", port))
{
    message.Attachments.Add(new Attachment(stream, "name.xyz"))
    client.Send(messgae);
}

If you put MemoryStream in using block, it will do the same thing as your try/finally block.

SᴇM
  • 7,024
  • 3
  • 24
  • 41
2

From the reference documentation you don't need to when you are using using.

Mail Message disposes Attachment Collection, which then disposes all its attachements.


Regarding, should we take or rely on this approach then I totally agree with Zohar on

Disposing of an IDisposable should be expressed in your code, either by calling Dispose explicitly or with the using statement.

ZerosAndOnes
  • 1,083
  • 1
  • 13
  • 25
  • Thank you, that's exactly what I needed. How did you find this answer, since it requires some browsing through objects and nested `Dispose` methods :) – Michał Turczyn Oct 18 '17 at 06:02
  • Google (mail message reference source) -> Ctrl + F (Dispose) -> Ctrl + F (AttachmentCollection) and finally Ctrl + F (Dispose) again. – ZerosAndOnes Oct 18 '17 at 06:03