Using AutoResetEvent to signal worker thread

I have a service that is running constantly processing data, it receives requests to process new data through messaging. While it's busy processing new requests get merged together so that they are then all processed at once. An AutoResetEvent is used to notify the processor that a new request is available.

My question is in EventLoop should it be possible that currentRequest after the WaitOne to be null?

Is it bad practice to have the _eventAvailable.Set() outside of the lock(_eventLocker)? I moved it out so that it wouldn't 开发者_如何学JAVAstart going at the WaitOne and immediately contest the lock(_eventLocker).

Any suggestions on how to better write the following code?

public sealed class RealtimeRunner : MarshalByRefObject
{
    /// <summary>
    /// The actual event, new events get merged into this if it is not null
    /// </summary>
    private Request _pendingRequest;

    /// <summary>
    /// Used to signal the runner thread when an event is available to process
    /// </summary>
    private readonly AutoResetEvent _eventAvailable = new AutoResetEvent(false);
    private readonly object _eventLocker = new object();


    /// <summary>
    /// Called on a background thread via messaging
    /// </summary>
    public void QueueEvent(RealtimeProcessorMessage newRequest)
    {
        bool mergedRequest;
        lock (_eventLocker)
        {
            if (_pendingRequest == null)
            {
                mergedRequest = false;
                _pendingRequest = new Request(newRequest, _engine);
            }
            else
            {
                mergedRequest = true;
                _pendingRequest.Merge(newRequest, _engine);
            }
        }

        _eventAvailable.Set();
    }


    /// <summary>
    /// This is running on its own thread
    /// </summary>
    private void EventLoop()
    {
        while (true)
        {
            // Block until something exists in _pendingRequest
            _eventAvailable.WaitOne();

            Request currentRequest;
            lock (_eventLocker)
            {
                currentRequest = _pendingRequest;
                _pendingRequest = null;
            }

            // CAN THIS EVER BE NULL?
            if (currentRequest == null) 
                continue;

            //do stuff with the currentRequest here
        }
    }
}

Yes, the if (currrentRequest == null) could evaluate to true. Consider two threads racing to call _eventAvailable.Set(). One completes the call and the other gets preempted. Meanwhile the EventLoop thread wakes up and completes an entire iteration of the loop. You now have a situation where _pendingRequest is null and the WaitHandle is still waiting to be signaled again.

I want to present an entirely different solution to the problem. It looks your code could be simplified by using the producer-consumer pattern. This pattern is most easily implemented using a blocking queue. The BlockingCollection class implements such a queue.

public sealed class RealtimeRunner : MarshalByRefObject
{
  private BlockingCollection<Request> m_Queue = new BlockingCollection<Request>();

  public void QueueEvent(RealtimeProcessorMessage newRequest)
  {
    m_Queue.Add(new Request(newRequest _engine));
  }

  private void EventLoop()
  {
    while (true)
    {
      // This blocks until an item appears in the queue.
      Request request = m_Queue.Take();
      // Process the request here.
    }
  }
}