IanG on Tap

John Sands points out a flaw in the code I showed in a recent blog for using timeouts on locks without abandoning most of the convenience of C#'s lock keyword .

I made a number of modifications to the example after Eric Gunnerson pointed out that it would be more efficient to use a struct than a class, since we can avoid an unnecessary heap overhead. However, I didn't want to lose the debug feature I added whereby in debug builds, you'd get an assertion if you failed to release a lock. So I made the type a class in release builds and a struct in debug builds.

I was a little uncomfortable with this at the time, but was trying to get the best of everyone's suggestions into a single example. However, John is quite right - changing between a struct and a class between release and debug builds is just too big a change.

So, here's yet another version:

using System;
using System.Threading;

// Thanks to Eric Gunnerson for recommending this be a struct rather
// than a class - avoids a heap allocation.
// Thanks to Change Gillespie and Jocelyn Coulmance for pointing out
// the bugs that then crept in when I changed it to use struct...
// Thanks to John Sands for providing the necessary incentive to make
// me invent a way of using a struct in both release and debug builds
// without losing the debug leak tracking.

public struct TimedLock : IDisposable
{
    public static TimedLock Lock (object o)
    {
        return Lock (o, TimeSpan.FromSeconds (10));
    }

    public static TimedLock Lock (object o, TimeSpan timeout)
    {
        TimedLock tl = new TimedLock (o);
        if (!Monitor.TryEnter (o, timeout))
        {
#if DEBUG
            System.GC.SuppressFinalize(tl.leakDetector);
#endif
            throw new LockTimeoutException ();
        }

        return tl;
    }

    private TimedLock (object o)
    {
        target = o;
#if DEBUG
        leakDetector = new Sentinel();
#endif
    }
    private object target;

    public void Dispose ()
    {
        Monitor.Exit (target);

        // It's a bad error if someone forgets to call Dispose,
        // so in Debug builds, we put a finalizer in to detect
        // the error. If Dispose is called, we suppress the
        // finalizer.
#if DEBUG
        GC.SuppressFinalize(leakDetector);
#endif
    }

#if DEBUG
    // (In Debug mode, we make it a class so that we can add a finalizer
    // in order to detect when the object is not freed.)
    private class Sentinel
    {
        ~Sentinel()
        {
            // If this finalizer runs, someone somewhere failed to
            // call Dispose, which means we've failed to leave
            // a monitor!
            System.Diagnostics.Debug.Fail("Undisposed lock");
        }
    }
    private Sentinel leakDetector;
#endif

}
public class LockTimeoutException : ApplicationException
{
    public LockTimeoutException () : base("Timeout waiting for lock")
    {
    }
}

The trick I've used here is to add a field to the struct in debug builds to hold onto a reference to an object with a finalizer. In release builds that object simply isn't there. This gives us the debug-only leak tracking behaviour, but without the rather drastic step of changing a public class to a public struct for release builds!

So, is this the last you'll hear of this sample? I'm afraid it might not be... Philip Haack has added his own twist to this sample. He has added code to provide a stack backtrace of the thread that is holding the lock in the case where deadlock occurs! (So you get two stack traces - the thread that failed to get the lock and the thread currently holding the lock.) I'm considering adding the same facility to this modified version. However, there are non-trivial costs involved in keeping track of stack traces, particularly if you use a technique where you store the trace 'just in case' every time a lock is acquired. This is what Phillip's solution does, and it's not obvious how to avoid it. I'm trying to work out if there's a way around this using the debug API, or some other technique that can be brought into play on demand without incurring unacceptably high costs in the case where a stack trace turns out not to be needed.