Is this producer/consumer model stable?

[Guest]

I am developing an application that has several multi threaded tasks where
one thread is doing IO and another thread is grabbing data from the first
thread to process it further.

I've been noticing that every once in a while my program simply hangs- it is
rare but it is annoying. I am worried it may be a synchronization issue
involving my producer/consumer threads.

I basically go like this:

AutoResetEvent autoEvent = new AutoResetEvent(false);
Queue q = new Queue();

private void AddToken (Token t)
{

lock (q)
{
q.Enqueue(t);
if (q.Count == 1) { autoEvent.Set(); }
}

}

public bool HasMoreTokens ()
{
if (status < StatusType.DONE && q.Count == 0)
{
autoEvent.WaitOne(); autoEvent.Reset();
}
return (q.Count > 0);
}

public Token NextToken ()
{
while (status < StatusType.DONE && q.Count == 0)
{
autoEvent.WaitOne(); autoEvent.Reset();
}
lock (q)
{
return (Token)q.Dequeue();
}
}

So the basic idea here is my producer makes token objects which get pushed
onto a Queue using the AddToken method. If the queue was empty, it sets the
AutoResetEvent so any waiting consumer thread is awakened.

A consumer thread would either call HasMoreTokens or NextToken which will
wait if the status is not 'DONE' (aka the producer is still working) and
the queue is empty. Once a token on the queue is available, it will get
awoken by the producer.

Does this look flawed?

 

[Jon Skeet [C# MVP]]

I am developing an application that has several multi threaded tasks where
one thread is doing IO and another thread is grabbing data from the first
thread to process it further.

I've been noticing that every once in a while my program simply hangs- it is
rare but it is annoying. I am worried it may be a synchronization issue
involving my producer/consumer threads.

There are a few problems in your code:

1) You should *always* lock the queue when you use it.
2) Your access to status isn't thread-safe
3) You should do the waiting and the dequeuing within a single lock
4) You don't need to call Reset on the autoEvent - that's what the
AutoReset part is there for Currently there's a race condition
there. I would actually suggest using Wait/Pulse instead of
AutoResetEvent, but that's just me.
5) Calling HasMoreTokens can "eat" the signal which another thread
could be waiting for. I would personally ditch the HasMoreTokens
method, making NextToken just return null if the status becomes
DONE while there are no items in the queue.

See http://www.pobox.com/~skeet/csharp/threads/deadlocks.shtml for my
own producer/consumer class.

 

[Guest]

Thanks Jon,

I have ditched the mess I had and implemented something similar to your
example. Now there's only a NextToken method and the thread handling IO will
put an EOF when done Token so the consumer will know to stop when it reads
such a token, so that whole status flag was dumped as well.

The only difference is I don't create a lock object, instead I just lock the
queue itself. Is there any reason why you do not lock the queue instead of
using a lock object?

 

[Jon Skeet [C# MVP]]

I have ditched the mess I had and implemented something similar to your
example. Now there's only a NextToken method and the thread handling IO will
put an EOF when done Token so the consumer will know to stop when it reads
such a token, so that whole status flag was dumped as well.

Good - I was thinking overnight about the best way of signalling an
end, and had come up with the same solution. The only difficulty is
that if you have more than one consumer thread, you need to know how
many EOF tokens to put in the queue.

The only difference is I don't create a lock object, instead I just lock the
queue itself. Is there any reason why you do not lock the queue instead of
using a lock object?

In this case it would be okay to lock on the queue, as every time I use
the queue I'm already locking it, and nothing else has access to it.
However, in general I prefer to have separate private references so
that I can guarantee that the only code locking it is the code I write.