I've written this class as an exercise for a synchronization construct to be used across multiple threads.

The intent is to have worker threads Increment() it, do some work, then finally Decrement()it.

Meanwhile, when a master thread decides to shut down the program, it signals the threads using a CancellationToken and Wait()s on this structure until all threads have ceased (signalled by _number hitting zero).

I am depending mainly on the fact that Interlocked operations imply full memory fences. I am concerned with the read on the Wait() method, see the comment there.

public class CountdownLatch
{
int _number;

public CountdownLatch()
{
this._event = new ManualResetEventSlim();
}

public void Increment()
{
Interlocked.Increment(ref this._number);
}

public void Decrement()
{
int currentNumber;
bool firstAttempt = true;

do
{
currentNumber = this._number;
if (!firstAttempt)
{
var spinWait = new SpinWait();
spinWait.SpinOnce();
}
else
{
firstAttempt = false;
}

if (currentNumber == 0)
{
throw new InvalidOperationException("Attempt to decrement past zero");
}
} while (Interlocked.CompareExchange(ref this._number, currentNumber - 1, currentNumber) != currentNumber);

if (currentNumber == 1)
{
this._event.Set();
}
}

public bool Wait(TimeSpan timeout)
{
// AFAICT the memory barrier here would prevent instruction reordering
// when the order of reads/writes matter, but here it doesn't. I just care
// to have the latest value of this._number across all CPUs/cores.
// Or does volatile read in C# specifically do both ?
if (this._number == 0)
{
return true;
}

return this._event.Wait(timeout);
}
}


### The Good

I don't write multithreaded code very often, so other reviewers will probably have other comments (read: I'm a beginner myself), but I like what I'm seeing.

Interlocked.Increment(ref this._number);


I don't see anything that could go wrong with this. This is how I've read a thread-safe increment should be done, using the Interlocked class.

while (Interlocked.CompareExchange(ref this._number, currentNumber - 1, currentNumber) != currentNumber)


Looks good to me. currentNumber belongs to the thread that's running this, so no thread can mess with that currentNumber while currentNumber - 1 or CompareExchange is being evaluated, and again the Interlocked class at play has my full trust.

Ignorance is bliss, nothing in sight.

### The Ugly

The very first thing that jumped at me, is the abundance of redundant this qualifiers. They're not needed, you have underscores:

int _number;


I like the underscore prefix, because it allows me to have a _number field and a number parameter or local variable, for example:

public CountdownLatch(ManualResetEventSlim event)
{
_event = event;
}


The way I read currentNumber, "current" stands for "current thread's working copy of _number". I find the name number conveys that more succinctly.

• I was hoping for some feedback on thread-safety rather than code style :-). I use "this" to increase readability, it is part of a code convention I follow in all my code. Your understanding of "current" is exactly right. I meant to emphasize that I am making a copy of shared memory to a private variable. Apr 6, 2014 at 15:52
• I'm not experienced enough with threaded code to specifically address the read in Wait; I preferred leaving that to other reviewers. Remember that CR answers may address any aspect of the code; if you used ReSharper you'd see code inspections suggesting to remove the redundant this qualifiers (they'd show up faded, too). It might be personal preference, but I find non-needed qualifiers clutter up the code, whether it's this or a namespace. Apr 6, 2014 at 16:18