HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Producer/Consumer queue

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
producerqueueconsumer

Problem

I use this class for scheduling bits of work on a single thread. Work can be scheduled to be executed immediately or at a future time with the use of timers. It has many producers (the work adders) and a single consumer (the thread which executes the work).

This class works well but I'm under no illusion that it's perfect. I'm looking for potential problems this class may exhibit especially relating to threading (dead locks etc).

```
internal delegate void MainLoopTask();
internal delegate object MainLoopJob();

internal static class MainLoop
{
private class DelegateTask
{
private bool _isBlocking;
private Exception _storedException;
private MainLoopTask _task;
private MainLoopJob _job;
private object _jobResult;
private ManualResetEvent _handle;

public DelegateTask()
{

}

public bool IsBlocking
{
get { return _isBlocking; }
set { _isBlocking = value; }
}

public Exception StoredException
{
get { return _storedException; }
set { _storedException = value; }
}

public object JobResult
{
get { return _jobResult; }
}

public MainLoopTask Task
{
get { return _task; }
set { _task = value; }
}

public MainLoopJob Job
{
get { return _job; }
set { _job = value; }
}

public ManualResetEvent WaitHandle
{
get
{
if (_handle == null)
_handle = new ManualResetEvent(false);
return _handle;
}
}

public void Execute()
{
try
{
if (_task != null)
_task();
if (_job != null)
_jobResult = _job();
}
catch (Exception ex)
{

Solution

-
A slightly better way to wait for the queue to be not empty in your Loop method would be to use a Monitor instead of an AutoResetEvent.

  • You would get rid of _handle



  • Replace all occurrences of _handle.Set() with Monitor.Pulse(_tasks)



-
Your waiting code would look like this:

DelegateTask task;
    lock (_tasks)
    {
        while (_tasks.Count == 0)
        {
            Monitor.Wait(_tasks);
        }
        task = _tasks.Dequeue();
    }

    task.Execute();


Slightly more compact code.

-
Your should get into the habit of creating dedicated objects for locking instead of locking on some data structure you happen to use. In your case it won't make much of a difference but you can get into deadlocks this way if you are not careful.

Code Snippets

DelegateTask task;
    lock (_tasks)
    {
        while (_tasks.Count == 0)
        {
            Monitor.Wait(_tasks);
        }
        task = _tasks.Dequeue();
    }

    task.Execute();

Context

StackExchange Code Review Q#10042, answer score: 2

Revisions (0)

No revisions yet.