CountdownLatch Thread-Safety Check

Question

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;
    readonly ManualResetEventSlim _event;

    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)
    {
        // Should this be a volatile read: Volatile.Read(ref this._number) ?
        // 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);
    }
}

Answer 1

The Good

I don't write multithreaded code very often, so other reviewers will probably have other comments (read: I'm a multithreading 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.

---

The Bad

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;
readonly ManualResetEventSlim _event;

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.