• February 2011
    M T W T F S S
     123456
    78910111213
    14151617181920
    21222324252627
    28  

  • Latest Posts

  • Latest Comments

  • Archives

SmtpClient.Dispose() Bummer

Headline: You will need to put in some try/catch logic when you call dispose on the SmtpClient.

I was working on getting rid of some of my code analysis warnings – “Warning    10    CA2000 : Microsoft.Reliability : In method ‘AccountMembershipService.SendActivationEmail(HttpContextBase, string, Guid, string, string)’, object ‘client’ is not disposed along all exception paths. Call System.IDisposable.Dispose on object ‘client’ before all references to it are out of scope. ”.

The original code looked like this:

Do NOT Dispose
  1. var client = new SmtpClient();
  2. // Fill out the message template that is stored in the Activate.txt file.
  3. using (var message = new TemplatedMailMessage(webmasterEmail, model.Email, templateFilePath, model))
  4. {
  5.     // Add the webmaster to the bcc.
  6.     message.Bcc.Add(webmasterEmail);
  7.     client.Send(message);
  8. }

However, when I changed it to this:

Dispose
  1. using (var client = new SmtpClient())
  2. {
  3.     // Fill out the message template that is stored in the Activate.txt file.
  4.     using (var message = new TemplatedMailMessage(webmasterEmail, model.Email, templateFilePath, model))
  5.     {
  6.         // Add the webmaster to the bcc.
  7.         message.Bcc.Add(webmasterEmail);
  8.         client.Send(message);
  9.     }
  10. }

And had the following in my App.config file:

App.config Mail Settings
  1. <!–This is used to just test the e-mail locally rather than actually sending the message via SMTP–>
  2. <system.net>
  3.   <mailSettings>
  4.     <smtp deliveryMethod="SpecifiedPickupDirectory">
  5.       <specifiedPickupDirectory pickupDirectoryLocation="M:\DropFolder" />
  6.     </smtp>
  7.   </mailSettings>
  8. </system.net>

I got this error message:

Test method KarlZMvc.Tests.Models.AccountModelsTest.CreateUser_Works threw exception:
System.InvalidOperationException: The SMTP host was not specified.

Sure enough, when you look in the code for Dispose you see the following trail:

  • Dispose calls Dispose(bool disposing)…
  • Dispose(bool disposing) calls this.transport.CloseIdelConnections(this.ServicePoint)…
  • ServicePoint calls this.CheckHostAndPort()…
  • CheckHostAndPort() looks like this:
Seriously?
  1. private void CheckHostAndPort()
  2. {
  3.     if ((this.host == null) || (this.host.Length == 0))
  4.     {
  5.         throw new InvalidOperationException(SR.GetString("UnspecifiedHost"));
  6.     }
  7.     if ((this.port <= 0) || (this.port > 0xffff))
  8.     {
  9.         throw new InvalidOperationException(SR.GetString("InvalidPort"));
  10.     }
  11. }

Seems pretty bad practice to throw exceptions in the Dispose method to me. So now I need to work out a compromise… How can I write quality code that will pass unit tests and follow best practices around the Dispose pattern?

Here is what I finally ended up with that 1) allowed my unit tests to pass, and 2) had no code analysis warnings, but 3) still makes me wince.

try/catch around Dispose()
  1. try
  2. {
  3.     using (var client = new SmtpClient())
  4.     {
  5.         // Fill out the message template that is stored in the Activate.txt file.
  6.         using (var message = new TemplatedMailMessage(webmasterEmail, model.Email, templateFilePath, model))
  7.         {
  8.             // Add the webmaster to the bcc.
  9.             message.Bcc.Add(webmasterEmail);
  10.             client.Send(message);
  11.         }
  12.     }
  13. }
  14. catch (InvalidOperationException)
  15. {
  16.     // I don't like swallowing this exception,
  17.     // but Microsoft really leaves me no choice.
  18. }

Revisit “Don’t Throw Exceptions in Dispose()”

So as I was typing the solution to my problem I was again troubled by the fact that I was getting this exception. The using statement is nice because, from the MSDN Documentation, “The using statement ensures that Dispose is called even if an exception occurs while you are calling methods on the object.”. So I doubt that most would expect a Dispose to throw an exception.

This troubled me enough I went looking for guidance from the “Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries”. Here is what I found (bold emphasis is my addition): “Avoid throwing an exception from within Dispose(bool) except under critical situations where the containing process has been corrupted (leaks, inconsistent shared state, etc.). Users expect that a call to Dispose will not raise an exception.”

I hope Microsoft reconsiders the current design of the SmtpClient in future releases.

This entry was posted on Friday, February 4th, 2011 at 11:16 and is filed under Technology. You can follow any responses to this entry through the RSS 2.0 feed. You can leave a response, or trackback from your own site.

1 Comment  »

  1. Richard says:
    10.05.11 at 15:02

    ReaderWriterLockSlim is even worse – calling Dispose a second time results in an ObjectDisposedException!

    https://connect.microsoft.com/VisualStudio/feedback/details/531334/readerwriterlockslim-does-not-implement-idisposable-properly

    Reply

RSS feed for comments on this post, TrackBack URI


Leave a Comment