2009-08-27

The Second Rule of Implementing IDisposable and Finalizers

This post is part of How to Implement IDisposable and Finalizers: 3 Easy Rules.

For a class owning managed resources, implement IDisposable (but not a finalizer)

IDisposable only has one method: Dispose. This method has one important guarantee: it must be safe to call multiple times.

An implementation of Dispose may assume that it is not called from a finalizer thread, that its instance is not being garbage collected, and that a constructor for its instance has completed successfully. These assumptions makes it safe to access other managed objects.

One mistake is to place a finalizer on a class that only has managed resources; this example code can result in an exception on the finalizer thread, which would crash the application:

// This is an example of an incorrect and buggy finalizer.
public sealed class SingleApplicationInstance
{
    private Mutex namedMutex;
    private bool namedMutexCreatedNew;
 
    public SingleApplicationInstance(string applicationName)
    {
        this.namedMutex = new Mutex(false, applicationName, out namedMutexCreatedNew);
    }
 
    public bool AlreadyExisted
    {
        get { return !this.namedMutexCreatedNew; }
    }
 
    ~SingleApplicationInstance()
    {
        // Bad, bad, bad!!!
        this.namedMutex.Close();
    }
}

Whether or not SingleApplicationInstance implements IDisposable, the fact that it's accessing a managed object in its finalizer is a recipe for disaster.

Here's an exmple of a class that removes the finalizer and then implements IDisposable in a correct but needlessly complex way:

// This is an example of a needlessly complex IDisposable implementation.
public sealed class SingleApplicationInstance : IDisposable
{
    private Mutex namedMutex;
    private bool namedMutexCreatedNew;
 
    public SingleApplicationInstance(string applicationName)
    {
        this.namedMutex = new Mutex(false, applicationName, out namedMutexCreatedNew);
    }
 
    public bool AlreadyExisted
    {
        get { return !this.namedMutexCreatedNew; }
    }
 
    // Needlessly complex
    public void Dispose()
    {
        if (namedMutex != null)
        {
            namedMutex.Close();
            namedMutex = null;
        }
    }
}

When a class owns managed resources, it may forward its Dispose call on to them. No other code is necessary. Remember that some classes rename "Dispose" to "Close", so a Dispose implementation may consist entirely of calls to Dispose and Close methods.

An equivalent - and simpler - implementation is here:

// This is an example of a correct IDisposable implementation.
public sealed class SingleApplicationInstance : IDisposable
{
    private Mutex namedMutex;
    private bool namedMutexCreatedNew;
 
    public SingleApplicationInstance(string applicationName)
    {
        this.namedMutex = new Mutex(false, applicationName, out namedMutexCreatedNew);
    }
 
    public bool AlreadyExisted
    {
        get { return !this.namedMutexCreatedNew; }
    }
 
    public void Dispose()
    {
        namedMutex.Close();
    }
}

This IDisposable.Dispose implementation is perfectly safe. It can be safely called multiple times, because each of the IDisposable implementations it invokes can be safely called multiple times. This transitive property of IDisposable should be used to write simple Dispose implementations like this one.

This post is part of How to Implement IDisposable and Finalizers: 3 Easy Rules.

22 comments:

  1. What about classes that inherit from an abstract class that has managed resources, and the the inheritor also has managed resources?

    ReplyDelete
  2. I would then implement Dispose as virtual, and call base.Dispose from the derived class' Dispose override.

    ReplyDelete
  3. What if the managed resource instance somehow got to be null? In your final example if namedMutex == null then you get an exception inside your Dispose.
    That should not happen and you should be sure to set up an instance in the constructor, but I can see some day someone screws up and you got a nasty bug in your Dispose.

    Could be that I miss something. Please correct me.

    ReplyDelete
  4. @Rodi: The design here is that SingleApplicationInstance owns the Mutex resource. Since it owns that resource, it is responsible for freeing it.

    For this reason, the Dispose method does not have a bug. It is possible that someone may introduce a bug during refactoring, but the Dispose method as it stands does not have a bug.

    In particular, I would say it's wrong to have Dispose check namedMutex for null. This implies that the design is that SingleApplicationInstance may or may not own a Mutex resource.

    ReplyDelete
  5. @Stephen: Thanks for answering. I guess it is mainly a design thing. It's not related just to the Dispose method. If you need to test for null in the Dispose you should have to test for null in every method where you'll be using the instance. It should indeed just be so that it will always be instanced and your unit tests should be able to pick it up if some other developer screws up and deletes the reference or disposes it too early.

    ReplyDelete
  6. ....instead of implementing IDisposable interface if we write a public method like

    public void ReleaseResource()
    { namedMutex.Close();
    }

    will result be same???

    ReplyDelete
  7. @Anonymous: not really. The advantage of using IDispose is that it has special language support (clients can use the "using" keyword instead of try/finally with a null check).

    IDisposable is the standard way that every other .NET programmer will expect you to release resources.

    ReplyDelete
  8. What exactly is the benefit of not coding a test to see if mutex is null? You speak of whether the object owns or maybe-owns the mutex as to whether that is the test for good code. I call BS on that and charge you with misdirecting a lot of inexperienced programmers. The fact is that the 1xCPU cycle it typically takes to execute a BranchNonZero (BNE) instruction which your IF statement represents means NOTHING in the course of your applications execution, but it does mean that the developer understand the following:
    1. Software Processes are complex
    2. Software is maintained by groups of people
    3. Software is maintained over time
    4. It is more important to be explicit about your intent than it is to have the next developer divine your intent by thinking that your coding values are somehow universal

    When I read your code I see risky vanity. When I see code with the IF statement I see code written to be bullet proof, designed to live long in a complex environment and owned by a team not an individual.

    And dont come and tell me that that contributes to bloat either. You will lose the balance of any respect I have.

    ReplyDelete
  9. @Anonymous:

    I don't include the "if" check precisely because the Dispose method should only dispose resources the object owns.

    If an object is refactored so that it no longer owns a resource, then Dispose needs to be refactored too (this would be true even if you had an if check).

    (BTW, I did not say to use an "if" statement if the object maybe-owns its resource. I never encourage "if" statements in Dispose; an object either owns a resource or it does not. It may share an owned resource by owning a reference-counted Disposable, but this is rarely needed).

    On the other hand, *not* calling Dispose when you *should* can cause some very difficult-to-find bugs. Many classes *require* Dispose to be called, no "if's" about it (pardon the pun!).

    So I claim the "if" check makes the code *less* robust in the face of complex software processes and team maintenance over time.

    What the benefits are of having an "if" check?
    1. It does not help when refactoring. Changing resource ownership requires changes to Dispose regardless of whether there's an "if" check.
    2. It does not help with robustness. If it actually avoids a null dereference, then you've only introduced a much harder-to-find error by skipping the owned resource's Dispose.


    On the other hand, I think *not* having an "if" check more clearly communicates the designed intent of the code. That is the reason for my recommendation (not for performance or avoiding code bloat ;).


    That said, you are free to take my advice and toss it in the trash. I am, after all, just a random blogger. :)

    ReplyDelete
  10. So as I understand it, the class never sets namedMutex to null, nor does it allow anything external to set it to null. Therefor, it knows never to check for null. You are doing away with unnecesary flags and checks. That does, however, delay GC until the instance of the class itself is set to null, no?

    ReplyDelete
  11. Not in the common use case. Normally, when Dispose is called, the object is about to become eligible for GC. In this case, the object (and its namedMutex) are both eligible for GC by the time Dispose returns.

    You may be interested in this blog post I did a while back: http://nitoprograms.blogspot.com/2010/02/q-should-i-set-variables-to-null-to.html

    ReplyDelete
  12. I've read a lot about the dispose pattern and these rules. It all makes good sense and it does make my job easier, but only if I am perfect enough to always call Dispose in my disposable classes. I find, however, that I want to add a finalizer and GC.SuppressFinalize in Dispose anyway because I can put logging into the finalizer to detect when I fail to properly call dispose. Is there a simpler, more elegant way to detect failure to dispose for an application in the field?

    ReplyDelete
    Replies
    1. There is no easier method that I'm aware of.

      Most logging methods may fail from within a finalizer, especially during process exit. Console.WriteLine is guaranteed to succeed, but pretty much anything else may fail.

      I have (in the distant past) thrown an exception in this situation, crashing the process intentionally. However, all of my more recent code just ignores this situation. AFAIK, there just isn't a good way to detect it other than in a finalizer, and your code is so restricted in a finalizer that there isn't much you can do even when you do detect it...

      Delete
    2. Thanks. I don't really care what happens during exit. I can't crash in a kiosk sort of situation. So it's what might otherwise leak in a long-running app for lack of Dispose calls that I try to catch from looking at plain text logs written within the Finalize. One logging call is all the extra work I do in a Finalize. I have found a few missing Disposes this way. My intention is to eventually remove the finalizers and conform to the rules you lay out, but for now the finalizers are a handy tool for debugging unattended systems.

      Delete
    3. In fact, you SHOULD call Dispose in your destructor, just in case the person who instantiated you fails to call your Dispose method properly. But as Stephen said, you must take care not to throw any exceptions in your destructor, as they will not be catchable by your application, and therefore will kill your application. Please see here: http://msdn.microsoft.com/en-us/library/vstudio/b1yfkh5e(v=vs.100).aspx

      Delete
    4. No, you should NOT call Dispose in your destructor/finalizer.

      In the context of this blog post, the objects in question only have managed resources, not unmanaged resources. In this situation, the Microsoft pattern would have a finalizer that essentially does nothing (it would call Dispose in a way that Dispose had no work to do). It is completely wrong to dispose managed resources from a finalizer, and there are no unmanaged resources to dispose.

      Delete
    5. OK I must be missing something here: who will call Dispose if the client doesn't? The GC certainly isn't...

      Delete
    6. It's the client's responsibility to call Dispose. If they don't, then no one does. The GC will invoke finalizers, which will clean up resources so there's no resource leaks.

      However, some types require a Dispose call for correctness (e.g., to flush a buffer to a stream); those Dispose methods *must* be called by the client. If you try to call Dispose from a finalizer, it runs too late, and you'll end up with a race condition that could potentially crash your application.

      Delete
  13. My class owns a managed resource (an API connection to an external system), therefore I am implementing IDisposable. Due to slow performance when making the connection, the connection is made once in the constructor and I expose a Disconnect() method that delegates to the api.Disconnect() method.

    However, I cannot be certain that users of my class will always call my Disconnect() method, and I want to make absolutely sure that the connection is closed when my object is disposed/finalized. Even though I do not have unmanaged resources, is this a case where a finalizer (and the full Dispose pattern) is needed? Otherwise how can I ensure the connection is closed and I do not have a leak?

    Many thanks

    ReplyDelete
    Replies
    1. I would say it depends on whether there is an actual "leak" or not. What happens if Disconnect is never called? If the system as a whole recovers from that situation (e.g., the external system will eventually detect the connection is no longer there or just times it out, and if the external system allows other connections from the same source) then it can be treated as a "managed" resource - it's inefficient but there's no leak.

      That's probably the best way to go, because the system as a whole must be able to recover from connection problems. The only other situation I can think of is if the (client-side) API layer itself has some restriction, like only allowing one connection per process. In that case, I would lean towards putting in a finalizer which actually (intentionally) crashes.

      Delete
  14. You're overlooking a possible scenario with your advice to avoid null checks in Dispose().

    Yes, *if* the IDisposable resource is created in the constructor of the class and is never released until the class is ready to be disposed, your approach is fine, because the IDisposable resource is guaranteed to exist when Dispose() is called.

    However, there are cases when that is *not* true, even though the class is still responsible for owning the resource.

    For example, if I have a class that can be configured to do something at regular intervals, it may create and own an IDisposable System.Threading.Timer. However, if that functionality is not called into service, the Timer may never be created. Or, maybe it was used at some point in the life of my class but turned off later. In either case, the Timer object is not guaranteed to exist when Dispose() is called; therefore a null check is required in Dispose().

    Another scenario might be when an exception is thrown while attempting to instantiate an IDisposable resource. The resource was not created successfully, so it will be null when Dispose() is called on the owning class. There may be other IDisposable resources besides that one that need to be cleaned up, though.

    In short, it is possible (and in my experience, somewhat common) that even though a class may own an IDisposable resource if it exists, it is not guaranteed to exist when Dispose() is called. Therefore, a null check is required in Dispose() to avoid an exception in Dispose().

    ReplyDelete