boost scoped_lock. Will this lock?

Question

solved

I changed the bfs::directory_iterator Queue to a std::string queue, and surprisingly solved the problem.

---

Hi, I have a gut feeling that i'm doing things wrong.

I've implemented (or attempted to) the thread pool pattern.

N threads read from a Queue, but i'm having some trouble. Here's what i got:

//inside a while loop
bool isEmpty;
bfs::directory_iterator elem;

{   
    boost::mutex::scoped_lock lock(this->queue_mutex);
    isEmpty = this->input_queue.isEmpty();

    if (!isEmpty){

     elem= *(this->input_queue.pop());
    }   
    else{
     continue;
    }   
}

Will the scoped_lock still work inside the if's body? Im starting to believe that it wont (after running many tests). If not, is there any scoped way to do this (i.e not the explicit lock unlock way)

Thanks in advance.

update

The code that adds elements to the queue looks like this

  //launches the above code, passing a reference to mutex and queue.
   Threads threads(queue,queue_mutex);

    for (bfs::directory_iterator dir_it:every file in directory){
      boost::mutex::scoped_lock lock(queue_mutex);

      queue.push(dir_it);


    }

Im placing a cout to control the poped filename, and if I push 2 files (file1) and (file2), and use 2 threads, I get boths "file2".

  class Threads{

   boost::thread::thread_group group; 
    Thread (N){

    //also asigns a reference to a queue and a mutex.
     for(i 1..N){ 
       //loop is posted above.
       group.add(new boost::thread(boost::bind(&loop,this)));
     }
    }
 };

Answer 1

No, the lock won't work if it's moved inside the if, because there would be a race condition on the check against emptiness. The last element may have been removed between checking and locking.

Answer 2

The code as posted appears fine - if you're seeing problems maybe there's some other place where the lock should be taken and isn't (such as the code that adds something to the queue).

Answer 3

I would add the lock to the queue... managing locks external to the queue is tricky, tends to confuse the code but also is quite fragile since the unlocked data structure is exposed.

The pattern that appears to be garnering utility for queues is a try_pop & try_push method. The Parallel Extensions to .NET is using this pattern with System.Collections.Concurrent.ConcurrentQueue.

This is done with either a lock-free queue or by simply embedding the queue and locks in a container with appropriate interfaces. Anthony Williams has a good post on how to do this with a std::queue here.

your code can look like this:

//inside a while loop
bfs::directory_iterator elem;

while (this->input_queue.pop(*elem))
{
  ... // do something
}