boost::shared_ptr and multithreaded access

Question

Hi!

I'm trying to implement a multithreaded framework, in which output objects are created at the end of every frame that my networking thread runs, so that another thread can, at the beginning of its frame, obtain the most recent "completed output" pointer and know that it has safe and complete read-only access to any data stored within the output object.

My (very early) approach to this primarily involves the following blocks of code:

NetworkHandler -

void NetworkHandler::Tick()
 {
  // Tick on our io_service
  m_ios.poll();

  // Assemble new output data
  this->AssembleOutput();
 }

ClientNetworkHandler -

void ClientNetworkHandler::AssembleOutput()
 {
  std::tr1::shared_ptr<crusade::task::TaskOutput> newOutput(new crusade::task::TaskOutput());
  newOutput->m_outputElements["connected"] = std::tr1::shared_ptr<crusade::task::TaskOutputElement>(new NetworkConnectedTaskOutputElement(this->m_isConnected));
  this->m_latestOutput.swap(newOutput);
 }

PyCruHandler -

void PyCruHandler::Tick()
 {
  printf("PyCruHandler\n");
  // Get any necessary inputs from other threads
  m_latestNetworkOutput.swap(crusade::task::THManager::GetInstance()->GetTaskHandler(crusade::task::THManager::TH_NETWORK)->GetLatestOutput());
  // Other unrelated processing to go here
 }

Basically, ClientNetworkHandler and PyCruHandler run independently on separate threads. PyCruHandler never actually does anything with its copy of m_latestNetworkOutput; I've commented out every other instance where it is accessed in any way, and I still have the following issue:

If I allow both threads to call swap (or the operator= equivalent), then eventually (usually within 2 seconds of running, but sometimes it takes a few minutes) I will get the following error on either an operator new or allocator deletion of some sort:

"HEAP: Free Heap block 2bab3b0 modified at 2bab3dc after it was freed Windows has triggered a breakpoint.

This may be due to a corruption of the heap, which indicates a bug... etc."

I am only a newbie, but to me this seems to indicate some sort of issue with thread-safety and timing-sensitive access issues between the shared_ptr objects. However, I have been embarassingly confused by the explanations (both here and elsewhere) of the nuances of shared_ptr thread-safety - one piece of reading indicates that the reference count is thread safe, so it's safe to copy shared_ptrs around (but their inner objects won't be thread safe), other pieces of reading indicate that there is effectively NO thread-safety of any usefulness in shared_ptr. I've read the boost documentation on thread safety of shared_ptrs, but that still didn't clarify in my mind whether this should be an issue in my code or not.

My question is, can anyone here detect any obvious flaw in what I'm doing? My goal is that I can access the latest output object still stored by the owning thread, and then that object won't be deleted (even after the owning thread has moved on to later outputs) until every user of the output is done with it as well. Shared pointer seemed perfect for this in my mind... but after 3 hours of banging my head against this I'm starting to wonder...

Thank you very much in advance, and I apologize if I posted this incorrectly somehow; it's my first time here and the FAQ seemed pretty laid-back as far as protocol goes!

Answer 1

Probably the best resource here is the documentation:

Thread Safety

shared_ptr objects offer the same level of thread safety as built-in types. A shared_ptr instance can be "read" (accessed using only const operations) simultaneously by multiple threads. Different shared_ptr instances can be "written to" (accessed using mutable operations such as operator= or reset) simultaneosly by multiple threads (even when these instances are copies, and share the same reference count underneath.)

Examples:

shared_ptr<int> p(new int(42));

//--- Example 1 ---

// thread A
shared_ptr<int> p2(p); // reads p

// thread B
shared_ptr<int> p3(p); // OK, multiple reads are safe

//--- Example 2 ---

// thread A
p.reset(new int(1912)); // writes p

// thread B
p2.reset(); // OK, writes p2

//--- Example 3 ---

// thread A
p = p3; // reads p3, writes p

// thread B
p3.reset(); // writes p3; undefined, simultaneous read/write

//--- Example 4 ---

// thread A
p3 = p2; // reads p2, writes p3

// thread B
// p2 goes out of scope: undefined, the destructor is considered a "write access"

//--- Example 5 ---

// thread A
p3.reset(new int(1));

// thread B
p3.reset(new int(2)); // undefined, multiple writes

I'm going to assume that m_latestOutput in your code is a shared_ptr - in that case, the example you are hitting most closely resembles number 5 (multiple writes).

Answer 2

According to documentation you must syncronize on simultaneous read and write. Swap is both :). Consider using operator=() instead, that would be only write.

Also, you client threads must make a copy of m_latestNetworkOutput (that would be read) if they want to keep object alive untill they are done (I think that's what you are doing in PyCruHandler::Tick()).

Regardless, you will have to syncronize write:

this->m_latestOutput.swap(newOutput);

And read:

m_latestNetworkOutput.swap(crusade::task::THManager::GetInstance()->GetTaskHandler(crusade::task::THManager::TH_NETWORK)->GetLatestOutput());

And change swap to assigment -- you don't need old pointer after swap, do you?