Is this atomic pointer swap pattern safe?

user_7153906 0
Viewed 279

The goal of the code/pattern below is to achieve high read performance. The read happens with very high frequency, and the write (update) is very infrequent. So I naturally would avoid using an exclusive lock like a mutex in both read/update. I also don't want to use a reader/writer lock approach because the high frequency of read operations will likely starve the update thread.

So, the pattern I propose is to use atomic raw pointers and swap. The read operation will just dereference the atomic pointer. The update operation will construct the new data on the heap and have an atomic pointer that points to it. Then swap them.

The swap doesn't have to be atomic. I just need to avoid a data race that causes pointer data corruption and crashes. So there is a time window where a read thread will already load the old data pointer and about to dereference it. I need to ensure I don't delete the old data pointer until all lingering read threads that have a reference to old data pointers are gone (read is non-blocking). That is why I sleep a bit after the swap. Then I delete the old data pointer.

Is there any hole in the implementation for thread safety? I couldn't use atomic<shared_ptr> because that is only in C++20, I think. So this is like a poor man's job for what couldn't been done with atomic<shared_ptr>. There is thread synchronization because of atomic, but still not the performance impact of an exclusive lock or a reader/writer lock.

std::atomic<vector<string>*> data;

// Read thread (happen with extremely high frequency):
string& Read() {
  return data->load()->at(1);
}

// Update thread (happen infrequently):
void Update() {
  std::atomic<vector<string>*> newData = new std::vector<string>();
  // insert new data to newData ...
  // ....
 
  // swap pointers (swap as whole doesn't need to be atomic, but set pointers should be atomic)
  newData.store(data.exchange(newData));

  // sleep a bit so any read thread that already has gotten the old data pointer can
  // still get the old value before we delete the old data pointer
  std::this_thread::sleep_for(100ms);

  // delete the old data (After swap, newData points to old data)
  delete newData.load();
}
c++17 atomic thread-safety c++
1 Answers

The code you have written is not "safe" by any good definition of that term. When it comes to writing good threading code, it shouldn't be about the "probability" that something will go wrong. You should assume that it will go wrong and use tools to make sure that it cannot go wrong.

This situation is what the atomic shared_ptr accessor functions are made for. They've been around since C++11 (though they're deprecated in C++20, since atomic<shared_ptr> exists).

std::shared_ptr<vector<string>> data;

// Read thread (happen with extremely high frequency):
string& Read() {
  auto local_data = atomic_load(&data);
  //If `data` gets changed now, the destructor of `local_data` will take care of it.
  return local_data->at(1);
}

// Update thread (happen infrequently):
void Update() {
  auto newData = std::make_shared<std::vector<string>>();
  // insert new data to newData ...
  // ....
 
  // swap pointers (swap as whole doesn't need to be atomic, but set pointers should be atomic)
  newData = atomic_exchange(&data, newData);

  //Destructor takes care of deleting the old.
}

The primary caveat of these functions is that they exhibit UB if data is ever null when you call one of them. So... don't do that. This means you cannot call Update until data has been given memory to manage.

So you'll need to initialize data with something outside of calls to Update.

The design of this API is such that, when the time comes to switch to C++20's atomic<shared_ptr>, all you need to do is change the type used for data.

user_7340690
Related