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.