How to properly move/read from a shared_ptr

Question

I have a caching system in one of my programs. I have a single, static class that maintains this cache, and use the cache in multiple threads concurrently. I am running into a problem maintaining the caching system correctly. Here is some sample code.

class db_cache
{
    public:
        typdef std::map<int, int> map_t;

        static void update_cache();
        static boost::shared_ptr< const map_t > get_cache();

    private:
        db_cache();
        static void run_update();
        static boost::shared_ptr< const map_t > cur_cache_;
        static boost::shared_ptr< const map_t > old_cache_;
 };

 void db_cache::update_cache()
 {
     cur_cache_ = boost::make_shared< map_t >();
     old_cache_ = boost::make_sahred< map_t >();

     //   
     //Setup connection to server that sends updates
     //

     //Initialize cache
     run_update();
     while(true)
     {
         if(recv().compare("update") == 0)
         {
              //Update cache if update message recieved
              run_update();
         }
     }
}

void db_cache::run_update()
{
    //Create new cache to swap with current cache
    auto new_cache = boost:make_shared<map_t>();

    //
    //Put data in new cache
    //

    boost::atomic_store(&old_cache_, boost::move(cur_cache_));
    boost::atomic_store(&cur_cache_, boost::shared_ptr< const map_t >(boost::move(new_cache)));
}

auto db_cache::get_cache() -> boost::shared_ptr< const map_t >
{
    return boost::atomic_load(&cur_cache_);
}

I am currently getting a crash at boost::atomic_store(&old_cache_, boost::move(cur_cache_));. The crash seems to be because old_cache_ is null. This seems to happen on the second time an update message is received. I am assuming what is happening(not 100% sure, but the only one way I can think), is:

1. The first time the message is received, cur_cache_ is copied to old_cache_.
2. cur_cache_ is replaced with new_cache, causing the old cur_cache_(what old_cache_ is currently pointing too) to be null.
3. old_cache_ causes crash when boost::atomic_store is called again due to it being null.

My question is, why does the boost::atomic_store(&old_cache_, boost::move(cur_cache_)); not cause the reference counter of cur_cache_ to increase. Is there a way I can make this happen?

Other note:

The reason I have old_cache_ is because I believe I was having a problem when reading from the cache, which is most likely also a problem. My program seemed to be crashing when trying to read elements from the map returned from get_cache(), so to make sure they stayed in scope until all threads that currently had a copy were done, I save the last version of the cache. I suppose if I had the correct way to do this, I could get rid of the old_cache_ all together, which should solve the problem above.

Edit:

Here is the code using the cache:

//Vector big, don't want to copy
const std::vector *selected_vec = &(*db_cache::get_cache()).at(get_string);
for( std::vector<std::string>::iterator it = selected_vec->begin(), e = selected_vec->end(); it != e; ++it)
{
   //String can be big, don't want to copy
   const std::string *cur_string = &(*it);
   if(cur_string == nullptr || cur_string->size() == 0)
   {
       continue;
   }
   char a = cur_string->at(0);  //Crash here

   //Do Stuff
}

My actual map_t type is a std::map<std::string,std::vector<std::string>> type not a std::map<int,int>. After calling get_cache(), I get the vector I want, and iterator over the vector. For each string, I try and get the first char. When I try and get the char, the program crashes. The only thing I can think of is that selected_vec is deleted.

Answer 1

You have a data race in run_update. The line that sets old_cache_:

boost::atomic_store(&old_cache_, boost::move(cur_cache_));

is performing a non-atomic modification to cur_cache_. Recall the signature of atomic_store:

namespace boost {
template<class T>
void atomic_store( shared_ptr<T>* p, shared_ptr<T> r );
}

In passing the expression boost::move(cur_cache_) to that second parameter, your function creates the actual argument object by moving from cur_cache_ and leaving it set to nullptr. Even if this modification was atomic, there's a window between this line and the later line that sets cur_cache_ in which clients will see the nullptr returned from get_cache. If you absolutely want to preserve the value of cur_cache_ in old_cache_, you need to use atomic_exchange to simultaneously set cur_cache_ and retrieve the old value:

void db_cache::run_update()
{
    auto new_cache = boost:make_shared<map_t>();

    // ...

    auto old = boost::atomic_exchange(&cur_cache_, boost::move(new_cache));
    boost::atomic_store(&old_cache_, boost::move(old));
}

but it would appear that you have no use for old_cache_ once the race is fixed, and you could eliminate it altogether:

void db_cache::run_update()
{
    auto new_cache = boost:make_shared<map_t>();

    // ...

    boost::atomic_store(
        &cur_cache_,
        boost::shared_ptr<const map_t>(boost::move(new_cache))
    );
}

The source of your original issue is in this "client" code:

const std::vector *selected_vec = &(*db_cache::get_cache()).at(get_string);

You store a pointer into the internals of an object accessed through a shared_ptr, but don't retain a reference to the object denoted by that shared_ptr. When you later dereference that pointer in the loop, it's possible that its referent has been destroyed. You need to keep a copy of the shared_ptr around to ensure that the referent stays alive while you use it (and you may as well use references instead of pointers):

boost::shared_ptr<const map_t> cache = db_cache::get_cache();
//Vector big, don't want to copy
const std::vector &selected_vec = cache->at(get_string);
for( std::vector<std::string>::iterator it = selected_vec->begin(), e = selected_vec->end(); it != e; ++it)
{
   //String can be big, don't want to copy
   const std::string &cur_string = *it;
   if(cur_string.size() == 0)
   {
       continue;
   }
   char a = cur_string.at(0);  //Don't crash here

   //Do Stuff
}

Answer 2

I suppose I can put an answer for my crashing issue, although I still think there is a better way to design the solution. Basically, my three step list was someone correct, boost::atomic_store(&old_cache_, boost::move(cur_cache_)); was causing cur_cache_ to have 0 references, and the object cur_cache_ was pointing too was being freed. The next time through the function, old_cache_ was then pointing too to the freed pointer, and it was causing a crash. My solution was to change

boost::atomic_store(&old_cache_, boost::move(cur_cache_));

to

boost::atomic_store(&old_cache_, cur_cache_);

which stopped cur_cache_'s use_count increase to two, after the first call, and go back down to 1 after the second call.

I still believe that there is a better way to structure my code without having to keep the old_cache_, and would be happy to accept an answer of someone who can explain that.