0

I seem to be having an issue synchronizing a data collection. In this particular class, I have a data collection, mutex, and condition variable that looks something like:

map<std::string, double> collection;
boost::mutex collectionMutex;
boost::condition_variable collectionConditional;

For the most part, this works. However, recently I have added a function where I essentially assign a new value for each value in the collection. There's about 100-200 values in the collection, so not very many; all of the assignments happen very quickly. I've also made sure that no calculations occur during this step, it is merely a series of assignments. Surrounding the assignments, I have code that looks like this(based on an answer here on stackoverflow):

boost::mutex::scoped_lock lock(collectionMutex);
while(!lock.owns_lock())
{
    collectionConditional.wait(lock);
}
// Assignments here
collectionConditional.notify_one();

Elsewhere in the code, I have a function that 'reads' the information from the collection, and that looks something like this:

double result = 0;
boost::mutex::scoped_lock lock(collectionMutex);
while(!lock.owns_lock())
{
    collectionConditional.wait(lock);
}

result = collection["Some String"];

collectionConditional.notify_one();

What's happening is that when my 'write' function is called, it seems to deadlock for a while. It breaks out of it eventually so it's not a complete deadlock, but the waiting period can be a long time(several seconds). This should be something that's on the order of milliseconds or less.

The odd part is that I've successfully used the above writer function before and had multiple threads writing to this data collection without a problem. The reason I've made this change is a move to centralize the updates to this collection to one place, rather than having it be dependent on the run-time state of other threads.

Shaz
  • 1,376
  • 8
  • 18
  • 2
    `operator[]` is not const. You can only call it with the write lock. Even when the key exists it could be changing internals. I've seen real implementations where this happens too. – Flexo Jun 11 '13 at 20:54
  • `owns()` is a postcondition of the `boost::mutex::scoped_lock(mutex&)` constructor ([documentation](http://www.boost.org/doc/libs/1_53_0/doc/html/boost/interprocess/scoped_lock.html)), so your `while` loops will never run. What are you trying to achieve with the condition variable? – Casey Jun 11 '13 at 21:02
  • 2
    Maybe [`boost::shared_mutex`](http://www.boost.org/doc/libs/1_41_0/doc/html/thread/synchronization.html#thread.synchronization.mutex_types.shared_mutex) is better suited for your use case. – πάντα ῥεῖ Jun 11 '13 at 21:44
  • 1
    Upon further consideration, holding the mutex is a precondition of `boot::condition_variable::wait`, so it's a very good thing that the while loops never run: they have undefined behavior. – Casey Jun 11 '13 at 21:57
  • Yes use collection.find() rather than operator[]. Put the read code in a const member function and make the mutex `mutable` to avoid such errors. This doesn't answer your question though. – Pete Jun 11 '13 at 22:35
  • This may be worth a read: http://doc.qt.digia.com/qq/qq11-mutex.html – Pete Jun 11 '13 at 22:36
  • 1
    And shared_mutex as suggested is probably your solution. – Pete Jun 11 '13 at 22:39
  • Thanks @Pete. I removed the operator[] call from my read function and made it const. I also got rid of the condition_variable and followed the shared_mutex example [in this post](http://stackoverflow.com/questions/989795/example-for-boost-shared-mutex-multiple-reads-one-write). It runs smoothly now. – Shaz Jun 12 '13 at 16:12
  • Thanks @g-mukalik I ended up using a shared_mutex – Shaz Jun 12 '13 at 16:12
  • Thanks @Casey I got rid of the condition_variable(I'm not entirely sure why it was in there). – Shaz Jun 12 '13 at 16:13
  • Thanks @Flexo I got rid of the operator[] in the read function and used .find() instead. – Shaz Jun 12 '13 at 16:13
  • You could writeup an answer that merges all the knowledge from the comments to show how you solved it. – Flexo Jun 12 '13 at 16:25

1 Answers1

2

Thanks to the comments on my question, I ended up doing several things:

  1. Stopped using operator[] in the read function and made the read function const.

  2. Stopped using the condition_variable

  3. Used shared_mutex based on the example in this other post.

The code:

map<std::string, double> collection;
mutable boost::shared_mutex collectionMutex;

...

//Write function:
void mapData()
{
    // get upgradable access
    boost::upgrade_lock<boost::shared_mutex> lock(collectionMutex);

    // get exclusive access
    boost::upgrade_to_unique_lock<boost::shared_mutex> uniqueLock(lock);

    // Assignments here.  Eg:
    // collection["Some String"] = 0.0;
}

// Read function:
double readData(std::string name) const
{
    double result = 0;
    boost::shared_lock<boost::shared_mutex> lock(collectionMutex);
    map<std::string, double>::const_iterator it = collection.find(name);
    if(it != data.end())
    {
        result = it->second;
    }
    return result;
}
Community
  • 1
  • 1
Shaz
  • 1,376
  • 8
  • 18