views:

1335

answers:

7
+4  Q: 

Locking in C#

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;
        }
}
+1  A: 

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.

Kevin Conner
+1  A: 

Dirty reads?

brian
What are "dirty reads?" Does that have something to do with unsafe/unmanaged code?
Chris
No, take a look at the Wikipedia article: http://en.wikipedia.org/wiki/Isolation_level#READ_UNCOMMITTED_.28dirty_reads.29
noocyte
+6  A: 

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.

Mark Bessey
actually that's only because you're on a 32bit OS *grin* If however you were on Windows Mobile I'm not so sure that's the case.
blowdart
However, incrementing or adding to a 32-bit value isn't atomic, so you still have to be careful.
Matt Howells
Matt, if you're incrementing or adding to a 32-bit value, that would be a write, and therefore the statement "reading or writing a 32-bit... field is an atomic operation" from Mark would be incorrect, no? Your statement seems to contradict Mark.
Chris
Incrementing a value (unless you use AtomicIncrement) is reading THEN writing a value. Neither the read or write will fail or be interrupted, but you might have a race condition in that case, where a lock might be necessary.
Mark Bessey
Ahh, thanks for the clarification Mark!
Chris
+1  A: 

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.

unforgiven3
What would you suggest instead of using static variables? For example, I've got a Databases.dll assembly that reads a connection string from a .config file, and stores it in a static class so that it can be used to create DataContext objects. What would be an alternative to that, for example?
Chris
In the example, however, no other thread can change the value during the read, so no contention for the field. So no lock, right?
Chris
Yes, I'd say no lock in that case...Hmm. In this case, you might be okay with the static variable, in fact I'd probably do the same, because it's overkill in this case (IMHO) to do something else. I'd say approach static variables on a case-by-case basis.
unforgiven3
A: 

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

Kevin Conner
The IL for the second example is shorter, but not in a meaningful way. You're right.
Chris
+2  A: 

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?

Haacked
My thoughts exactly.
Chris
+10  A: 

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

Matt Howells
"Early optimisation is the root of all evil, etc". I thought that was 'premature optimization', early optimization is good if it comes after measurements, premature...is another thing and usually with no measurements at all..
Pop Catalin
I think that "early" means "premature" in the context of the quote.
Mark Cidade
I am slightly confused here, I thought that each thread has its *own* stack space. As count is an int (stack variable), surely this value would be isolated to an individual thread, on its own stack?
miguel
@miguel: If _count_ was a local variable then you are correct, it would only exist on the stack, each thread would have their own _count_, and there would be no thread safety issue. However, _count_ is a field, so does not live only on the stack and multiple threads can access it.
Matt Howells