C# Threading issue with AutoResetEvent

Question

How to properly synchronize this? At the moment it is possible that SetData is called after e.WaitOne() has completed so d could be already set to another value. I tried to insert locks but it resulted into a deadlock.

AutoResetEvent e = new AutoResetEvent(false);

public SetData(MyData d)
{
   this.d=d;
   e.Set();    // notify that new data is available
}

// This runs in separate thread and waits for d to be set to a new value
void Runner() 
{    
   while (true)
   {
      e.WaitOne();  // waits for new data to process
      DoLongOperationWith_d(d);
   }
}

Will the best solution be to introduce a new boolean variable dataAlreadyBeenSetAndWaitingToBeProcessed that is set in SetData to true and at the end of DoLongOperationWith_d it could be set to true, so if SetData is called with this variable set to true it could just return?

Whipperin

Answer 1

This is untested, but is an elegant way to do this with the .net based primitives:

class Processor<T> {
    Action<T> action;
    Queue<T> queue = new Queue<T>();

    public Processor(Action<T> action) {
        this.action = action;
        new Thread(new ThreadStart(ThreadProc)).Start();
    }

    public void Queue(T data) {
        lock (queue) {
            queue.Enqueue(data);
            Monitor.Pulse(queue); 
        }            
    }

    void ThreadProc() {
        Monitor.Enter(queue);
        Queue<T> copy;

        while (true) {                 
            if (queue.Count == 0) {
                Monitor.Wait(queue);
            }

            copy = new Queue<T>(queue);
            queue.Clear();
            Monitor.Exit(queue);

            foreach (var item in copy) {
                action(item); 
            }

            Monitor.Enter(queue); 
        }
    }
}

Example program:

class Program {

    static void Main(string[] args) {

        Processor<int> p = new Processor<int>((data) => { Console.WriteLine(data);  });
        p.Queue(1);
        p.Queue(2); 

        Console.Read();

        p.Queue(3);
    }
}

This is a non-queue version, a queue version may be preferred:

object sync = new object(); 
AutoResetEvent e = new AutoResetEvent(false);
bool pending = false; 

public SetData(MyData d)
{
   lock(sync) 
   {
      if (pending) throw(new CanNotSetDataException()); 

      this.d=d;
      pending = true;
   }

   e.Set();    // notify that new data is available
}

void Runner() // this runs in separate thread and waits for d to be set to a new value
{

     while (true)
     {

             e.WaitOne();  // waits for new data to process
             DoLongOperationWith_d(d);
             lock(sync) 
             {
                pending = false; 
             }
     }
}

Lunetta

@Spencer Ruport: What? If pending is set to true the first time SetData is called, then it will throw the second time. I'm sure there's some way to break this, but I don't think it's with the sequence you described. – Onto

but this.d can not be set unless pending is false. – Lunetta

My bad. I didn't see the if(pending) there. – Elkins

in my opinion, the lock(sync) is not really necessary because boolean assignment is atomic – Whipperin

well, the problem with the second sample is that data can sneak in without triggering the autoreset event on time. the queue based approach is much cleaner. – Lunetta

Do I have to copy the Queue? Would it work to do the following: while (true) { lock (queue) { if (queue.Empty) Monitor.Wait(queue); action(dequeue()); } } – Whipperin

it would but you would then be locked while processing the items, which would mean you could not queue more items while processing. – Lunetta

For info, I'd add a "continue" after the Wait (when the queue is empty) - just in case you ever need to pulse the object for any other reason (for example, to indicate "exit yourself"), it is a good idea to re-confirm any assumptions like "there is data" – Meares

Answer 2

There are two possibly troubling scenarios here.

1:

• DoLongOperationWith_d(d) finishes.
• SetData() is called, storing a new value in d.
• e.WaitOne() is called, but since a value has already been set the thread waits forever.

If that's your concern, I think you can relax. From the documentation, we see that

If a thread calls WaitOne while the AutoResetEvent is in the signaled state, the thread does not block. The AutoResetEvent releases the thread immediately and returns to the non-signaled state.

So that's not a problem. However, depending on how and when SetData() is called, you may be dealing with the more serious

2:

• SetData() is called, storing a new value in d and waking up the runner.
• DoLongOperationWith_d(d) starts.
• SetData() is called again, storing a new value in d.
• SetData() is called again! The old value of d is lost forever; DoLongOperationWith_d() will never be invoked upon it.

If that's your problem, the simplest way to solve it is a concurrent queue. Implementations abound.

Fagot

Answer 3

You can use 2 events,

AutoResetEvent e = new AutoResetEvent(false);
AutoResetEvent readyForMore = new AutoResetEvent(true); // Initially signaled

public SetData(MyData d)
{
   // This will immediately determine if readyForMore is set or not.
   if( readyForMore.WaitOne(0,true) ) {
     this.d=d;
     e.Set();    // notify that new data is available
  }
  // you could return a bool or something to indicate it bailed.
}

void Runner() // this runs in separate thread and waits for d to be set to a new value
{

     while (true)
     {

             e.WaitOne();  // waits for new data to process
             DoLongOperationWith_d(d);
             readyForMore.Set();
     }
}

One of the things you can do with this approach is have SetData take a timeout, and pass that into WaitOne. I think however you shoudl investigate ThreadPool.QueueUserWorkItem.

Allopath

The trouble with this is that SetData will be blocked as soon as it starts processing. – Lunetta

The queue is obviously superior. I just tried to answer the question exactly as asked. – Allopath