HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Double checked locking with shared_ptr<>

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withshared_ptrdoublelockingchecked

Problem

I have an unordered_map> into which I want to lazily create the instances of foo based on callbacks. These callbacks would come from potentially a pool of threads, so to handle this, at the start I create the map and insert empty shared_ptr<>s into the map for all possible indexes. Now I have the code below which is designed to create the foo instance based on demand.

Is my implementation of double checked locking with shared_ptr<> correct?

void Publish(const bar& data)  {
    auto id = data.get_id();
    auto it = foos_.find(id);
    if (it == foos_.end()) {
      // throw
    }
    // Check if this is present
    auto sp = std::atomic_load_explicit(&(it->second), std::memory_order_relaxed);
    std::atomic_thread_fence(std::memory_order_acquire);
    if (!sp) {
      // Create the instance of the foo for this id
      std::lock_guard lock(mutex_);
      (void) lock;
      // Check again
      sp = std::atomic_load_explicit(&(it->second), std::memory_order_relaxed);
      if (!sp) {
        // Create new foo
        sp = std::make_shared(...);
        std::atomic_thread_fence(std::memory_order_release);
        std::atomic_store_explicit(&(it->second), sp, std::memory_order_relaxed);
      } // Someone else created it...
    }

    // Push the data in
    sp->Publish(data);
  }

Solution

Your implementation of Double Checked Locking is semantically correct; the acquire and release fences provide minimal necessary ordering.

I am wondering why you are using standalone fences, which is almost never necessary..
It is more common to use memory ordering directly on atomic operations:

auto sp = std::atomic_load_explicit(&(it->second), std::memory_order_relaxed);
std::atomic_thread_fence(std::memory_order_acquire);

...

std::atomic_thread_fence(std::memory_order_release);
std::atomic_store_explicit(&(it->second), sp, std::memory_order_relaxed);


Can be replaced with:

auto sp = std::atomic_load_explicit(&(it->second), std::memory_order_acquire);

...

std::atomic_store_explicit(&(it->second), sp, std::memory_order_release);


Furthermore, I understand why you are using (void) lock;, but I would not use that line just to suppress a compiler warning.
Also I am surprised about that warning since a modern compiler should know that RAII objects have a function. Are you sure there is a warning or is that an assumption ?

Code Snippets

auto sp = std::atomic_load_explicit(&(it->second), std::memory_order_relaxed);
std::atomic_thread_fence(std::memory_order_acquire);

...

std::atomic_thread_fence(std::memory_order_release);
std::atomic_store_explicit(&(it->second), sp, std::memory_order_relaxed);
auto sp = std::atomic_load_explicit(&(it->second), std::memory_order_acquire);

...

std::atomic_store_explicit(&(it->second), sp, std::memory_order_release);

Context

StackExchange Code Review Q#152847, answer score: 3

Revisions (0)

No revisions yet.