Multithreading and Properties. Part 1

Alex Harbuzenko · · .Net, C#, Interlocked

In a single-threaded world there are plenty of things we never even stop to think about. For example, in an online game the following description of a “living object’s” (an NPC or a player) health in C# looks perfectly acceptable:

public int Health
{
    get { return m_health; }
    set { m_health = value; }
}

When damage is dealt, the following code is perfectly logical:

int damage = 100;
target.Health -= damage;

if (target.Health <= 0)
{
    target.Die();
}

That is, we subtract the health value, then check whether the target is still alive. Time passes, player counts grow, processors get cheaper. Now we’ve already got several multi-core processors, thousands of concurrent players, heavy load. In this light a single processing thread looks archaic and sooner or later will be turned into several competing threads.
If we leave the code above unchanged in a multithreaded model, sooner or later we’re going to run into unpleasant surprises. To make this easier to follow, I’ll rewrite the property access the way the compiler does internally, but keep this part in C# syntax:

int damage = 100;
int tmpHealth = target.get_Health();
target.set_Health(tmpHealth - damage);
if (target.get_Health() <= 0)
{
    target.Die();
}

I won’t go into why Health -= turns into this little monster – go read Richter, or MSDN. What matters far more is that in these few lines of code we already have several synchronization problems. Let me lay out, in a table, a few possible sequences of events with two threads simultaneously trying to perform this operation. For convenience let’s say the m_health value started out at 150, and damage for our two threads is 100 and 200 respectively.

Scenario 1

Thread 1 Thread 2
int tmpHealth = target.get_Health();
tmpHealth value = 150
int tmpHealth =target.get_Health();
tmpHealth value = 150
target.set_Health(tmpHealth – damage);
m_health value = 50
target.set_Health(tmpHealth – damage);
m_health value = -50
if (target.get_Health() <= 0)
{
    target.Die();
}
condition is true, since m_health is already -50,
target.Die() gets called
if (target.get_Health() <= 0)
{
    target.Die();
}
condition is true, target.Die() gets called

Scenario 2

Thread 1 Thread 2
int tmpHealth = target.get_Health();
tmpHealth value = 150
target.set_Health(tmpHealth – damage);
m_health value = 50
int tmpHealth = target.get_Health();
tmpHealth value = 50
target.set_Health(tmpHealth – damage);
m_health value = -150
if (target.get_Health() <= 0)
{
    target.Die();
}
condition is true, target.Die() gets called
if (target.get_Health() <= 0)
{
    target.Die();
}
condition is true, since m_health = -150,
target.Die() gets called

Scenario 3

Thread 1 Thread 2
int tmpHealth = target.get_Health();
tmpHealth value = 150
int tmpHealth = target.get_Health();
tmpHealth value = 150
target.set_Health(tmpHealth – damage);
m_health value = -50
target.set_Health(tmpHealth – damage);
m_health value = 50
if (target.get_Health() <= 0)
{
    target.Die();
}
condition is false, since m_health = 50
if (target.get_Health() <= 0)
{
    target.Die();
}
condition is false

This isn’t every possible scenario, just a few of them. Regardless of how many processors you have, there’s always a chance that the same piece of code running on different threads will execute in an order that works against you. In the case of NPC health, this can lead to Die() getting called multiple times, or to one of the calls’ result being lost. Someone might suggest adding a lock inside the Health property itself:

public int Health
{
    get
    {
        lock (m_healthLock)
            return m_health;
    }
    set
    {
        lock (m_healthLock)
            m_health = value;
    }
}

But a lock like this buys us nothing beyond a strictly ordered sequence of reads/writes, which doesn’t change anything in our scenario. The correct fix is to wrap the method itself in a lock with double-checking:

if (target.Health > 0)
    lock (taget.HealthLock)
        if (target.Health > 0)
        {
            int damage = 100;
            target.Health -= damage;

            if (target.Health <= 0)
            {
                target.Die();
            }
        }

This approach gives the optimal result, but it comes with significant code changes, as well as touching a CriticalSection (paired with the ThinLock in .Net 2.0 and above) on every single access.
Of course, things would be easier if we were using C++, where a method for getting health could return a reference or a pointer, but C# has no such standard mechanism (let’s set aside unsafe code and pointers for now). It would also be simpler if Health weren’t a property but a plain field. In that case, we could just write:

int damage = 100;
if (Interlocked.Add(ref target.Health, -damage) > 0)
{
    target.Die();
}

The atomic Interlocked.Add operation guarantees the subtraction happens without interference from other threads, and also returns the old new value of the variable. This way, we can reliably tell whether this particular hit was the killing blow.
We’ve moved away from the principles of “pretty code” in exchange for extra performance. In most cases that’s an acceptable trade-off, but properties have one more useful property – they can be virtual.
For example, in the base implementation of a “living object” we have the same plain-variable logic:

public virtual int Health
{
    get { return m_health; }
    set { m_health = value; }
}

But in the subclass implementing the player, the write instead goes into a DB object:

public override int Health
{
    get { return m_dbCharacter.Health; }
    set { m_dbCharacter.Health = value; }
}

Switching to Interlocked methods becomes a problem in this case, so the most sensible option is to stop using the setter directly (for example, declare it as protected in .Net 3.5) and add a separate method for changing the object’s health:

void ModifyHealth(int value)
{
    if (Health > 0)
        lock (m_healthLock)
            if (Health > 0)
            {
                Health += value;

                if (Health <= 0)
                {
                    Die();
                }
            }
}

I’d like to use atomic methods here too, but that’s already problematic, since m_dbCharacter.Health can itself very well be a property.
The highest-performance solution is to drop virtuality on properties like this and update the DB object at key points instead (when leaving the game, on death, and so on). The most correct solution is to avoid public setters altogether for properties that might be touched from multiple threads. My advice to reasonable people is to settle on one of these two options. As for everyone else (fans of nonstandard solutions and “crusades”), I’ll offer a few of my own approaches in the next articles on this topic.

Update 03.04.08: Interlocked.Increment/Interlocked.Decrement don’t take a second parameter. The correct one to use is Interlocked.Add. If I confused anyone, my apologies, I got confused myself.

Update 04.04.08: During testing it turned out that Interlocked.Add returns the new value, not the old one. I missed that, mea culpa.

Originally published 2008-03-28.