Locking in C#

Question

I'm still a little unclear and when to wrap a lock around some code. My general rule-of-thumb is to wrap an operation in a lock when it reads or writes to a static variable. But when a static variable is ONLY read (e.g. it's a readonly that is set during type initialization), accessing it doesn't need to be wrapped in a lock statement, right? I recently saw some code that looked like the following example, and it made me think there may be some gaps in my multithreading knowledge:

class Foo
{
    private static readonly string bar = "O_o";

    private bool TrySomething()
    {
        string bar;

        lock(Foo.objectToLockOn)
        {
            bar = Foo.bar;          
        }       

        // Do something with bar
    }
}

That just doesn't make sense to me--why would there by concurrency issues with READING a register?

Also, this example brings up another question. Is one of these better than the other? (E.g. example two holds the lock for less time?) I suppose I could disassemble the MSIL...

class Foo
{
    private static string joke = "yo momma";

    private string GetJoke()
    {
        lock(Foo.objectToLockOn)
        {
            return Foo.joke;
        }
    }
}

vs.

class Foo
{
    private static string joke = "yo momma";

        private string GetJoke()
        {
            string joke;

            lock(Foo.objectToLockOn)
            {
                joke = Foo.joke;
            }

            return joke;
        }
}

Answer 1

Since none of the code you've written modifies the static field after initialization, there is no need for any locking. Just replacing the string with a new value won't need synchronization either, unless the new value depends on the results of a read of the old value.

Static fields aren't the only things that need synchronization, any shared reference which could be modified is vulnerable to synchronization issues.

class Foo
{
    private int count = 0;
    public void TrySomething()    
    {
        count++;
    }
}

You might suppose that two threads executing the TrySomething method would be fine. But its not.

1. Thread A reads the value of count (0) into a register so it can be incremented.
2. Context switch! The thread scheduler decides thread A has had enough execution time. Next in line is Thread B.
3. Thread B reads the value of count (0) into a register.
4. Thread B increments the register.
5. Thread B saves the result (1) to count.
6. Context switch back to A.
7. Thread A reloads the register with the value of count (0) saved on its stack.
8. Thread A increments the register.
9. Thread A saves the result (1) to count.

So, even though we called count++ twice, the value of count has just gone from 0 to 1. Lets make the code thread-safe:

class Foo
{
    private int count = 0;
    private readonly object sync = new object();
    public void TrySomething()    
    {
        lock(sync)
            count++;
    }
}

Now when Thread A gets interrupted Thread B cannot mess with count because it will hit the lock statement and then block until Thread A has released sync.

By the way, there is an alternative way to make incrementing Int32s and Int64s thread-safe:

class Foo
{
    private int count = 0;
    public void TrySomething()    
    {
        System.Threading.Interlocked.Increment(ref count);
    }
}

Regarding the second part of your question, I think I would just go with whichever is easier to read, any performance difference there will be negligible. Early optimisation is the root of all evil, etc.

Why threading is hard

Answer 2

Reading or writing a 32-bit or smaller field is an atomic operation in C#. There's no need for a lock in the code you presented, as far as I can see.

Answer 3

It looks to me that the lock is unnecessary in your first case. Using the static initializer to initialize bar is guaranteed to be thread safe. Since you only ever read the value, there's no need to lock it. If the value's never going to change, there will never be any contention, why lock at all?

Answer 4

Dirty reads?

Answer 5

In my opinion, you should try very hard to not put static variables in a position where they need to be read/written to from different threads. They are essentially free-for-all global variables in that case, and globals are almost always a Bad Thing.

That being said, if you do put a static variable in such a position, you may want to lock during a read, just in case - remember, another thread may have swooped in and changed the value during the read, and if it does, you may end up with corrupt data. Reads are not necessarily atomic operations unless you ensure they are by locking. Same with writes - they are not always atomic operations either.

Edit: As Mark pointed out, for certain primitives in C# reads are always atomic. But be careful with other data types.

Answer 6

If you're just writing a value to a pointer, you don't need to lock, since that action is atomic. Generally, you should lock any time you need to do a transaction involving at least two atomic actions (reads or writes) that depend on the state not changing between the beginning and end.

That said – I come from Java land, where all reads and writes of variables are atomic actions. Other answers here suggest that .NET is different.

Answer 7

As for your "which is better" question, they're the same since the function scope isn't used for anything else.