On Mon, Jan 18, 2010 at 8:00 AM, girish hilage
The code I have now written is as follows (which seems to work fine) :
This code is broken. I find it hard to read and understand, and it contains 4 different mutexes, which is asking for trouble. But besides those 'aesthetic' complaints, it really is broken. (IIUC) It appears that you are trying very hard to make sure someone is waiting on the condition before notifying. I was tempted to say this can't be done, but you have done it nonetheless. Although I am not sure it helps. Please research 'spurious wakeups' of condition variables:
void mywait (boost::condition_variable &cond, boost::mutex &mut, boost::mutex &flag_mut, bool &flag) { boost::unique_lockboost::mutex lock(mut);
enable_flag (flag_mut, flag);
cond.wait (lock);
This cond.wait(lock) may wake up even though cond.notify_one() was NOT called. This may seem strange, but it is allowed because otherwise implementing conditions becomes very hard. And by allowing it, it encourages developers to code with conditions properly. To use a condition you MUST do so in a loop which checks the global *state* that the condition is communicating. ie in your case 'new job available'. The standard could have stored state inside the condition variable, but it was harder, and it was also assumed that you can determine the state better on your own, so it is outside the condvar. It also makes it easier for multiple threads to agree - they can all look at the external state (available jobs) and decide if something needs to be done, or if another thread has already done it. Another way to look at things: spurious wakeups mean that, in effect, notify_one might wake up more than one thread (they still battle for the mutex of course), but notify_one is essentially (worse case) the same as notify_all - ie all the other threads could 'spuriously' wake up. ie notify_one could be implemented by just calling notify_all (it would be a poor implementation, but valid). So notify_one is just an optimization (*probably* only one thread will awaken), not a guarantee. So what happens in your code if cond.wait() wakens spuriously? It appears a job is 'done' (or attempted) even though no job exists. If you can tell (from the worker thread) that there is no job to do, then make THAT your external state, and wrap the cond.wait() inside a loop: }
void worker_function (void) {
for (;;) {
while ( ! /* there is work to be done */) // note the '!' at the
front cond.wait(mutex); ..... read new job .....
// probably need to hold lock while reading job (from some shared queue or whatever) // then unlock so you can do the work *in parallel* with other threads mutex.unlock(); // unlock before doing work
..... do the work .....
mutex.lock(); // lock again before checking /* there is work to be done */, unlocked inside cond.wait()
} }
int main (void) { ..... create worker threads .....
while (/*there is work to be done */) {
cond.notify_all(); // there may be more than one job to do, so notify all
/* wait for job_done notification from any of the busy thread */ mywait (job_done_condition, job_done_mutex, job_done_notification_reached_flag_mutex, job_done_notification_reached_flag);
Why do you wait for the job to be done? If you send a job to another thread, then do nothing but wait for it to be done, why not just do it yourself? ie it appears that your code will run like this: Thread 1 Thread 2 wait: check for job ..... notify wake wait for done: do job ..... keep doing that job ..... keep doing that job ..... job done! wake! wait for new job (ie repeat) do the 2 threads ever do any real work at the same time, or is one always waiting for the other?
} }
I hope this will not create any problems. (I have also edited the problem statement in the mail below to make it more clear. Shown by __TEXT_EDITED__.)
Regards, Girish
I honestly had (still have?) trouble understanding the code, but I don't think it works, and I think it could be much simpler. Basically, just use the condvars to tell threads 'hey *CHECK* if there is something to do'. Do NOT try to use condvars to actually communicate state. Just 'hey check the state'. Then let the threads decide what to do based on that state. condvars also nicely handle the lock/unlock/relock that you probably needed to do around the check and wait anyhow. So the primary purpose of the lock is to protect your state, not for the condvar. The condvar is just nice enough to know how to work with that. In your case your state is the state of the job list. Put a mutex around that, and have the condvar use that mutex as well. Check the state of the job list to decide what (if anything) to do. There is only one state (job list) and one mutex and one condvar. Tony Tony