tags:

views:

627

answers:

5

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; 
    }
    }
+6  A: 
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.

Darin Dimitrov
@Darin main benefit is that SMTP client now sends the QUIT command finally during a dispose :) very happy about this!
JL
@Darin: WTF?? SmtpClient implements IDisposable in .NET 4.0??? This is a pretty big breaking change. This hurts.
Steven
@Steven, yes and it seems at last it properly closes the connection by sending the QUIT command to the server.
Darin Dimitrov
Tsk, tsk, tsk, IDisposable abuse. The System.Net team has no shame.
Hans Passant
+13  A: 
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(); 
    }
}
Jon Skeet
@Jon question updated...
JL
@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...
Jon Skeet
@Jon once the message is sent, I have no need for an instance to message or attachments. Should I rather implement a destructor?
JL
@Jon: `SmtpClient` does not implement `IDisposable` :-)
Steven
@Jon: Sorry, you are right. According to Darin Dimitrov it does implement IDisposable in .NET 4.0. It didn't in .NET 3.5. http://msdn.microsoft.com/en-us/library/system.net.mail.smtpclient.aspx
Steven
+2  A: 

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(); 
    }
}
Chris S
+2  A: 

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.

dbemerlin
Thats really nice looking code, thank you very much!
JL
+1  A: 

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 both under .NET 2.0 (+3.0 and 3.5) and under .NET 4.0.

Steven