2
\$\begingroup\$

I need a class that holds a huge amount of data, which should be useable across multiple threads. To avoid copying the data every time, I used a std::shared_ptr. To make it thread-safe I need a mutex to secure the data. But if I'm right, this mutex must be shared too. Since boost::recursive_mutex (and other mutexes I guess) are non-copyable, I put it into a std::shared_ptr too, to make sure the same mutex is used in all threads sharing the same data.

Questions:

  1. Can I use a mutex in a shared_ptr? The implementation is shown below and it works, but I'm unsure, since I didn't find a definite answer elsewhere.

  2. Are the copy-constructor, the assignment-operator and the swap-method correctly (thread-safe) implemented?

SharedData.h:

#ifndef SHAREDDATA_H
#define SHAREDDATA_H

#include <boost/thread/recursive_mutex.hpp>
#include <memory>

class SharedData
{
private:
  std::shared_ptr<float> m_pData;
  std::shared_ptr<boost::recursive_mutex> m_pMutex;

public:
  SharedData(void)
  {
    m_pMutex = std::make_shared<boost::recursive_mutex>();
    m_pData = std::make_shared<float>(1.23456789);
  }

  SharedData(SharedData const &_copyFrom)
  {
    m_pMutex = _copyFrom.m_pMutex;
    boost::recursive_mutex::scoped_lock otherLock(*_copyFrom.m_pMutex);
    m_pData = _copyFrom.m_pData;
  }

  SharedData const & operator=(SharedData const &_copyFrom)
  {
    if (this != &_copyFrom)
    {
      boost::recursive_mutex::scoped_lock myLock(*m_pMutex, boost::defer_lock);
      boost::recursive_mutex::scoped_lock otherLock(*_copyFrom.m_pMutex, boost::defer_lock);
      boost::lock(myLock, otherLock);

      SharedData tmp(_copyFrom); //Copy-and-swap
      this->swap(tmp);
    }

    return *this;
  }

  ~SharedData(void)
  {
  }

  void swap(SharedData &_other)
  {
    boost::recursive_mutex::scoped_lock myLock(*m_pMutex, boost::defer_lock);
    boost::recursive_mutex::scoped_lock otherLock(*_other.m_pMutex, boost::defer_lock);
    boost::lock(myLock, otherLock);

    std::swap(m_pData, _other.m_pData);
    std::swap(m_pMutex, _other.m_pMutex);
  }

  //If data is going to be modified and thus needs to be copied
  operator float *()
  {
    boost::recursive_mutex::scoped_lock lock(*m_pMutex);

    if (m_pData.unique() == false)
    {
      SharedData data;
      *this = data;
    }

    return m_pData.get();
  }

  //The data is not modified
  operator float const *() const
  {
    boost::recursive_mutex::scoped_lock lock(*m_pMutex);

    return m_pData.get();
  }

  void reset()
  {
    boost::recursive_mutex::scoped_lock lock(*m_pMutex);
    m_pData.reset();
  }
};


#endif /*SHAREDDATA_H*/

main.cpp :

#include <boost/thread.hpp>

boost::mutex g_mutexCout;

void dontmodify(SharedData const data)
{
  g_mutexCout.lock();
  //const-operator is called
  std::cout << "Thread id : " << boost::this_thread::get_id() << " ptr = " << data << " value = " << *data << std::endl;
  g_mutexCout.unlock();
}

void modify(SharedData data)
{
  g_mutexCout.lock();
  //const-operator is not called
  std::cout << "Thread id : " << boost::this_thread::get_id() << " ptr = " << static_cast<float const *>(data) << " value = " << *static_cast<float const *>(data) << std::endl;
  g_mutexCout.unlock();
}

int main(int argc, char* argv[])
{
  SharedData data;
  boost::thread t1(dontmodify, data);
  boost::thread t2(modify, data);

  t1.join();
  t2.join();

  return 0;
}

The above code of SharedData is wrong. I let it run with the Application Verifier active, and it crashed in the assignment-operator, since (I think), the mutexes are swapped and the application broke when the assign-block is leaved.

Have a look here and here.

\$\endgroup\$
  • \$\begingroup\$ I don't quite understand why you wrote it that way. Couldn't it be as simple as struct Data { float value; boost::recursive_mutex mutex; }; and then using a std::shared_ptr<Data>? What is the use you have in mind for your class? \$\endgroup\$ – 5gon12eder May 19 '15 at 17:33
  • \$\begingroup\$ The intention was, to make it easy for my team members to work with that class, without messing around with the locks. Your idea is good, but everyone that wants to use the value must first lock the mutex manually. \$\endgroup\$ – AquilaRapax May 20 '15 at 6:30
  • \$\begingroup\$ I asked because your modify and dontmodify client (?) code is also dealing with locks so I'm not sure what the gain is. I could think of a few patterns that could help you but that would be out of scope of a code review because they would have little to nothing to do with your original code. If you are interested in suggestions for different solutions, maybe your question might be a better fit on Stackoverflow or Programmers? \$\endgroup\$ – 5gon12eder May 20 '15 at 17:53
  • \$\begingroup\$ Hehe, i originally posted this question on Stackoverflow, but i was told to move it to codereview ;). The code i presented here, was just a sample and test. the modify and dontmodify methods just lock a mutex to avoid messing up the output. They are just there for testing, which of the access-methods is used. \$\endgroup\$ – AquilaRapax May 22 '15 at 9:01
  • \$\begingroup\$ So, are you after a review of your code or a suggestion for other code? \$\endgroup\$ – 5gon12eder May 22 '15 at 18:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.