synchronizations of threads

[puzzlecracker]

The following example of rudimentary thread pool is 'bluntly' copied
from "Accelerate d C#" book. My questions follow the code:

using System;
using System.Threadin g;
using System.Collecti ons;
public class CrudeThreadPool
{
static readonly int MAX_WORK_THREAD S = 4;
static readonly int WAIT_TIMEOUT = 2000;
public delegate void WorkDelegate();

public CrudeThreadPool () {
stop = 0;
workLock = new Object();
workQueue = new Queue();
threads = new Thread[ MAX_WORK_THREAD S ];
for( int i = 0; i < MAX_WORK_THREAD S; ++i ) {
threads[i] =new Thread( new
ThreadStart(thi s.ThreadFunc) );
threads[i].Start();
}
}
private void ThreadFunc() {
lock( workLock ) {
int shouldStop = 0;
do {
shouldStop = Interlocked.Exc hange( ref stop,stop );
if( shouldStop == 0 )
{
WorkDelegate workItem = null;
if( Monitor.Wait(wo rkLock, WAIT_TIMEOUT) ) {
// Process the item on the front of the
queue
lock( workQueue ) {
workItem =(WorkDelegate)
workQueue.Deque ue();
}
workItem();
}
}
} while( shouldStop == 0 );
}
}
public void SubmitWorkItem( WorkDelegate item ) {
lock( workLock ) {
lock( workQueue ) {
workQueue.Enque ue( item );
}
Monitor.Pulse( workLock );
}
}
public void Shutdown() {
Interlocked.Exc hange( ref stop, 1 );
}
private Queue workQueue;
private Object workLock;
private Thread[] threads;
private int stop;
}
public class EntryPoint
{
static void WorkFunction() {
Console.WriteLi ne( "WorkFuncti on() called on Thread {0}",
Thread.CurrentT hread.GetHashCo de() );
}
static void Main() {
CrudeThreadPool pool = new CrudeThreadPool ();
for( int i = 0; i < 10; ++i ) {
pool.SubmitWork Item(new
CrudeThreadPool .WorkDelegate(E ntryPoint.WorkF unction) );
}
pool.Shutdown() ;
}
}

SO my question is why do we need need workLock, can we just use
workQueue to synchronize the access to the queue and manage the
execution of threads in the pool?


Thanks

[Peter Duniho]

Re: synchronization s of threads

On Sun, 19 Oct 2008 14:48:35 -0700, puzzlecracker <ironsel2000@gm ail.com>
wrote:

[...]
SO my question is why do we need need workLock, can we just use
workQueue to synchronize the access to the queue and manage the
execution of threads in the pool?

Is that really the code directly from the book, verbatim? What edition of
the book are you looking at?

To answer your immediate question, it's not the lock using workLock that's
superfluous, it's the lock using workQueue. In fact, IMHO the lock using
workQueue shouldn't be used at all. As has been discussed elsewhere,
including in Jon Skeet's web article on threading, using a privately
allocated instance of Object for a lock is preferable to using the object
that actually needs the synchronized access, because doing so ensures that
only that particular code is locking on that particular object.

At the very least, using a "public" reference (that is, a reference that
some other code might see, whether the variable itself is public or not)
runs the risk of increased contention, and it even slightly increases the
risk of having a deadlock bug.

In this particular context, I see no value at all in the inner lock. The
code is already synchronized with the other code in the example that
touches the object and that's sufficient.

Now, that said, that's not the only, or even most serious problem I see
with the code. I am surprised to see the non-generic Queue class being
used, but more significantly the use of the "stop" variable to signal a
shutdown rather than just communicating through the queue, which leads to
the two-second timeout, another questionable design choice, and the fact
that the workLock lock is held during the entire execution of the worker
delegate (imagine if the .NET ThreadPool class blocked any thread trying
to queue a work item until _all_ currently queued and executing tasks were
done), these are all bad problems IMHO.

In other words, I'm not sure I'd use this kind of code as a way to learn
about how best to implement threaded code. It has the scent of being
written by someone who knows just enough to be dangerous.

Interestingly, Jon reviewed this book and while he did have some
observations about the threading examples, he didn't have any particular
comments about that example. I don't know whether that's because Jon
thought the queue example was fine, or he just thought his point about the
weakness in Nash's presentation on threading was made well enough with the
example he did comment on, but if the former that might mean I've
overlooked something that justifies the implementation is it is. I don't
think I did, but you never know. :)

Pete

[puzzlecracker]

Re: synchronization s of threads

Is that really the code directly from the book, verbatim?  What editionof  
the book are you looking at?

Yes, copied exactly as stated written in the book (page 343)

To answer your immediate question, it's not the lock using workLock that's  
superfluous, it's the lock using workQueue.  In fact, IMHO the lock using  
workQueue shouldn't be used at all.  As has been discussed elsewhere,  
including in Jon Skeet's web article on threading, using a privately  
allocated instance of Object for a lock is preferable to using the object 
that actually needs the synchronized access, because doing so ensures that  
only that particular code is locking on that particular object.

That is a different issue, but I agree that a separate object (aka
lock) instance should be used for synchronization .