5
\$\begingroup\$

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);
    }
}
\$\endgroup\$
2
\$\begingroup\$

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.


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.

\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$ – Raif Atef Apr 6 '14 at 15:52
  • \$\begingroup\$ 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. \$\endgroup\$ – Mathieu Guindon Apr 6 '14 at 16:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.