Locking
Education means a lot. When we are working on a system and we are developing features we need to understand the constructs that we use. Locking for instance. Yes it can be tricky, yes it may not be easy to understand, it may even be hard to test. If you are not sure about something you need to read about it, consult a more experienced developer, whatever it takes for you to reasonably understand what it is you are writing. Nothing you write as a programmer should be magic to you - at least once you are past the Junior Developer stage.Lets Dig In
I've seen this a few times in code that is in the wild at large corporations.How Not To Lock
In the code below look at the method NotALock(). An experienced developer should immediately see this and know there is something wrong
using System; using System.Threading; using System.Threading.Tasks; namespace This_Is_Not_A_Lock { public class NotLocked { private int counter = 0; private int timesCalled = 0; private static object locker = new object(); public void NotALock() { Interlocked.Increment(ref timesCalled); locker = new object(); lock (locker) { Interlocked.Increment(ref counter); Thread.Sleep(50); Interlocked.Decrement(ref counter); } } public void IsALock() { Interlocked.Increment(ref timesCalled); lock (locker) { Interlocked.Increment(ref counter); Thread.Sleep(50); Interlocked.Decrement(ref counter); } } public void WriteValue() { Console.WriteLine("Counter={0} Called={1}", counter, timesCalled); } public void Reset() { counter = 0; timesCalled = 0; } } internal class Program { private static void Main(string[] args) { Console.WriteLine("\r\nDemonstrating not a lock.\r\n"); var nl = new NotLocked(); var t = Task.Factory.StartNew(() => { while (true) { nl.WriteValue(); Thread.Sleep(500); } }); Parallel.For(0, 500, new ParallelOptions { MaxDegreeOfParallelism = 128 }, (i) => { nl.NotALock(); }); Console.WriteLine("\r\nTransitioning to an actual lock.\r\n"); nl.Reset(); Parallel.For(0, 500, new ParallelOptions { MaxDegreeOfParallelism = 128 }, (i) => { nl.IsALock(); }); } } }
Locking explained
Here is a very nice and technical explanation by Joe Albahari http://www.albahari.com/threading/part2.aspx. This is a mutually exclusive lock - only one thread is allowed inside the lock(locker) { } constraint at a time. When a lock is written correctly it uses the variable lock(variable) to "track" the mutual exclusion. When I say "track" I am just trying to simplify things. You can read more about the actual technical details and realities involved in locking in the link.Here is how I like to explain a lock/mutex. A mutex is a door that controls access to a room. The room is the code between the curly braces. At the final/end curly brace the "door" is unlocked. Correct code looks like this. If you are a geek and following along - the door object would be static and be initialized once in the static constructor.
lock (door) { room(); }
The door is the control point, no one can gain access to the room except through the door. If it is written properly locks work just fine. However if you write the lock like the NotALock() method then you basically get this.
door = new object(); lock (door) { room(); }
What this code does is create a new door to the "room" for everyone - we call those threads - that wants to enter the room. Each door will only let one thread in - but we are creating a new door every single time we want in. Imagine this...
Yeah, if you've seen Monsters Inc you know the scene. Doors everywhere. Well that's what happens when you new up a door object just before using it in a lock statement.
Not only are you creating a new door, you are creating an object that as soon as that lock is over, has to be garbage collected.
What To Do
If you see this in code that you work on. First, be brave. Second, explain to your team/manager/product owner why it HAS to be fixed. This is clearly not the intention of the original writer of the code. Locks written like this have no purpose, they do nothing and whatever they are supposed to be protecting is no longer protected.If you have to explain it to someone who is not as technical then use the door analogy, it's not perfect but I find that it typically works pretty well.
The Code
You can just copy and paste the code into a command line C# app (program.cs) and run it.
The lock should only allow the value of counter to become 1/one. Never more than one
You'll see that NotALock() does not keep the counter from becoming more than 1/one. It seems to max out at about 5/five on my machine which is a quad core AMD A10. When the code gets to the transition point and switches to the correct locking mechanism you will see that the value NEVER increases above 1/one. Another side effect of the code being correct. The code that calls IsALock() takes longer to execute. Why is that? That is because the lock is working and only one thread is inside at anytime. And each thread has to wait individually at the Thread.Sleep(50) method call. So each thread has to wait 50 milliseconds. so the given number of calls is 500, in the Parallel.For, so 500 * 50 is 25,000 milliseconds, so the code should take at least 25 seconds to run.
Egoless programming
You are not the code you wrote yesterday. Your code does not define you. Code reviews are important. If you see something that you feel or you know is broken you MUST say something. The person who wrote the code should not take offense because we are not the code we wrote... No one is attacking anyone for the code they wrote, we are just asking questions, checking assumptions and sometimes educating.
