none
how to correctly dispose MailMessage and smtpclient objects RRS feed

  • Question

  • I use this class to create mail message and then send the email:

    AlternateView AlterView(string content, string contentId, string filePath)
        {
            AlternateView av = null;
            LinkedResource lr = null;
    
            try
            {
                using (av = AlternateView.CreateAlternateViewFromString(content, null, MediaTypeNames.Text.Html))
                {
                    using (lr = new LinkedResource(filePath, MediaTypeNames.Image.Jpeg))
                    {
                        lr.ContentId = contentId;
                        av.LinkedResources.Add(lr);
                    }
                }
            }
            catch (Exception ex)
            {
                Main_Log.GetLogger().Error(Main_Log.MessageForLogFile("creating email alternative view process failed due to the exception in application. ", ex.Message, ex.HResult));
            }
    
            return av;
        }
        Attachment Attach(string fileName, string filePath)
        {
            FileStream fs = null;
            Attachment a = null;
    
            try
            {
                using (fs = new FileStream(filePath, FileMode.Open, FileAccess.Read))
                {
                    using (a = new Attachment(fs, fileName, MediaTypeNames.Application.Octet)) ;
                }
            }
            catch (Exception ex)
            {
                Main_Log.GetLogger().Error(Main_Log.MessageForLogFile("email file attaching process failed due to the exception in application. ", ex.Message, ex.HResult));
            }
    
            return a;
        }  
        string Send(MailMessage message)
        {
            using (SmtpClient smtp = new SmtpClient())
            {
                smtp.Host = smtpAddress;
                smtp.Port = smtpPortNumber;
                smtp.EnableSsl = enableSSL;
                smtp.Credentials = new NetworkCredential(emailFrom, emailFromPassword);
    
                try
                {
                    smtp.Send(message);
                    return Main_Log.ApplicationSuccessMessage;
                }
                catch (Exception ex)
                {
                    Main_Log.GetLogger().Error(Main_Log.MessageForLogFile("email sending process failed due to the exception in application. ", ex.Message, ex.HResult));
                    return Main_Log.ApplicationFailureMessage;
                }
            }
        }
        MailMessage Message(string body, string to)
        {
            MailMessage message = new MailMessage();
    
            message.IsBodyHtml = isBodyHtml;
            message.From = new MailAddress(emailFrom, emailFromDisplayName);
            message.To.Add(new MailAddress(to, to));
            //message.CC.Add(new MailAddress(emailCC, emailCCDisplayName));
            //message.Bcc.Add(new MailAddress(emailBCC, emailBCCDisplayName));
            message.Subject = emailSubject;
            message.Body = body;
            //message.Attachments.Add(Attach("", ""));
            //message.AlternateViews.Add(AlterView("","",""));
    
            return message;
        }
        public string SendEmail(string body, string to)
        {
            MailMessage message = new MailMessage();
    
            message = Message(body, to);
            return Send(message);
        }

    now i have questions about this class:

    1- after this line in attach method: using (a = new Attachment(fs, fileName, MediaTypeNames.Application.Octet)) ; this warning keeps showing up and i have no idea why: Possible mistaken empty statement

    2- did i correctly use dispose method in my class?

    3- how can set timer for sending email process? i mean i dont want sending process take for ever in case of slow connection or other possible exceptions.

    this class works fine but i guess its not the good enough. i really appreciated if you can help me to improve it. thank you. any


    Saturday, July 4, 2015 6:44 AM

Answers

  • 2. No you did not dispose things correctly. You're supposed to dispose objects that are no longer in use and you dispose attachements & co. while they're still in use. It's problematic to use "using" correctly when the object to be disposed is returned from a method.

    I would suggest that you place all the email sending code in the SendEmail method. It's just not worth it to create 2 different methods to that create attachments and doing so complicates the cleanup story.

    You also forgot to dispose the MailMessage objects (yes, objects as you create 2 of them and one of them isn't used anywhere).

    3. The SmtpClient class has a Timeout property that defaults to 100 seconds (expressed in milliseconds), feel free to adjust that to a lower value. Note however that this timeout starts from the moment when the client successfully connects to the server. Before that there's a socket connection timeout that cannot be controlled. You'll notice that if you attempt to send email to a dead mail server.

    • Marked as answer by Bouki Saturday, July 4, 2015 9:13 AM
    Saturday, July 4, 2015 8:58 AM
    Moderator
  • 1. It is correctly pointing out that this using does nothing:

    using (a = new Attachment(fs, fileName, MediaTypeNames.Application.Octet));

    Extra semikolons are a common issue for broken control structures. In this case the using ends at the character and would NOT include any brakets put afterwards. So this warning makes a lot of sense.

    Of course in this case it seems like that is intentional. Note however that the JIT-Compiler will propably just cut that line out if you don't do anything with a.

    2. For all intents using is just a shorthand to write a try...finally (if not null dispose) block. Like a normal finally Dipose will run in case of exception, in case of normal execution and in case of return.
    Create, use, Dipose is the most solid pattern.
    I am uncertain if you hit all stuff that is disposeable, as I usually do not work with those classes.

    3. You mean a timeout? It involves at least two alternate threads in addition to the UI thread:
    Nr. 2 does the actuall work and returns to Nr 1.
    Nr. 1 polls for Nr. 2 to return and looks on the clock (interchangeable), to end Nr. 2 if it takes too long and return a Timeout exception.

    There might be some build in way to do that, but you won't get around multithreading.

    4. A pet peeve of mine is Exception Handling, wich is potentially faulty in your code.
    One is you catch Exception. NEVER do that except for logging and throwing on. Right now you catch Fatal Exception like OutOfMemory. Wich your can not (could not possibly) handle.
    The second issue is that you only log the Exception Message. Always log ex.ToString(). As it include the other 95% of Exception information.

    Vexing exceptions - Fabulous Adventures In Coding - Site Home - MSDN Blogs

    Exception Handling Best Practices in .NET - CodeProject


    • Edited by Christopher84 Saturday, July 4, 2015 8:05 AM
    • Marked as answer by Bouki Saturday, July 4, 2015 9:13 AM
    Saturday, July 4, 2015 8:04 AM
  • try
    {
        smtp.Send(message);
        // SendEmail already does this
        DisposeMessage(message);
        return Main_Log.ApplicationSuccessMessage;
    }
    catch (Exception ex)

    DisposeMessage should be in Send or SendEmail but not in both places. And it should be in a finally block.

    foreach (Attachment attachment in message.Attachments)
        attachment.Dispose();
    // you did not create the Attachments collection
    // it's not your job to dispose it
    message.Attachments.Dispose();
    // this is useless
    message = null;

    I think that's all in relation to dispose. There are plenty of other issues in your code:

    // this if is useless
    if (attachments.Length > 0)
        foreach (string attachment in attachments)
    // Consider using another name, SendActivationCode or something like that
    // It's too easy to confuse this with a general purpose SendEmail method and send activation emails with bogus codes
    public string SendEmail(string activationCode, string to, string toDisplayName)
    // there's no point in specifying an email address as display name
    message.To.Add(new MailAddress(to, to));
    Attachment attach = Attach(attachment);
    // The question is, do you really want the continue sending 
    // the email without attachment? The caller of this method 
    // would probably expect that if the method returns successfully 
    // then the email has been sent together with its attachments.
    if (attach != null)




    • Edited by Mike DanesModerator Saturday, July 4, 2015 10:20 AM
    • Marked as answer by Bouki Saturday, July 4, 2015 12:43 PM
    Saturday, July 4, 2015 10:20 AM
    Moderator

All replies

  • 1. It is correctly pointing out that this using does nothing:

    using (a = new Attachment(fs, fileName, MediaTypeNames.Application.Octet));

    Extra semikolons are a common issue for broken control structures. In this case the using ends at the character and would NOT include any brakets put afterwards. So this warning makes a lot of sense.

    Of course in this case it seems like that is intentional. Note however that the JIT-Compiler will propably just cut that line out if you don't do anything with a.

    2. For all intents using is just a shorthand to write a try...finally (if not null dispose) block. Like a normal finally Dipose will run in case of exception, in case of normal execution and in case of return.
    Create, use, Dipose is the most solid pattern.
    I am uncertain if you hit all stuff that is disposeable, as I usually do not work with those classes.

    3. You mean a timeout? It involves at least two alternate threads in addition to the UI thread:
    Nr. 2 does the actuall work and returns to Nr 1.
    Nr. 1 polls for Nr. 2 to return and looks on the clock (interchangeable), to end Nr. 2 if it takes too long and return a Timeout exception.

    There might be some build in way to do that, but you won't get around multithreading.

    4. A pet peeve of mine is Exception Handling, wich is potentially faulty in your code.
    One is you catch Exception. NEVER do that except for logging and throwing on. Right now you catch Fatal Exception like OutOfMemory. Wich your can not (could not possibly) handle.
    The second issue is that you only log the Exception Message. Always log ex.ToString(). As it include the other 95% of Exception information.

    Vexing exceptions - Fabulous Adventures In Coding - Site Home - MSDN Blogs

    Exception Handling Best Practices in .NET - CodeProject


    • Edited by Christopher84 Saturday, July 4, 2015 8:05 AM
    • Marked as answer by Bouki Saturday, July 4, 2015 9:13 AM
    Saturday, July 4, 2015 8:04 AM
  • 2. No you did not dispose things correctly. You're supposed to dispose objects that are no longer in use and you dispose attachements & co. while they're still in use. It's problematic to use "using" correctly when the object to be disposed is returned from a method.

    I would suggest that you place all the email sending code in the SendEmail method. It's just not worth it to create 2 different methods to that create attachments and doing so complicates the cleanup story.

    You also forgot to dispose the MailMessage objects (yes, objects as you create 2 of them and one of them isn't used anywhere).

    3. The SmtpClient class has a Timeout property that defaults to 100 seconds (expressed in milliseconds), feel free to adjust that to a lower value. Note however that this timeout starts from the moment when the client successfully connects to the server. Before that there's a socket connection timeout that cannot be controlled. You'll notice that if you attempt to send email to a dead mail server.

    • Marked as answer by Bouki Saturday, July 4, 2015 9:13 AM
    Saturday, July 4, 2015 8:58 AM
    Moderator
  • thank you so much guys. as always helpful.

    i change few things and i guess its better now. would you please take look at it:

            private string Send(MailMessage message)
            {
                using (SmtpClient smtp = new SmtpClient())
                {
                    smtp.Host = smtpAddress;
                    smtp.Port = smtpPortNumber;
                    smtp.EnableSsl = enableSSL;
                    smtp.Timeout = timeout;
                    smtp.Credentials = new NetworkCredential(emailFrom, emailFromPassword);
    
                    try
                    {
                        smtp.Send(message);
                        DisposeMessage(message);
                        return Main_Log.ApplicationSuccessMessage;
                    }
                    catch (Exception ex)
                    {
                        Main_Log.GetLogger().Error(Main_Log.MessageForLogFile("email sending process failed due to the exception in application. ", ex.Message, ex.HResult));
                        return Main_Log.ApplicationFailureMessage;
                    }
                }
            }
            private void DisposeMessage(MailMessage message)
            {
                foreach (Attachment attachment in message.Attachments)
                    attachment.Dispose();
                message.Attachments.Dispose();
                message = null;
            }
            
            private Attachment Attach(string fileName)
            {
                try
                {
                    Attachment attach = new Attachment(fileName, MediaTypeNames.Application.Octet);
                    return attach;
                }
                catch (Exception ex)
                {
                    Main_Log.GetLogger().Error(Main_Log.MessageForLogFile("email file attaching process failed due to the exception in application. ", ex.Message, ex.HResult));
                    return null;
                }
            }
            private MailMessage Message(string activationCode, string to, string toDisplayName)
            {
                MailMessage message = new MailMessage();
    
                message.IsBodyHtml = isBodyHtml;
                message.From = new MailAddress(emailFrom, emailFromDisplayName);
                message.To.Add(new MailAddress(to, toDisplayName));
                message.Subject = "Send Activation Code";
                message.Body = "Your Activation Code Is: " + activationCode;
    
                return message;
    
            }
            private MailMessage Message(string subject, string body, string to, string cc, string bcc, string[] attachments)
            {
                MailMessage message = new MailMessage();
    
                message.IsBodyHtml = isBodyHtml;
                message.From = new MailAddress(emailFrom, emailFromDisplayName);
                message.To.Add(new MailAddress(to, to));
    
                if (!string.IsNullOrEmpty(cc))
                    message.CC.Add(new MailAddress(cc, cc));
    
                if (!string.IsNullOrEmpty(bcc))
                    message.Bcc.Add(new MailAddress(bcc, bcc));
    
                if (attachments.Length > 0)
                    foreach (string attachment in attachments)
                    {
                        Attachment attach = Attach(attachment);
                        if (attach != null)
                            message.Attachments.Add(attach);
                    }
    
                message.Subject = subject;
                message.Body = body;
    
                return message;
            }
    
            public string SendEmail(string activationCode, string to, string toDisplayName)
            {
                MailMessage message = Message(activationCode, to, toDisplayName);
                string status = Send(message);
                DisposeMessage(message);
                return status;
            }
            public string SendEmail(string subject, string body, string to, string cc, string bcc, string[] attachments)
            {
                MailMessage message = Message(subject, body, to, cc, bcc, attachments);
                string status = Send(message);
                DisposeMessage(message);
                return status;
            }

    Saturday, July 4, 2015 9:17 AM
  • try
    {
        smtp.Send(message);
        // SendEmail already does this
        DisposeMessage(message);
        return Main_Log.ApplicationSuccessMessage;
    }
    catch (Exception ex)

    DisposeMessage should be in Send or SendEmail but not in both places. And it should be in a finally block.

    foreach (Attachment attachment in message.Attachments)
        attachment.Dispose();
    // you did not create the Attachments collection
    // it's not your job to dispose it
    message.Attachments.Dispose();
    // this is useless
    message = null;

    I think that's all in relation to dispose. There are plenty of other issues in your code:

    // this if is useless
    if (attachments.Length > 0)
        foreach (string attachment in attachments)
    // Consider using another name, SendActivationCode or something like that
    // It's too easy to confuse this with a general purpose SendEmail method and send activation emails with bogus codes
    public string SendEmail(string activationCode, string to, string toDisplayName)
    // there's no point in specifying an email address as display name
    message.To.Add(new MailAddress(to, to));
    Attachment attach = Attach(attachment);
    // The question is, do you really want the continue sending 
    // the email without attachment? The caller of this method 
    // would probably expect that if the method returns successfully 
    // then the email has been sent together with its attachments.
    if (attach != null)




    • Edited by Mike DanesModerator Saturday, July 4, 2015 10:20 AM
    • Marked as answer by Bouki Saturday, July 4, 2015 12:43 PM
    Saturday, July 4, 2015 10:20 AM
    Moderator