Simple producer-consumer collection without BlockingCollection

Question

I want to write a simple producer-consumer queue, without using the built-in System.Collections.Concurrent.BlockingCollection. Here's a quick attempt that "seems" to work. Is there anything wrong with it threading-wise, race conditions, deadlocks etc.?

class ProducerConsumerQueue<T>
{
    Queue<T> Queue = new Queue<T>();
    ManualResetEvent Event = new ManualResetEvent(false);
    object Lock = new object();

    public void Add(T t)
    {
        lock (Lock)
        {
            Queue.Enqueue(t);
        }
        Event.Set();
    }

    public bool TryTake(out T t, int timeout)
    {
        if (Event.WaitOne(timeout))
        {
            lock (Lock)
            {
                if (Queue.Count > 0)
                {
                    t = Queue.Dequeue();
                    if (Queue.Count == 0) Event.Reset();
                    return true;
                }
            }
        }
        t = default(T);
        return false;
    }
}

Btw. the only two methods I need are Add and TryTake, I don't need IEnumerable etc.

why not to use better implementations(better performance without deadlocks) like `ConcurrentQueue` ? — ibubi, Dec 17 '18 at 08:22
@ibubi I need timeout when removing items from queue, ConcurrentQueue does not have that. — kaalus, Dec 17 '18 at 08:30
This is better suited for https://codereview.stackexchange.com/ — FCin, Dec 17 '18 at 08:32
You want a blocking Queue with a TryTake method in which you can pass a timeout? — Hasan Emrah Süngü, Dec 17 '18 at 08:34
@kaalus, then how about changing `Queue Queue = new Queue();` into `ConcurrentQueue` so that you get rid of `Lock` then instead of ManualResetEvent, into `Semaphoreslim`. Other than that it looks like what you have is working — Hasan Emrah Süngü, Dec 17 '18 at 08:41
@kaalus, It is just a hunch but are you somehow trying to replicate golang`s channel feature? — Hasan Emrah Süngü, Dec 17 '18 at 08:43
@EmrahSüngü No, sorry, I'm not trying to replicate anything in golang. — kaalus, Dec 17 '18 at 08:48
The [`BlockingCollection.TryTake`](https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.blockingcollection-1.trytake) method has an optional `timeout` parameter, and is available from .NET Framework 4.0 onwards. Why don't you want to use it? — Theodor Zoulias, Apr 09 '22 at 03:28

Marc Gravell · Answer 1 · 2018-12-17T09:25:49.983

Microsoft recently dropped System.Threading.Channels, which is intended to provide optimized producer/consumer APIs, which may be a good fit in this case. It covers unbounded and bounded scenarios, and includes single plus multiple reader/writer scenarios. The API is pretty simple and intuitive to use; the only slight caveat is that it uses an async-orientated API (for the consumer, and - in the case of bounded channels - for the producer).

The point here being: the code you don't write tends to be code that has fewer pain points - especially if it was written by a team with expertise and interest in the specific problems being targeted.

However: you can do everything in your current code without needing the ManualResetEvent - lock in C# is just a wrapper around the simplest parts of Monitor, but Monitor also provides wait/pulse functionality:

class ProducerConsumerQueue<T>
{
    private readonly Queue<T> Queue = new Queue<T>();

    public void Add(T t)
    {
        lock (Queue)
        {
            Queue.Enqueue(t);
            if (Queue.Count == 1)
            {
                // wake up one sleeper
                Monitor.Pulse(Queue);
            }
        }
    }

    public bool TryTake(out T t, int millisecondsTimeout)
    {
        lock (Queue)
        {
            if (Queue.Count == 0)
            {
                // try and wait for arrival
                Monitor.Wait(Queue, millisecondsTimeout);
            }
            if (Queue.Count != 0)
            {
                t = Queue.Dequeue();
                return true;
            }
        }
        t = default(T);
        return false;
    }
}

Thanks Marc. I've been analyzing your code, and I don't fully understand one thing. AFAIK Monitor.Pulse does not have internal state, therefore if no one is waiting on the monitor, the pulse will not wake up a reader and the first item in the queue will never be retrieved. Actually another thing is, if someone is waiting on the monitor in TryTake, he's holding a Queue lock, therefore no one can pulse the monitor in Add, which is in the same lock. Please let me know if I get this wrong. — kaalus, Dec 18 '18 at 00:20
@kaalus, there is a time out on Monitor.Wait. if nobody pulses it will fail to acquire the lock then continie — Hasan Emrah Süngü, Dec 18 '18 at 01:22
@kaalus when you call `Wait`, it puts the current thread into the pending queue for that monitor, and *releases* the lock for the duration of the wait; when the `Wait` completes (either by pulse or by timeout), the thread *re-acquires* the lock. So; `Wait` is a temporary surrender of the lock, with a pulse/timeout mechanism. — Marc Gravell, Dec 18 '18 at 10:26

score 0 · Answer 2 · answered Dec 17 '18 at 08:29

0

I think using both lock and a ManualResetEvent is redundant. I suggest you read more about ManualResetEvent on how to enter and exit synchronized areas in your code (you can also take a look at other synchronization mechanisms available under System.Threading).

If it's not just for exercise, you can also take a look at NetMQ.

Hope it helps!

answered Dec 17 '18 at 08:29

Itay Podhajcer

2,616
2
9
14

Good, though if OP isn't going to use first-class `BlockingCollection` he probably isn't going to use a 3rd party solution like NetMQ – Dec 17 '18 at 08:36

score -1 · Answer 3 · answered Dec 17 '18 at 09:10

As per my comment in the question,

Here is a my proposed solution.

public class BlockingQueue<T>
{
    // In order to get rid of Lock object
    // Any thread should be able to add items to the queue
    private readonly ConcurrentQueue<T> _queue = new ConcurrentQueue<T>();

    // Only one thread is able to consume from queue
    // You can fine tune this to your interest
    private readonly SemaphoreSlim _slim = new SemaphoreSlim(1,1);

    public void Add(T item) {
        _queue.Enqueue(item);
    }

    public bool TryTake(out T item, TimeSpan timeout) {
        if (_slim.Wait(timeout)){
            return _queue.TryDequeue(out item);
        }
        item = default(T);
        return false;
    }
}

For the downvoter, I am more than happy to fix any mistakes if you tell me the problem :) thanks a lot — Hasan Emrah Süngü, Dec 18 '18 at 00:47
The `SemaphoreSlim` is never released. The `Add` method does not awake a waiting consumer. The `SemaphoreSlim` is a sufficient mechanism to use for this purpose, but it is not used correctly in the above code. — Theodor Zoulias, Apr 09 '22 at 03:38

Simple producer-consumer collection without BlockingCollection

3 Answers3