Is this BlockingQueue Thread safe?

Question

I was trying myself on implementing a minimal thread safe blocking queue, what I came up with is:

class BlockingQueue<T>
{
    private Queue<T> myQueue = new Queue<T>();
    private SemaphoreSlim semaPhore = new SemaphoreSlim(0); 
    public void Enqueue(T t)
    {
        lock(myQueue)
        {
            myQueue.Enqueue(t);
            semaPhore.Release();
        }
    }
    public T Dequeue()
    {       
        semaPhore.Wait();
        lock(myQueue)
        {
            return myQueue.Dequeue();
        }
    }
}

I tried stress testing it with several producers and consumers at the same time enqueueing / dequeueing at random time intervals and it didn't fail.

However, if I look critically at the code, could something happen between the "semaPhore.Wait()" and the "lock(myQueue)" command?

Are you trying to learn how to write a `BlockingQueue`, or is it for work? Because in .NET 4.0 there is a `BlockingQueue<>` — xanatos, Jun 05 '15 at 11:30
May be better placed on [codereview.stackexchange.com](https://codereview.stackexchange.com/)? — wonderb0lt, Jun 05 '15 at 11:31
I'm voting to close this question as off-topic because it should be moved to CodeReview. — , Jun 05 '15 at 11:32
@AndreasNiedermair There is a concrete question: *However, if I look critically at the code, could something happen between the "semaPhore.Wait()" and the "lock(myQueue)" command?*? — xanatos, Jun 05 '15 at 11:36
@wonderb0lt The OP suspects a very specific part of the code may not work as expected. That makes it off-topic for Code Review. — Mast, Jun 05 '15 at 11:39
I am aware that there is a BlockingQueue, this was learning purposes. — trykyn, Jun 05 '15 at 11:46
Just realized, the thing I was suspecting is: 2 threads try to dequeue on an empty queue, then elements A and B are enqueued. What could happen that when element A is added, the semaphore triggers, and before the lock, the semaphore could trigger again and the second consumer would lock it first. But this doesn't pose a problem, because now there are 2 elements there, so both may just dequeue fine and I didn't say anything about the order — trykyn, Jun 05 '15 at 11:53

xanatos · Accepted Answer · 2015-06-05T11:47:01.000

2

Yes it could happen... A Thread.Abort() would break this queue.

semaPhore.Wait();

// Here a Thread.Abort() happens

lock(myQueue) ...

Then what happens is that there is one element in the queue that no one will be able to recover, because there is one less "free" semaphore slot than queue items.

Other than this (and the fact that there is a BlockingCollection<> written by experts in .NET 4.0), I will say that the code is correct. semaPhore.Release() causes an implicit barrier (because internally it uses a lock), so there are no problems of the order of writings in memory (first the Enqueue() will be really done, then the Release() will be done). Note that it will be under-performing, because each operation requires two lock (yours plus the one of SemaphoreSlim).

edited Jun 05 '15 at 11:47

answered Jun 05 '15 at 11:34

xanatos

109,618
12
197
280

3

Correct, but Thread.Abort() will break a lot of things. It's exotic enough that I would explicitly include it in the requirements for such a queue. – H H Jun 05 '15 at 11:38
@HenkHolterman You are quite right... But Microsoft altered the `Monitor.Enter` just for the `Thread.Abort` (and then interestingly didn't alter all the other synchronization objects...) – xanatos Jun 05 '15 at 11:40
so if I'd want to consider this case of Thread.Abort(), I should go the long way, similar to microsoft lock(){} implementation – trykyn Jun 05 '15 at 11:49
@trykyn Yes... You would need to "merge" the `SemaphoreSlim` (that internally uses `lock`, `Monitor.Pulse`...) with the handling of the `Queue<>` – xanatos Jun 05 '15 at 11:57
1

And with Monitor.Wait and .Pulse you wouldn't need an extra sync object. – H H Jun 05 '15 at 11:59
@trykyn Note that I consider `Monitor.Pulse`/`Monitor.PulseAll` to be "advanced voodoo magic" :-) – xanatos Jun 05 '15 at 12:00

score -1 · Answer 2 · answered Jun 05 '15 at 13:28

Using the suggestions I now came up with this, surprisingly it uses even less lines of code :)

class BlockingQueue<T>
{
    private Queue<T> myQueue = new Queue<T>();

    public void Enqueue(T t)
    {
        lock(myQueue)
        {
            myQueue.Enqueue(t);
            Monitor.Pulse(myQueue);
        }
    }
    public T Dequeue()
    {       
        lock(myQueue)
        {
            Monitor.Wait(myQueue);
            return myQueue.Dequeue();
        }
    }
}

OK, and what happens when a Thread.Abort() happens between Enqueue() and Pulse() ? — H H, Jun 05 '15 at 18:29

Is this BlockingQueue Thread safe?

2 Answers2