Code analysis complains I'm not disposing objects. What is wrong here?

Question

Consider this code

private MailMessage GetMailMessageFromMailItem(Data.SystemX.MailItem mailItem)
        {

            var msg = new MailMessage();

            foreach (var recipient in mailItem.MailRecipients)
            {
                var recipientX = Membership.GetUser(recipient.UserKey);
                if (recipientX == null)
                {
                    continue;
                }

                msg.To.Add(new MailAddress(recipientX.Email, recipientX.UserName));
            }

            msg.From = new MailAddress(ConfigurationManager.AppSettings["EmailSender"],
                                   ConfigurationManager.AppSettings["EmailSenderName"]);

            msg.Subject = sender.UserName;
            if (!string.IsNullOrEmpty(alias)) msg.Subject += "(" + alias + ")";
            msg.Subject += " " + mailItem.Subject;
            msg.Body = mailItem.Body;
            msg.Body += Environment.NewLine + Environment.NewLine + "To reply via Web click link below:" + Environment.NewLine;
            msg.Body += ConfigurationManager.AppSettings["MailPagePath"] + "?AID=" + ContextManager.AccountId + "&RUN=" + sender.UserName;

            if (mailItem.MailAttachments != null)
            {
                foreach (var attachment in mailItem.MailAttachments)
                {
                    msg.Attachments.Add(new Attachment(new MemoryStream(attachment.Data), attachment.Name));
                }
            }

            return msg;
        }

I'm just taking my database type and converting to MailMessage. It get's sent in another function.

Code analysis tells me I'm not disposing "msg" which is correct. But if I do it here - I get exception when I'm trying to send it.

Also, it complains about not disposing MemoryStream here:

msg.Attachments.Add(new Attachment(new MemoryStream(attachment.Data), attachment.Name));

I have no idea on how to properly dispose it. I tried different things but was getting exceptions when sending mail saying "Stream is closes"

Jon Skeet · Accepted Answer · 2011-08-19T19:34:27.250

2

Basically you shouldn't - disposing of the mail message later will dispose of each attachment, which will dispose of each stream. Besides, failing to dispose of a MemoryStream which isn't being used in remoting won't do any harm.

I suggest you suppress the warning for this method.

EDIT: I suspect you can use [SuppressMessage] to suppress the message.

Note that there's a risk that some code will throw code half way through the method, so you end up never being able to dispose of the message even if you have a using statement in the calling code. If you're really bothered, you could write:

private MailMessage GetMailMessageFromMailItem(Data.SystemX.MailItem mailItem)
{
    bool success = false;
    var msg = new MailMessage();
    try
    {
        // Code to build up bits of the message
        success = true;
        return msg;
    }
    finally
    {
        if (!success)
        {
            msg.Dispose();
        }
    }
}

Personally I'd say this is overkill though.

edited Aug 19 '11 at 19:34

answered Aug 19 '11 at 17:41

Jon Skeet

1,421,763
867
9,128
9,194

@katit: Pass - I don't use code analysis. I'm sure there are plenty of instructions online though. – Jon Skeet Aug 19 '11 at 18:01
@katit: Have edited the answer with a suggestion for suppression. – Jon Skeet Aug 19 '11 at 19:34
Yes, thanks! It was first post in google on how to :) It is easy to supress by right-click on that warning in VS and choose "Suppress in code" :) – katit Aug 19 '11 at 20:41
This is a legitimate catch from the code analysis tool, recommending suppression does not seem right. – justin.m.chase Aug 20 '11 at 22:11
@justin.m.chase: For the memory stream at least, not disposing it will do no harm. What unmanaged resources do you believe a MailMessage will allocate before it's sent? I agree it shouldn't be suppressed *in general* but I think in this case it's okay. – Jon Skeet Aug 20 '11 at 22:19
@Jon: Ok haha. I guess I was thinking more of the general case. According to reflector the Dispose method on MailMessage disposes a number of streams, including attachments and such. It also seems to consider them to be managed resources rather than unmanaged so you are probably right, that in this case it will not matter. What if the attachments are really large? My only other argument would be that its good to get in the habit of doing whats right in the general case. – justin.m.chase Aug 20 '11 at 22:32
@justin: If they were being loaded from a `FileStream`, that would make perfect sense... but it would be harder to deal with, too. Basically anything which transfers ownership is tricky like this. – Jon Skeet Aug 20 '11 at 22:33

Jethro · Answer 2 · 2011-08-19T17:56:10.153

0

With regards to "not disposing "msg"", the only way I can think of is instead of returning MailMessage rather pass in a reference to a MailMessage. Something like this. Not sure if it's such a good idea.

private void GetMailMessageFromMailItem(ref MailMessage msg, Data.SystemX.MailItem mailItem)

edited Aug 19 '11 at 17:56

answered Aug 19 '11 at 17:42

Jethro

5,896
3
23
24

I suspect that the stream will be stored in the attachment and only read later on. – Jon Skeet Aug 19 '11 at 17:52
@Jon Skeet, now that I think about it, the stream would need to stay open. – Jethro Aug 19 '11 at 17:55

score -1 · Answer 3 · answered Aug 19 '11 at 19:33

-1

The creator of the disposable object should also dispose it. If you can't dispose the message here then it should be passed in from a creator somewhere else. Code analysis is right in this case and you can end up with very unfortunate and hard to debug leaks if you ignore these messages.

answered Aug 19 '11 at 19:33

justin.m.chase

13,061
8
52
100

Ownership transfer is a very useful concept, which unfortunately neither C# nor Code Analysis provide help with. – Ben Voigt Aug 19 '11 at 20:08
Therefore it is to be avoided. Thus it is wise to assign the creator responsibility for disposing it's disposable resources. – justin.m.chase Aug 20 '11 at 22:10
The creator is not always the logical owner. For example, in the factory pattern, the creator is never the owner, nor can the factory dispose the resource it creates. Instead, an undisposed resource must be returned. – Ben Voigt Aug 20 '11 at 22:33
Unless all disposable resources are passed to the factory and the object created by the factory is not itself disposable. – justin.m.chase Aug 20 '11 at 22:36
If the object created by the factory owns disposable objects, it needs to be disposable in order to ensure that they are disposed at the correct time. – Ben Voigt Aug 20 '11 at 22:38
So it shouldn't own any, they should be passed to it instead, since it's creator cannot also dispose it. The owner of those disposable resources will dispose them at the correct time. For example, pass a Stream into the factory instead of passing a path. – justin.m.chase Aug 20 '11 at 22:45
But often it isn't natural for the lifetime of every object to be bounded by the duration of the method that creates it. You certainly can't have that and a single message loop. Just look at the constructor of any WinForms window... all the subcontrols are created inside the constructor, but it wouldn't make sense to also dispose them inside the constructor. Components would be much less reusable if their lifetimes had to be strictly nested, instead of arbitrarily overlapping. – Ben Voigt Aug 21 '11 at 02:45

Code analysis complains I'm not disposing objects. What is wrong here?

3 Answers3