Like most modern languages, C# has built-in support for synchronization via the lock statement,
but as in all areas around concurrent programming, there are many ways to get this wrong, especially ways to get it wrong that are super hard to debug.
These techniques have been developed over some years, though I haven't kept up on best practices recommended by others who are truly experts in the field of concurrency. Of course I welcome feedback or corrections.
This also intends to eliminate every possible race condition or timing window, no matter how rare; this is a quest for correctness, because once you've learned to see concurrency bugs, they're easier to avoid in the first place.
I have a suite of applications that talk to the database for a line-of-business application written by a third-party, and though any part of the app can talk directly to the database at any time, there are a few kinds of objects that are used so often by so many different modules that I have built-in support for loading them via a context helper object.
We'll consider the Users table; where each has a unique numeric key. The rest of the members
of the object don't matter here:
public class User {
public int NBR { get; set; } // unique key
// userid, full name, etc.
}
We could just preload the users at application startup, but this would be a waste of time if a particular module never used it, so we have adopted a lazy-loading scheme where we maintain a dictionary of users (for quick finding by number), but loading only the first time:
public class MyContext {
// DON'T DO IT THIS WAY
public Dictionary<int, User> Users {
get {
_Users ??= Db.LoadUsers().ToDictionary(x => x.NBR);
return _Users;
}
}
// we won't show these again
private Dictionary<int, User> _Users = null;
public User findUser(int nbr)
{
// force load of Users the first time
return Users.TryGetValue(nbr, out var user)
? user
: null;
}
}
Here, the private variable _Users is loaded from the DB only if it hasn't been loaded before, and the findUser()
forces a load on the first call.
This works most of the time, but what if two parts of the app—whether separate threads, or different async code—call this at the same time? We'd now be doing two database loads, possibly walking all over each other. This would be rare, and though double-loading of the data wouldn't be catastrophic in this case, other operations that are not idempotent might be. So we need to fix this.
Unsurprisingly, we're going to use the lock statement to provide a critical section:
once inside, the test can be made safely to load from the db one time only:
public class MyContext {
public Dictionary<int, User> Users {
get {
lock (this)
{
_Users ??= Db.LoadUsers().ToDictionary(x => x.NBR);
}
return _Users;
}
}
...
}
This completely fixes the concurrency issue, but it also imposes lock overhead
on every fetch, and this could be a substantial performance hit when we only
really need the protection once.
So, we only enter the lock if the Users haven't been loaded, making the hot path avoid the locking:
public class MyContext {
public Dictionary<int, User> Users {
get {
if (_Users == null)
{
// RACE CONDITION HERE
lock (this)
{
_Users = Db.LoadUsers().ToDictionary(x => x.NBR);
}
}
return _Users;
}
}
...
}
This is better, and once the data is loaded there's no more locking, but we still have
a small concurrency window between the if test and the lock
statement itself: what if they ran and entered the first if at the same time?
We're back in the same position with the first example.
To really get this right, we have to do the test again inside the lock to avoid this race condition:
public class MyContext {
public Dictionary<int, User> Users {
get {
if (_Users == null)
{
lock (this)
{
// only load if not already assigned
_Users ??= Db.LoadUsers().ToDictionary(x => x.NBR);
}
}
return _Users;
}
}
...
}
This solution is good and completely solves both concurrency and performance issues, but there's a somewhat different pattern where it's not good enough.
Let's look at a different example where we're not using .ToDictionary()
but instead build the dictionary explicitly with some special handling on each user record:
public class MyContext {
public Dictionary<int, User> Users {
get {
if (_Users == null)
{
lock (this)
{
if (_Users == null)
{
_Users = new Dictionary<int, User>();
// RACE CONDITION HERE
foreach (var user in Db.LoadUsers())
{
// do stuff to the user record, then store in dict
_Users.Add(user.NBR, user);
}
}
}
}
return _Users;
}
}
...
}
Now we've created a new race condition that could be really bad.
What if, immediately after thread #1 assigns _Users to a new (and empty) dictionary,
thread #2 comes along to do the same thing? Because _Users is no longer null, it
assumes it's fully built so returns a partial dictionary to the caller.
This means thread #2's caller gets an incomplete dictionary, and perhaps might not even find a user that's known to be in the database. This would be very rare, but would be maddening to debug.
This is on top of whatever concurrency issues arise by reading and writing to a dictionary at the same time.
The fix is to create a local variable to contain the dictionary, build it, then
assign to _Users only when it's complete. This means no other thread
ever evades the lock by seeing an incomplete (or empty) dictionary:
public class MyContext {
public Dictionary<int, User> Users {
get {
if (_Users == null)
{
lock (this)
{
if (_Users == null)
{
var tmp = new Dictionary<int, User>();
foreach (var user in Db.LoadUsers())
{
// do stuff to the user record, then store in dict
tmp.Add(user.NBR, user);
}
_Users = tmp; // NO RACE CONDITION
}
}
}
return _Users;
}
}
...
}
One thing I'm not entirely sure about: can we use this as the locking
variable? Some Microsoft guidance suggests no because you don't know what other
parts of the code might use the same thing, creating lock competition where it's
not needed.
In some applications this could actually cause deadlock.
To properly fix this, we a custom locker variable for each
thing being protected. These serve only to be the anchor
for the lock statement (you can't use the _Users
variable because you can't lock on a null).
public class MyContext {
...
private Dictionary<int, User> _Users = null;
private readonly object _Users_locker = new();
}
... and then use lock(_Users_locker) instead of lock(this).
The examples above are for database loading, but could be for any kind of one-time initialization, whether from a database, a long-running file operation, or fetching data from the cloud.
There's certainly some overhead by implementing lazy loading, but it's been working terrifically well for me, and I'm not aware of ever tripping over a concurrency bug.
(front panel of my HP LaserJet 4200)
