37

My main concern is with the boolean flag... is it safe to use it without any synchronization? I've read in several places that it's atomic (including the documentation).

class MyTask
{
    private ManualResetEvent startSignal;
    private CountDownLatch latch;
    private bool running;

    MyTask(CountDownLatch latch)
    {
        running = false;
        this.latch = latch;
        startSignal = new ManualResetEvent(false);
    }

    // A method which runs in a thread
    public void Run()
    {
        startSignal.WaitOne();
        while(running)
        {
            startSignal.WaitOne();
            //... some code
        }
        latch.Signal();
    }

    public void Stop()
    {
        running = false;
        startSignal.Set();
    }

    public void Start()
    {
        running = true;
        startSignal.Set();
    }

    public void Pause()
    {
        startSignal.Reset();
    }

    public void Resume()
    {
        startSignal.Set();
    }
}

Is this a safe way to design a task in this way? Any suggestions, improvements, comments?

Note: I wrote my custom CountDownLatch class in case you're wondering where I'm getting it from.

Update:
Here is my CountDownLatch too: 

public class CountDownLatch 
{
    private volatile int m_remain;
    private EventWaitHandle m_event;

    public CountDownLatch (int count)
    {
        if (count < 0)
            throw new ArgumentOutOfRangeException();
        m_remain = count;
        m_event = new ManualResetEvent(false);
        if (m_remain == 0)
        {
            m_event.Set();
        }
    }

    public void Signal()
    {
        // The last thread to signal also sets the event.
        if (Interlocked.Decrement(ref m_remain) == 0)
            m_event.Set();
    }

    public void Wait()
    {
        m_event.WaitOne();
    }
}
Kiril
  • 39,672
  • 31
  • 167
  • 226

4 Answers4

49

You better mark it volatile though:

The volatile keyword indicates that a field might be modified by multiple concurrently executing threads. Fields that are declared volatile are not subject to compiler optimizations that assume access by a single thread. This ensures that the most up-to-date value is present in the field at all times.

But I would change your loop:

    startSignal.WaitOne();
    while(running)
    {
        //... some code
        startSignal.WaitOne();
    }

As it is in your post the 'some code' might execute when the thread is stopped (ie. when Stop is called) which is unexpected and may be even incorrect.

Remus Rusanu
  • 288,378
  • 40
  • 442
  • 569
  • @Remus Good catch in my while loop code order. Regarding the volatile flag: does it really make a difference if the flag is volatile in this case? If it misses the first update, then it's going to do one more run through the loop and catch it the next time around... – Kiril Mar 19 '10 at 04:31
  • 6
    If you don't mark it volatile, the generated code might optimize the value into a registry and your thread will *never* see the change. – Remus Rusanu Mar 19 '10 at 04:34
  • 4
    @Remus I get it now: the atomicity has nothing to do with the visibility between threads... just because an operation is executed in one CPU cycle it doesn't mean that the result will be visible to the other threads unless the value is marked volatile. – Kiril Mar 19 '10 at 04:40
  • Since we're at it: your latch.Release() must be in a finally block. And since you wrote it yourself, you better make sure the latch is correct too, there could be a mirriad gotchats in that too. – Remus Rusanu Mar 19 '10 at 04:56
  • @Remus I've updated my question and added the CountDownLatch, but I think it's fine... it's pretty simple and it has been working fine. – Kiril Mar 19 '10 at 05:09
  • 14
    @Lirik: what you just said is extremely important so let's repeat it: **in the most general case, atomicity has nothing whatsoever to do with volatility**. The rules of *C#* are such that everything volatile is also atomic, but not the other way around; some things are atomic without being volatile. The CLR makes no such guarantee; in the CLR, volatility and atomicity are orthogonal. You can have nonatomic-nonvolatile, atomic-nonvolatile, nonatomic-volatile and atomic-volatile reads and writes. – Eric Lippert Mar 19 '10 at 05:24
  • @Eric That clarification is indeed **VERY** important. – Kiril Mar 19 '10 at 05:34
6

Booleans are atomic in C#, however, if you want to modify it in one thread and read it in another, you will need to mark it volatile at the very least,. Otherwise the reading thread may only actually read it once into a register.

Michael Burr
  • 333,147
  • 50
  • 533
  • 760
  • @Michael So how many cycles through the loop can realistically pass before the loop catches it? I assumed that it would be one, but I assume there is no such guarantee... – Kiril Mar 19 '10 at 04:35
  • @Michael I got it now: I just realized that I was confusing atomicity with visibility. – Kiril Mar 19 '10 at 04:42
2

Booleans are atomic in C#: http://msdn.microsoft.com/en-us/library/aa691278(VS.71).aspx

Kevin Sylvestre
  • 37,288
  • 33
  • 152
  • 232
0

BTW, I just noticed this part of the code:

// A method which runs in a thread
    public void Run()
    {
        startSignal.WaitOne();
        while(running)
        {
            startSignal.WaitOne();
            //... some code
        }
        latch.Signal();
    }

You will need to unblock the worker thread twice using "startSignal.Set()" for the code within the while block to execute.

Is this deliberate?

akapet
  • 31
  • 3
  • 2
    @Akapetronics startSignal only needs to be set once. WaitOne doesn't reset the event, so I can call startSignal.WaitOne() multiple times and it will not block until startSignal.Reset() is called. The design is deliberate: the thread will block until the Start method is called and the Start method first sets the running flag and sets the startSignal so the while loop can begin execution. If I reverse the order and first set the startSignal then the while loop might exit before I even set the running flag. Notice that this design eliminates the need for locks. – Kiril Mar 19 '10 at 05:15