When should the lock be used?
A lock should be used to protect shared resources in multithreaded code. Not for anything else.
But is it necessary when the application does not spin off any other threads?
Absolutely not. It's just a time waster. However do be sure that you're not implicitly using system threads. For example if you use asynchronous I/O you may receive callbacks from a random thread, not your original thread.
Is there performance issues with using lock?
Yes. They're not very big in a single-threaded application, but why make calls you don't need?
...if that is a good design pattern to follow in the future[?]
Locking everything willy-nilly is a terrible design pattern. If your code is cluttered with random locking and then you do decide to use a background thread for some work, you're likely to run into deadlocks. Sharing a resource between multiple threads requires careful design, and the more you can isolate the tricky part, the better.
Worst (won't actually work)
Change the access modifier of counter
to public volatile
As other people have mentioned, this on its own isn't actually safe at all. The point of volatile
is that multiple threads running on multiple CPUs can and will cache data and re-order instructions.
If it is not volatile
, and CPU A increments a value, then CPU B may not actually see that incremented value until some time later, which may cause problems.
If it is volatile
, this just ensures the two CPUs see the same data at the same time. It doesn't stop them at all from interleaving their reads and write operations which is the problem you are trying to avoid.
Second Best:
lock(this.locker) this.counter++
;
This is safe to do (provided you remember to lock
everywhere else that you access this.counter
). It prevents any other threads from executing any other code which is guarded by locker
.
Using locks also, prevents the multi-CPU reordering problems as above, which is great.
The problem is, locking is slow, and if you re-use the locker
in some other place which is not really related then you can end up blocking your other threads for no reason.
Best
Interlocked.Increment(ref this.counter);
This is safe, as it effectively does the read, increment, and write in 'one hit' which can't be interrupted. Because of this, it won't affect any other code, and you don't need to remember to lock elsewhere either. It's also very fast (as MSDN says, on modern CPUs, this is often literally a single CPU instruction).
I'm not entirely sure however if it gets around other CPUs reordering things, or if you also need to combine volatile with the increment.
InterlockedNotes:
- INTERLOCKED METHODS ARE CONCURRENTLY SAFE ON ANY NUMBER OF COREs OR CPUs.
- Interlocked methods apply a full fence around instructions they execute, so reordering does not happen.
- Interlocked methods do not need or even do not support access to a volatile field, as volatile is placed a half fence around operations on given field and interlocked is using the full fence.
Footnote: What volatile is actually good for.
As volatile
doesn't prevent these kinds of multithreading issues, what's it for? A good example is saying you have two threads, one which always writes to a variable (say queueLength
), and one which always reads from that same variable.
If queueLength
is not volatile, thread A may write five times, but thread B may see those writes as being delayed (or even potentially in the wrong order).
A solution would be to lock, but you could also use volatile in this situation. This would ensure that thread B will always see the most up-to-date thing that thread A has written. Note however that this logic only works if you have writers who never read, and readers who never write, and if the thing you're writing is an atomic value. As soon as you do a single read-modify-write, you need to go to Interlocked operations or use a Lock.
Best Answer
It is bad form to use
this
in lock statements because it is generally out of your control who else might be locking on that object.In order to properly plan parallel operations, special care should be taken to consider possible deadlock situations, and having an unknown number of lock entry points hinders this. For example, any one with a reference to the object can lock on it without the object designer/creator knowing about it. This increases the complexity of multi-threaded solutions and might affect their correctness.
A private field is usually a better option as the compiler will enforce access restrictions to it, and it will encapsulate the locking mechanism. Using
this
violates encapsulation by exposing part of your locking implementation to the public. It is also not clear that you will be acquiring a lock onthis
unless it has been documented. Even then, relying on documentation to prevent a problem is sub-optimal.Finally, there is the common misconception that
lock(this)
actually modifies the object passed as a parameter, and in some way makes it read-only or inaccessible. This is false. The object passed as a parameter tolock
merely serves as a key. If a lock is already being held on that key, the lock cannot be made; otherwise, the lock is allowed.This is why it's bad to use strings as the keys in
lock
statements, since they are immutable and are shared/accessible across parts of the application. You should use a private variable instead, anObject
instance will do nicely.Run the following C# code as an example.
Console output