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.
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
SendMessagecan itself still be the target of aSendMessagecall.)That’s reentrancy and is even scarier. Furthermore, if the caller of GetSingletonThing() holds a lock, you now have wild lock inversion.
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)
That really just trades the problem for an identical problem: You just renamed “wait for m_lock” to “wait for creator via condition variable”.
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