CODEWITHSUNDEEP
c#disposeidisposable
lomaxx
lomaxx

Reputation: 115773

Correct usage of Dispose method on class that implements IDisposable

I was working with some code today that used a System.Net.Mail.MailMessage class like so

public MailMessage CreateMessage(string fromAddress, string recipient)
{
    MailMessage message = new MailMessage(fromAddress, recipient);
    message.Subject = subject;
    message.Body = body;
    return message;
}

Ignoring the trivial nature of this method, I got a compiler warning saying

object 'message' is not disposed along all exception paths. Call System.IDisposable.Dispose on object 'message' before all references to it are out of scope.

This is interesting because the compiler is warning that the message is not disposed before it goes out of scope, but I would assume that returning a reference to it would mean that whilst the message variable is out of scope, the would still be a reference to the underlying object, in which case I doubt very much that I would want to dispose it.

This has left me a little confused as the implication of the warning message is that you shouldn't return disposable objects. Is this really the case or is this just a case of compiler warning gone mad?

Upvotes: 3

Views: 1360

Answers (2)

supercat
supercat

Reputation: 81159

One pattern I've seen which may sometimes be helpful is to create an object called a DisposeWrapper<T> which holds an IDisposable object of type T, supplied in the constructor, and which supports two methods: Keep and Dispose. Calling Dispose without calling Keep will call Dispose on the wrapped IDisposable; calling Keep will prevent Dispose from hitting the IDisposable.

For convenience, it may be handy to have a static non-generic DisposeWrapper class with a generic factory method for DisposeWrapper<T> to allow the compiler to use method type inference to infer the type of the wrapper class.

One could thus do something like [I'm used to vb syntax, not c#, so apologies if this isn't quite right):

{
  using(var wrapper = DisposeWrapper.Create(new SomeDisposableThing))
  {
    ... do some stuff with wrapper.Value;
    return wrapper.Keep();
  }
}

If the code reaches the "return wrapper.Keep();", then wrapper.Value will be returned and not disposed. If the code exits the Using statement without calling wrapper.Keep, then wrapper.Value will be disposed.

It's sometimes annoying when compilers and tools insist that one must trap every possible way code could escape without cleaning up an IDisposable (especially when there's no way in C# to safely use field initializers with IDisposable fields) but I think wrapping an IDisposable should make the compiler happy.

An alternative pattern is to have a wrapper which holds a List of IDisposable, and use a generic method to "register" IDisposables as they are created (the method can return the new IDisposable as its appropriate type). This can be a nice pattern to use in a constructor; with a little work it can also be used for field initializers in vb.net.

Upvotes: 1

Jon
Jon

Reputation: 437376

The meaning of this warning is that if the method throws (e.g. in the Subject setter), you could be left with an undisposed MailMessage which noone has any reference to.

You are supposed to guard against this happening by something like this:

public MailMessage CreateMessage(string fromAddress, string recipient)
{
    MailMessage message = new MailMessage(fromAddress, recipient);
    try {
        message.Subject = subject;
        message.Body = body;
        return message;
    }
    catch {
        if (message != null) {
            message.Dispose();
        }
        throw;
    }
}

The compiler doesn't have anything against returning IDisposable instances :)

Upvotes: 7

Related Questions