C# how to correctly dispose of an SmtpClient?

Question

VS 2010 code analysis reports the following:

Warning 4 CA2000 : Microsoft.Reliability : In method 'Mailer.SendMessage()', 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.

My code is :

public void SendMessage()
    {
        SmtpClient client = new SmtpClient();

        client.Send(Message);
        client.Dispose(); 
        DisposeAttachments(); 
    }

How should I correctly dispose of client?

Update: to answer Jons question, here is the dispose attachments functionality:

private void DisposeAttachments()
{
    foreach (Attachment attachment in Message.Attachments)
    {
        attachment.Dispose();
    }
    Message.Attachments.Dispose();
    Message = null; 
}

Last Update full class listing (its short)

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Net.Mail;

public class Mailer
{
    public MailMessage Message
    {
        get;
        set;
    }

    public Mailer(MailMessage message)
    {
        this.Message = message; 
    }

    public void SendMessage()
    {
        using (SmtpClient client = new SmtpClient())
        {
            client.Send(Message);
        }
        DisposeAttachments(); 
    }

    private void DisposeAttachments()
    {
        foreach (Attachment attachment in Message.Attachments)
        {
            attachment.Dispose();
        }
        Message.Attachments.Dispose();
        Message = null; 
    }
}

Rowan

@JL - instead of disposing attachments manually you should be disposing the mailmessage itself which disposes attachments, alternate views, and other pieces. – Ferrule

True but in legacy version (pre .net 4.0) SmtpClient didn't implement a Dispose method – Shipley

Answer 1

public void SendMessage()
{
    using (SmtpClient client = new SmtpClient())
    {
        client.Send(Message);
    }
    DisposeAttachments(); 
}

That way the client will be disposed even if an exception is thrown during the Send method call. You should very rarely need to call Dispose explicitly - it should almost always be in a using statement.

However, it's not clear how the attachments are involved here. Does your class implement IDisposable itself? If so, that's probably the place to dispose of the attachments which are presumably member variables. If you need to make absolutely sure they get disposed right here, you probably need:

public void SendMessage()
{
    try
    {
        using (SmtpClient client = new SmtpClient())
        {
            client.Send(Message);
        }
    }
    finally
    {
        DisposeAttachments(); 
    }
}

Martamartaban

@JL: Hmm... it still feels like the attachments should be disposed as part of the instance being disposed, probably. Your DisposeAttachments message also means you can't get at the message afterwards, which sounds a little odd... – Martamartaban

once the message is sent, I have no need for an instance to message or attachments. Should I rather implement a destructor? – Rowan

SmtpClient does not implement IDisposable :-) – Disconnection

Sorry, you are right. According to Darin Dimitrov it does implement IDisposable in .NET 4.0. It didn't in .NET 3.5. msdn.microsoft.com/en-us/library/… – Disconnection

JFYI: Keep in mind that if connection to smtp-server is down, the exception is sometimes thrown during calling .Dispose(), not .Send(). So keep the whole using block inside a try-catch, no just the Send() call. – Hartzell

@jitbit: Only if you want to catch that exception rather than letting it bubble up... – Martamartaban

@JonSkeet of course you're right. Just wanted to emphasize, that IF you want to catch connection-errors - keep the "dispose()" call inside the TRY-block. Which is NOT something people expect. – Hartzell

Answer 2

The SmtpClient class in .NET 4.0 now implements IDisposable, while the SmtpClient class in .NET 2.0 lacks this interface (as Darin noted). This is a breaking change in the framework and you should take appropriate actions when migrating to .NET 4.0. However, it is possible to mitigate this in your code before migrating to .NET 4.0. Here is an example of such:

var client = new SmtpClient();

// Do not remove this using. In .NET 4.0 SmtpClient implements IDisposable.
using (client as IDisposable)
{
    client.Send(message);
} 

This code will compile and run correctly both under .NET 2.0 (+3.0 and 3.5) and under .NET 4.0.

Disconnection

But... will it help with actually disposing the client correctly? – Valladares

Because the real issue with the earlier 3.5 version is that it doesn't send a QUIT message to the server, causing the server to keep waiting for communications. This apparently causes problems when then reconnecting to the same server. – Valladares

@Nyerguds: This construct will not fix that of course. – Disconnection

By the way, while this will compile, you must keep in mind that "as" is a safe cast which in case of failure returns null. Meaning, sure, it'll compile in pre-4.0, but it might make client null at that point if the cast fails, causing a null reference error right after that. – Valladares

@Valladares and what exactly will be null when that cast fails? The whole idea of this construct is to work in both scenarios (with and without disposable). There will not be any NullRefEx in that case. – Disconnection

Well, using specifies the object it'll try to dispose at the end of the operation... and in this case, that's client as IDisposable, AKA, null in .net 3.5. I haven't actually tested this, but unless the using system specifically checks that, I'd imagine that could give an error at the moment it tries to call Dispose() on that null. – Valladares

@Nyerguds: Instead of imaginening, why won't you test it, like I have done before writing down this answer? – Disconnection

Answer 3

using (SmtpClient client = new SmtpClient())
{
    client.Send(Message);
    DisposeAttachments(); 
}

Interesting - contrary to .NET 3.5, SmtpClient implements IDisposable in .NET 4.0, learning new things every day.

Zenia

@Darin main benefit is that SMTP client now sends the QUIT command finally during a dispose :) very happy about this! – Rowan

@Darin: WTF?? SmtpClient implements IDisposable in .NET 4.0??? This is a pretty big breaking change. This hurts. – Disconnection

@Steven, yes and it seems at last it properly closes the connection by sending the QUIT command to the server. – Zenia

Tsk, tsk, tsk, IDisposable abuse. The System.Net team has no shame. – Carbonize

Be warned - if you're not using the Network delivery method, and you haven't set the Host property, then the Dispose method will throw an InvalidOperationException! connect.microsoft.com/VisualStudio/feedback/details/539160/… – Crisper

Link from @RichardDeeming is dead but can be found on the waybackmachine. Apparently the bug in Dispose was fixed in .NET 4.5 web.archive.org/web/20130724055045/http://connect.microsoft.com/… – Exposure

Answer 4

I would do something like that:

class Attachments : List<Attachment>, IDisposable
{
  public void Dispose()
  {
    foreach (Attachment a in this)
    {
      a.Dispose();
    }
  }
}

class Mailer : IDisposable
{
  SmtpClient client = new SmtpClient();
  Attachments attachments = new Attachments();

  public SendMessage()
  {
    [... do mail stuff ...]
  }

  public void Dispose()
  {
    this.client.Dispose();
    this.attachments.Dispose();
  }
}


[... somewhere else ...]
using (Mailer mailer = new Mailer())
{
  mailer.SendMail();
}

This would allow reusing the SmtpClient Object if you want to send multiple mails.

Hoary

Answer 5

This is the neater solution that will pass the code police test (and dispose will always be called if Send fails):

public void SendMessage()
{
    using (SmtpClient client = new SmtpClient())
    {   
        client.Send(Message);
        DisposeAttachments(); 
    }
}

Hope

Answer 6

My original problem is about intermittent sending failures. E.g. First Send() succeeds, 2nd Send() fails, 3rd Send() succeeds. Initially I thought I wasn't disposing properly. So I resorted to using().

Anyways, later I added the UseDefaultCredentials = false, and the Send() finally became stable. I still retained the using() function.

Gigue

Answer 7

public void SendMessage()
{
    try
    {
        using (SmtpClient client = new SmtpClient())
        {
            client.Send(Message);
        client.dispose()

        }
    }
    finally
    {
        DisposeAttachments(); 
    }
}

Quechuan

Please, try to provide your code only answer with some description – Monocyte

Calling Dispose() inside a using block seems completely redundant. – Valladares