facebook / folly

An open-source C++ library developed and used at Facebook.
https://groups.google.com/forum/?fromgroups#!forum/facebook-folly
Apache License 2.0
28.43k stars 5.57k forks source link

Data race in folly::ConcurentHashMap. #1280

Open Milerius opened 4 years ago

Milerius commented 4 years ago

https://github.com/facebook/folly/blob/f6ac9ac15bce487b8a87b2a9e06949eb55074e99/folly/concurrency/detail/ConcurrentHashMap-detail.h#L247

Hello the following use case will data-race:

// Thread 1:

void awesome_function() {
  if (my_map.empty()) { //! call size() internally;

  }
}

//! Thread 2:

void another_awesome_function() {
  //! Clear internally lock size_ with a mutex data race occur in the thread 1 because size_ is not locked
  my_map.clear(); //!
}

Potential solution (size() function can lock size_):

 size_t size() {
    std::lock_guard<Mutex> g(m_);
    return size_;
  }

Where size_ is locked ?

void clear(hazptr_obj_batch<Atom>* batch) {
    size_t bcount = bucket_count_.load(std::memory_order_relaxed);
    Buckets* buckets;
    auto newbuckets = Buckets::create(bcount, batch);
    {
      std::lock_guard<Mutex> g(m_); //< here size_ is locked
      buckets = buckets_.load(std::memory_order_relaxed);
      buckets_.store(newbuckets, std::memory_order_release);
      size_ = 0;
    }
    buckets->retire(concurrenthashmap::HazptrTableDeleter(bcount));
  }

Another solution can be to have an std::atomic and returning size_.load(); when necessary.

magedm commented 4 years ago

Thank you for reporting this and the fix suggestions. We'll fix shortly.