The COM static store, part 3: Avoiding creation of an expensive temporary when setting a singleton

Raymond

Last time, we looked at one way to avoid a race condition when initializing a singleton in the COM static store. But it did create the possibility of creating an object that might be thrown away, and that could be a problem if the object is expensive to construct, or if other circumstances prevent you from creating more than one of those objects.

In that case, you can expand the lock to cover the construction of the Thing, and construct it only if you’re sure you’re going to need it.

Thing GetSingletonThing()
{
    auto props = CoreApplication::Properties();
    if (auto found = props.TryLookup(L"Thing")) {
        return found.as<Thing>();
    }
    auto guard = std::lock_guard(m_lock);
    if (auto found = props.TryLookup(L"Thing")) {
        return found.as<Thing>();
    }
    auto thing = MakeAThing();
    props.Insert(L"Thing", thing);
    return thing;
}

This avoids the creation of a throwaway Thing, but it does come at a cost: Since the Thing is created under the lock, its constructor is at risk of deadlocking if it acquires its own locks or performs cross-thread operations.

Suppose there’s another lock L, and the caller of Get­Singleton­Thing owns that lock. The Get­Singleton­Thing function sees that there is no Thing yet, so it takes its own private lock, and then constructs a new Thing. If the Thing constructor also attempts to acquire lock L, and the lock L is non-recursive, then you have recursive acquisition of L, which is formally undefined behavior.

Even if the lock L allows recursive acquisition, you can still deadlock:

Thread 1 Thread 2
Acquire lock L Call Get­Singleton­Thing
Call Get­Singleton­Thing Object doesn’t exist yet
Object doesn’t exist yet Acquire lock m_lock
Wait for lock m_lock Object still doesn’t exist yet
  Construct a new Thing
  Wait for lock L

Here we hit a classic deadlock, where each thread holds one lock and is waiting for the other one.

But even if there is no lock L, you can still run into problems if the construction of Thing requires cross-thread operations.

Thread 1 Thread 2
  Call Get­Singleton­Thing
Call Get­Singleton­Thing Object doesn’t exist yet
Object doesn’t exist yet Acquire lock m_lock
Wait for lock m_lock Object still doesn’t exist yet
  Construct a new Thing
  Send a request to Thread 1 to do some work

This time, Thread 2 is waiting for Thread 1 to do some work so it can finish constructing the Thing, but Thread 1 cannot do that work because it is waiting for the lock that protects Thing construction.

I’ve seen all of these types of deadlocks in production code. They hit rarely, but when they do, everybody has a bad day. Resolving the problem can be complicated because the locks or cross-thread operations are deeply embedded in the architecture, and a lot of refactoring has to be done to avoid dangerous operations while holding a lock.

So yeah, be extremely mindful about what you do while holding a lock. Don’t call out to foreign code while holding a lock.

Okay, enough about deadlocks. We’ll look at some ways of optimizing the COM singleton pattern next time.

5 comments

Comments are closed. Login to edit/delete your existing comments

  • Bwmat

    My preference for this kind of thing is to take the lock, and if the object hasn’t been created yet, leave a record that I’m about to create it, drop the lock, create it, and then take the lock, put it in, and drop it again for the last time.

    Any other thread that tries to get the object can see that someone else is creating it, and wait (ideally using something like a condition variable)

    • Ian Yates

      Good approach

      Similar to C# where people use a ConcurrentDictionary for this job, but to avoid double construction you use Lazy rather than just T. Creating the Lazy is cheap and quick and serves as the place holder. It has its own options then about allowing a race to initialise or not.

      Only issue is that it caches the Exception raised by the constructor if one was thrown rather than being allowed to try again. All depends on how you’d want that to behave – can of worms but worth understanding to avoid the corner cases that will eventually occur

    • Raymond ChenMicrosoft employee

      That really just trades the problem for an identical problem: You just renamed “wait for m_lock” to “wait for creator via condition variable”.

  • Neil Rashbrook

    Is there a pattern whereby while Thread 1 is waiting for that lock, it’s still able to process cross-thread operations? (Something along the lines of a thread waiting for a thread to reply to SendMessage can itself still be the target of a SendMessage call.)

    • Raymond ChenMicrosoft employee

      That’s reentrancy and is even scarier. Furthermore, if the caller of GetSingletonThing() holds a lock, you now have wild lock inversion.