Wednesday, 18 August 2010

IDisposable’s Should Assert In Their Finalizer

Paradoxically[*] I’m finding that C# makes resource management much harder than C++. In C++ life is so much easier because you always have to deal with resource management and RAII is the tool that makes it a no-brainer – you either use stack based variables or heap allocated variables and a smart-pointer class such as scoped_ptr/shared_ptr. C# on the other hand makes resource management ‘optional’ through the use of the IDisposable interface[%].

What Is and Isn’t Disposable?

And that is my first problem, you don’t always know what does and doesn’t need disposing – you have to go and find out. Of course IntelliSense can help you out here a little but it still means checking every object to see if it has a Dispose() method[#]. The other alternative is to hope you get it right and rely on a static analysis tool like FxCop to point out those ‘occasional’ mistakes. Personally I’ve yet to get anything really useful out of FxCop outside the usual stylistic faux pas’ which seems to be more the domain of StyleCop.

IDisposable is Viral

OK, that’s a little harsh on FxCop as I’m still learning to use it effectively. But after years of using C++ tools like Lint and BoundsChecker to watch my back I was more than a little disappointed. It does seem to point out if I aggregate a type that needs disposing and I haven’t implemented the Dispose pattern, which is nice. However Dispose() is like ‘const correctness’ in C++ - it’s viral - once you start correctly applying IDisposable to your types it then mushrooms and you now need to fix the types that aggregated those and so on.

Should Interfaces Inherit IDisposable?

This leads me to my first question – should interfaces inherit from IDisposable if you know that at least one implementation needs it? On the face of it the answer seems to be no as disposing is purely an implementation detail; but the whole point of interfaces is to avoid ‘programming to an implementation’. If the answer is no then the moment you cast down to an interface you hide the disposing behaviour. COM essentially has to deal with the same problem and its solution is to make AddRef() and Release() a fundamental requirement of every interface. Of course C# has RTTI built in through the use of the ‘as’ and ‘is’ keywords and so you can always attempt a cast to IDisposable from any other interface. However surely this way lies madness as your code would be littered with seemingly random ‘usings’ just in case an implementation later needed it. Here’s an example where this issues has cropped up most often to date…

We are using the Gateway Pattern extensively in our middle tier services to talk to other systems and so the gateway implementation often requires a WCF proxy which requires calling Close() (or a Socket, database connection etc). So, do I expose the disposing requirement through the gateway interface?

public interface ITradeGateway : IDisposable
{
. . .
}

public class TheirBackEnd : ITradeGateway
{
. . .
}

…or just implement IDisposable on the concrete type?

public interface ITradeGateway
{
. . .
}

public class TheirBackEnd : ITradeGateway, IDisposable
{
. . .
}

In my mind the first is more ‘discoverable’ than the second and it gives any static code analysis tools a fighting chance in pointing out where you might have forgotten to call Dispose(). Some might argue that at the point of creation you know the answer anyway as you have the concrete type so why does it matter? Well, I tend to wrap the creation of these kinds of services behind a factory method that returns the object via the intended interface so that you are not inclined to rely on the concrete type unnecessarily:-

public static TradeGatewayFactory
{
    public ITradeGateway CreateTradeGateway()
    {
        return new TheirBackEnd();
    }
}

Most of our factory methods are not quite this simple as they tend to take a configuration object that further controls the construction so that they can hide whether the ‘service’ is hosted in-proc (which is useful for testing and debugging) or remotely via a proxy[+].

Does It Matter If You Forget?

I mostly work on distributed systems where scalability and reliability are major concerns and perhaps I’m being overly pessimistic about the memory consumption of my services but I think it’s important that for certain kinds of resources that their lifetime is managed optimally[$]. At the moment I’m dealing with a managed wrapper over an in-house native library that is used to manipulate the key custom data container that the organisation traffics in. The underlying native implementation uses reference-counted smart pointers for efficiency and naturally this has leaked out into the managed wrappers so that many aspects of the API return objects that implement IDisposable. In fact it’s all too easy to use one of the helper methods (such as an index property) and find yourself leaking a temporary that you didn’t know about and bang goes your careful attempts to control the lifetime of the container, e.g.

// Good. 
using (var section = container.GetSection(“Wibble”)) 

   var entry = section.Value; 
   . . . 


// Leaky. 
var entry = container[“Wibble”].Value; 
. . .

I definitely think this scenario should be picked up by a static analysis tool and if I’ve read the blurb on FxCop 10.0 (that ships with VS2010) correctly then I have high hopes it will watch more of my back.

Assert In The Finalizer

So can we do anything else than rely on tooling? I think we can and that would be to put a Debug.Assert in the Finalizer - after all if the object is being consumed correctly then you should honour the contract and call Dispose() at the relevant point in time. I think it’s safe to say that the Garbage Collector does a pretty good job of hiding most mistakes by running frequently enough, but as Raymond Chen points out on his blog last week (which is “CLR Week”) - you should not rely on the Garbage Collector running at all.

For my own types that don’t manage any native resources themselves it could be implemented like this:-

public class ResourceManager : IDisposable
{
#ifdef DEBUG
    ~ResourceManager()
    {
        Debug.Assert(false);
    }
#endif
    . . .
    public void Dispose()
    {
        m_member.Dispose();

#ifdef DEBUG
        GC.SuppressFinalize(this);
#endif
    }
}

So basically we’re saying that if Dispose() is not invoked, then, when a Garbage Collection does finally occur at least we’ll know we forgot to do it. Sadly we can’t rely on being able to inspect the members in the debugger to work out which instance of an object was forgotten because finalizers can be run in any order; but maybe we’ll get lucky.

If you start from a clean slate then you can write a unit or integration test that forces a full garbage collection right after exercising your code to ensure any errant finalizers run and get instant feedback about your mistake:-

[Test]
public void should_not_leak_resources()
{
    var consumer = new ResourceConsumer();

    consumer.consumeResources();

    GC.Collect();
    GC.WaitForPendingFinalizers();
}

I’ll be honest and point out that I’ve put off actually trying this out in earnest until I have had time to investigate how to tap into the Asserting mechanism so that I can avoid hanging the test runner with a message box unless I’m running under the debugger. I’ve done this plenty of times with the Debug MS CRT (_CrtSetReportHook) so I’m sure there must be a way (I’ve only scratched the surface of the TraceListener class but I’m guessing it plays a part).

Debug Builds – Not The Done Thing?

Back in an earlier post Debug & Release Database Schemas I suggested there must be times when a debug build is used in the C#/.Net world. Unlike the C++ world, this beast does not seem to be at all prevalent. In fact I’ve yet to come across any 3rd party (or in-house teams) promoting a debug build. Visual Studio and C# supports the concept, but I wonder if teams only expect it to be used for internal testing? Jeffrey Richter briefly mentioned “Managed Debugging Assistants” in his book CLR via C# but I’ve yet to read up on how you use them effectively, i.e. tap into the mechanism programmatically so that I can log these failures whenever the services are running unattended; not just under the debugger.

[*] It’s not really a paradox as 15 years C++ vs 1 year C# isn’t exactly a fair comparison.

[%] Optional in the sense that not every type requires it.

[#] or Close() in the case of the thread synchronization types which is a nice inconsistency.

[+] I’m still not convinced by the use of an off-the-shelf Inversion of Control (IoC) framework as it only seems to save the odd line or two of code at the expense of managing another 3rd party dependency. I also much prefer creating immutable types that are fully constructed via the ctor than providing multi-phase construction via mutators which IoC frameworks seem to require. Maybe I just don’t work on the kind of systems they’re aimed at?

[$] The obvious question here I suppose is “Why are you using C# then?”. And the answer [for now] is “because we are”. I was expecting this to to scale-up further that it has, but we can still scale-out further if needs be.

2 comments:

  1. Hi Chris,

    I agree you you on this - especially on your comment about IDisposable going viral. I tend to use a base class for all of my disposable objects (where I can), which captures a stack trace when it gets created and asserts if it hasn't been disposed. I then get notified if one of my disposable objects has not been disposed of as expected. It also includes a 'DisposeStack' so that resources you own will get disposed of automatically when you do.

    One of the biggest issues issues that I've seen recently is that the OnUnloaded event doesn't always get called on a WPF control, meaning that if the control owns disposable items it never knows when it should dispose them!

    And whilst I'm ranting, if you share an object that requires disposing, who becomes responsible for disposing the object? How do you know you're the last one using it and that that it's safe to dispose?

    Cheers,
    Rik

    ReplyDelete
  2. Interesting idea, but how do you store the stack trace? You can't store it in a member as it could be finalised before your object - so you must have another reference. Maybe a static map and an integer based cookie?

    +1 for the object sharing rant. I found myself wanting a C# equivalent of shared_ptr<> pretty quickly and wondered what I'm missing because it doesn't seem that common a grumble.

    I suspect I'm way overusing IDisposable to simulate RAII because of C++ roots. I'm trying to let go I really am :-)

    ReplyDelete