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:
- var client = new SmtpClient();
- // Fill out the message template that is stored in the Activate.txt file.
- using (var message = new TemplatedMailMessage(webmasterEmail, model.Email, templateFilePath, model))
- {
- // Add the webmaster to the bcc.
- message.Bcc.Add(webmasterEmail);
- client.Send(message);
- }
However, when I changed it to this:
- using (var client = new SmtpClient())
- {
- // Fill out the message template that is stored in the Activate.txt file.
- using (var message = new TemplatedMailMessage(webmasterEmail, model.Email, templateFilePath, model))
- {
- // Add the webmaster to the bcc.
- message.Bcc.Add(webmasterEmail);
- client.Send(message);
- }
- }
And had the following in my App.config file:
- <!–This is used to just test the e-mail locally rather than actually sending the message via SMTP–>
- <system.net>
- <mailSettings>
- <smtp deliveryMethod="SpecifiedPickupDirectory">
- <specifiedPickupDirectory pickupDirectoryLocation="M:\DropFolder" />
- </smtp>
- </mailSettings>
- </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:
- private void CheckHostAndPort()
- {
- if ((this.host == null) || (this.host.Length == 0))
- {
- throw new InvalidOperationException(SR.GetString("UnspecifiedHost"));
- }
- if ((this.port <= 0) || (this.port > 0xffff))
- {
- throw new InvalidOperationException(SR.GetString("InvalidPort"));
- }
- }
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
- {
- using (var client = new SmtpClient())
- {
- // Fill out the message template that is stored in the Activate.txt file.
- using (var message = new TemplatedMailMessage(webmasterEmail, model.Email, templateFilePath, model))
- {
- // Add the webmaster to the bcc.
- message.Bcc.Add(webmasterEmail);
- client.Send(message);
- }
- }
- }
- catch (InvalidOperationException)
- {
- // I don't like swallowing this exception,
- // but Microsoft really leaves me no choice.
- }
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.
Leave a Reply