Why is queue lock in this example necessary

Question

I have got the example shown below. I cannot find out why there is lock on queue SyncRoot also while the both coherent algorithms are whole locked using the same object.

It is said that queue lock is necessary.

public class CrudeThreadPool
{
    static readonly int MaxWorkThreads = 4;
    static readonly int WaitTimeout = 2000;

    public delegate void WorkDelegate();

    public CrudeThreadPool() {
        stop = false;
        workLock = new Object();
        workQueue = new Queue();
        threads = new Thread[ MaxWorkThreads ];

        for( int i = 0; i < MaxWorkThreads; ++i ) {
            threads[i] =
                new Thread( new ThreadStart(this.ThreadFunc) );
            threads[i].Start();
        }
    }

    private void ThreadFunc() {
        lock( workLock ) {
            do {
                if( !stop ) {
                    WorkDelegate workItem = null;
                    if( Monitor.Wait(workLock, WaitTimeout) ) {
        
                        lock( workQueue.SyncRoot ) {
                            workItem =
                                (WorkDelegate) workQueue.Dequeue();
                        }
                        workItem();
                    }
                }
            } while( !stop );
        }
    }

    public void SubmitWorkItem( WorkDelegate item ) {
        lock( workLock ) {
            lock( workQueue.SyncRoot ) {
                workQueue.Enqueue( item );
            }

            Monitor.Pulse( workLock );
        }
    }

    public void Shutdown() {
        stop = true;
    }

    private Queue         workQueue;
    private Object        workLock;
    private Thread[]      threads;
    private volatile bool stop;
}

What it the reason for locking on queue SyncRoot, i.e. lock(workQueue.SyncRoot )?

Where did you get that code and why do you think it even works? — Evk, May 15 '16 at 19:06
If you would try to actually run this code, multiple times, you will see that it is broken. You queue 10 items, but random number of items get processed. Sometimes 10, sometimes 8, sometimes 4, sometimes 0. Person who wrote it did not understand what he was doing (especially how Monitor.Pulse and Monitor.Wait work), so not much reason to discuss why he locks over queue access. — Evk, May 16 '16 at 07:15
It is just an example. There is definitely no need to proces any specific count of items. — Yarl, May 16 '16 at 09:21
This code is broken because you might _never_ process items you put in queue. If you put 10 items - you except them all to be processed at least at some time in future. Not the case with this code. With broken code you can add or remove locking over queue - it does not matter. — Evk, May 16 '16 at 09:33

Slugart · Accepted Answer · 2016-05-15T20:37:56.577

1

The inner lock is not actually necessary because as long as the Wait is not reached again the lock will be held and will block all producers. Therefore this should work:

private void ThreadFunc() {
   do {
        if( !stop ) {
            WorkDelegate workItem = null;
            lock( workLock ) {
                if( Monitor.Wait(workLock, WaitTimeout) ) {
                    workItem = (WorkDelegate) workQueue.Dequeue();
                }
            }
            if (workItem != null) workItem();
        }
    } while( !stop );
}

public void SubmitWorkItem( WorkDelegate item ) 
{
    lock( workLock ) {
        workQueue.Enqueue( item );

        Monitor.Pulse( workLock );
    }
}

Joseph Albahari's site is an awesome reference for threading scenarios. Though as this is a classic producer/consumer scenario I would recommend you use a BlockingCollection.

edited May 15 '16 at 20:37

answered May 15 '16 at 18:52

Slugart

4,535
24
32

This will just throw exception that queue is empty - that's all. – Evk May 15 '16 at 18:55
Unfortunately that didn't become any better - now when queue is empty you continuosly loop in a cycle doing nothing and wasting CPU resources :) In sample code Monitor.Wait and Monitor.Pulse are used for signalling. – Evk May 15 '16 at 19:05
@Evk thanks for making me stop and think through the question. – Slugart May 15 '16 at 19:47
@Slugart As you said my conclusion was that queue lock **is not necessary**. – Yarl May 16 '16 at 06:59
As @Evk said, the code seems pretty broken from the start. If the question was how many lock objects are needed to implement a producer consumer queue then the answer would be one. – Slugart May 16 '16 at 07:35

Why is queue lock in this example necessary

1 Answers1