6
\$\begingroup\$

I have a class:

public class EmailSender: IDisposable
{
    private SmtpClient EmailSmtpClient { get; set; }
    private MailMessage Message { get; set; } 

    public override void Dispose()
    {
         EmailSmtpClient.Dispose();
         Message.Dispose();
         GC.Collect();
    } 
}

This is what I currently have implemented. What is the best way to release all the resources including the attachments (that are a part of MailMessage) immediately after the email is sent? Can anyone review my Dispose method and suggest some betterment? I will be thankful :)

\$\endgroup\$
  • \$\begingroup\$ Since EmailSender does not inherit from any class, there is no overriding here per se. This is just an interface implementation. Therefore, the override keyword may (and should) be removed from the Dispose() signature. \$\endgroup\$ – Igor Soloydenko May 4 '17 at 15:52
  • 2
    \$\begingroup\$ Why do you need an EmailSender class? Sending emails is an activity, and building classes for activities is a pattern common in Java, yet often critized. Hence you might get a better design by implementing a SendEmails (IEnumerable <Email> emails) method somewhere. The mere fact that then your problem would disappear seems to indicate that this is a better solution. \$\endgroup\$ – JohnB May 5 '17 at 7:06
11
\$\begingroup\$

I would suggest that you read these pieces from MSDN:

Basic Dispose Pattern, Implementing a Dispose Method

Some things that immediately stand out from your example:

  • Replace your GC.Collect() with GC.SuppressFinalize(this)
  • Check your resources aren't null before attempting to dispose them.
  • Check that your objects haven't already been disposed.

All of this is easily done by following the dispose pattern.

\$\endgroup\$
  • \$\begingroup\$ I would only do 1) iff your finalizer calls the Dispose method, which it should. \$\endgroup\$ – Aron May 5 '17 at 2:49
  • 1
    \$\begingroup\$ @Aron I disagree. Unless you have unmanaged resources to clean up, there is no need for a finalizer. And I don't think I ever had unmanaged resources to clean up. So from my experience you practically never need a finalizer and therefor you never need to suppress finalize. \$\endgroup\$ – Patrick Huizinga May 5 '17 at 7:33
  • 1
    \$\begingroup\$ * As I mentioned in my previous comment, you probably have never a need for suppress finalize. * Instead of checking your resources are not null, I would find a way to ensure they're never null (via a constructor for example). That also makes the other methods in the class easier to write. * You don't have to check if your objects have already been disposed, because those objects should be resistant to having Dispose() called multiple times. \$\endgroup\$ – Patrick Huizinga May 5 '17 at 7:39
  • \$\begingroup\$ @PatrickHuizinga You actually agreed with me. IFF means "if and only if". Maybe we work in different environments, we have different amounts of unmanaged code. \$\endgroup\$ – Aron May 5 '17 at 7:42
  • \$\begingroup\$ @Aron I guess I got tripped by the "which it should" part :) \$\endgroup\$ – Patrick Huizinga May 5 '17 at 7:51
4
\$\begingroup\$

As your method doesn't itself allocate any external resource, your should definitely not implement Basic Dispose Pattern, Implementing a Dispose Method.

These are useful when your class itself directly allocates an external resource (i.e. not when it is allocated through a disposable .NET class). (They can also be useful if your class needs to reference something in a static field).

You should also not dispose any object you didn't allocate in your own code. As your class receives it's SmtpClient and MailMessage through, it is not aware of their life cycle and should not dispose them (unless they are provided by an IoC container through these properties, but then they should be provided in a constructor).

The code allocating the MailMessage should itself take care of disposing it.

Your class could be completely in charge of the SmtpClient, and then should allocate it and dispose it.

Not implementing the full Dispose pattern

The full dispose pattern allows a finalizer to release external resources should the client code neglect to dispose it.

Your finalizer would then end up on the finalizer queue (along with those from the SmtpClient and MailMessage if they are not referenced elsewhere), and all three of them would then be called at the same time some time later.

Should the SmtpClient and MailMessage really not be referenced elsewhere, they would take care of their resources, and your finalizer would have nothing useful to do.

Should they be referenced elsewhere, your finalizer would dispose them, rendering them unusable.

\$\endgroup\$
  • \$\begingroup\$ msdn.microsoft.com/en-us/library/ms182328.aspx \$\endgroup\$ – Lachlan Stewart Kidson May 5 '17 at 6:39
  • \$\begingroup\$ "These are useful when your class itself directly allocates an external resource (i.e. not when it is allocated through a disposable .NET class)." is contrary to CA2213 \$\endgroup\$ – Lachlan Stewart Kidson May 5 '17 at 6:40
  • 1
    \$\begingroup\$ @LachlanStewartKidson, the rule you linked is referring to private fields, not public properties. It also says It is safe to suppress a warning from this rule if you are *not responsible* for releasing the resource held by the field. Clearly a class that get its properties injected from outside code can not possibly know whether or not those objects are in-use somewhere else and therefore can not manage their lifetime. In my opinion @Philippe made perfectly valid points. \$\endgroup\$ – Nikita B May 5 '17 at 9:18
  • \$\begingroup\$ @NikitaB You're correct. I missed this when I answered and misinterpreted Philippe's answer. \$\endgroup\$ – Lachlan Stewart Kidson May 5 '17 at 9:23
3
\$\begingroup\$

Your question actually consists of two.

"Implement IDisposable so that the resources are released"...

@Lachlan's answer provides the links that cover this topic in detail.

I would also recommend looking closely into this StackOverflow question. Pay special attention to Ian Boyd's answer which received 2010(!) upvotes

..."immediately after the email is sent"

It's important to remember that a disposable class generally does not control when it's Dispose() method is called from the outside. There are quite a few common ways around usage of disposable objects, the most common one would probably look like this:

using (var emailSender = new EmailSender(...)) {
  // ...
  emailSender.Send(email);                        // 1
  // ...                                          // 2
}                                                 // 3

The email is sent at line 1, but this is not what is going to trigger the resource disposal. I mean, nothing calls Dispose there. Some other code may exist at line(s) 2 -- the resources are still live. Only when we exit from the using block (at line 3), the Dispose() will be invoked explicitly. So, technically speaking, this is not "immediately after save".

Of course, you always have an option of writing the more old-school try-catch-finally and manually invoke Dispose() from the finally block, but it's less elegant in most of the cases from my experience.

Another thing you could do, is make your Send() method explicitly invoke Dispose(). That would guarantee the "immediate" resource disposal on email sending. You will need to implement Dispose() with extra care because it may be invoked many time. Also, it would lead to non-reusable EmailSender objects which may or may not be okay depending on many criteria.

class EmailSender : IDisposable {

  public void Send(Email email) {
    try {
      this.DoWhatNeedsToBeDoneForSending(email);
    } finally {
      this.Dispose();                // That will guarantee an attempt of resource release
    }
  }

  public void Dispose() {
    // ...
  }

}
\$\endgroup\$
  • \$\begingroup\$ using translates into try..finally much like your second code snippet - there's really no reason not to use using. If it must be "wrapped" somehow, just create a method that doesn't have code at //2 and call that. \$\endgroup\$ – Jesse C. Slicer May 4 '17 at 17:30
  • \$\begingroup\$ @JesseC.Slicer using translates into try..finally -- of course. I updated my answer so it's more explicit that Send is the method of EmailSender in second scenario. The idea here is to ensure resource release immediately on actual email sending completion -- which is the thing OP asked about. I am not suggesting that this is the best way to achieve the result, but it is one of possible approaches. The mere implementation of IDisposable by itself does not achieve this result. \$\endgroup\$ – Igor Soloydenko May 4 '17 at 17:36
  • 15
    \$\begingroup\$ I disagree with the idea of having an object dispose itself. The point of Dispose is to give others the ability to dispose of your object. If it takes care of releasing its own resources after every call, you shouldn't implement IDisposable at all and just release everything in the method, or implement a private cleanup method and call that from the method. \$\endgroup\$ – Matti Virkkunen May 4 '17 at 17:42
  • \$\begingroup\$ @MattiVirkkunen I never advocated for this approach, I said that if that was to be achieved as per exact question statement, self-management would be the only option. Your disagreement with the idea should be expressed in the comments to the question rather than answer, because the "immediate" resource release does not make much sense to me too. Once again, notice that my question explains that this behavior would be very unusual, and the workaround is provided for answer completeness reasons, not as a recommendation. \$\endgroup\$ – Igor Soloydenko May 4 '17 at 17:46
  • 1
    \$\begingroup\$ @IgorSoloydenko: Releasing your resources immediately makes perfect sense if you don't need to hold onto them - for instance SmtpClient is pretty lightweight and you could just make it a local variable. But this.Dispose was your idea and it's a weird one. \$\endgroup\$ – Matti Virkkunen May 5 '17 at 8:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.