fix race between StaticMetaBase::destroy() and StaticMetaBase::onThreadExit()

Summary:
We would like to guarantee that after `folly::ThreadLocal<>` dtor
returns no per-thread instances are longer alive.  Currently this is not a case:

* T1 is excuting `StaticMetaBase::onThreadExit()`, it acquired all per-thread
  instances and erased them from meta under `accessAllThreadsLock_`.
* T2 destroys `folly::ThreadLocal<>`, it executes `StaticMetaBase::destroy()`,
  collects all per-thread instances (thus missing the ones being destroyed by
  T1), destroys them and returns.
* T1 executes dtor of per-thread instances, after parent `folly::ThreadLocal<>`
  dtor is finished.

Reviewed By: ot

Differential Revision: D4109820

fbshipit-source-id: d547b8cc77c9871126538c38644c2e98ddccf220

folly/ThreadCachedInt.h

@@ -47,7 +47,7 @@ class ThreadCachedInt : boost::noncopyable {
 
   void increment(IntT inc) {
     auto cache = cache_.get();
-    if (UNLIKELY(cache == nullptr || cache->parent_ == nullptr)) {
+    if (UNLIKELY(cache == nullptr)) {
       cache = new IntCache(*this);
       cache_.reset(cache);
     }
@@ -122,15 +122,6 @@ class ThreadCachedInt : boost::noncopyable {
     target_.store(newVal, std::memory_order_release);
   }
 
-  // This is a little tricky - it's possible that our IntCaches are still alive
-  // in another thread and will get destroyed after this destructor runs, so we
-  // need to make sure we signal that this parent is dead.
-  ~ThreadCachedInt() {
-    for (auto& cache : cache_.accessAllThreads()) {
-      cache.parent_ = nullptr;
-    }
-  }
-
  private:
   std::atomic<IntT> target_;
   std::atomic<uint32_t> cacheSize_;
@@ -173,10 +164,8 @@ class ThreadCachedInt : boost::noncopyable {
     }
 
     ~IntCache() {
-      if (parent_) {
       flush();
     }
-    }
   };
 };
 

folly/detail/ThreadLocalDetail.cpp

@@ -98,8 +98,23 @@ uint32_t StaticMetaBase::allocate(EntryID* ent) {
 void StaticMetaBase::destroy(EntryID* ent) {
   try {
     auto& meta = *this;
+
     // Elements in other threads that use this id.
     std::vector<ElementWrapper> elements;
+
+    {
+      SharedMutex::WriteHolder wlock;
+      if (meta.strict_) {
+        /*
+         * In strict mode, the logic guarantees per-thread instances are
+         * destroyed by the moment ThreadLocal<> dtor returns.
+         * In order to achieve that, we should wait until concurrent
+         * onThreadExit() calls (that might acquire ownership over per-thread
+         * instances in order to destroy them) are finished.
+         */
+        wlock = SharedMutex::WriteHolder(meta.accessAllThreadsLock_);
+      }
+
       {
         std::lock_guard<std::mutex> g(meta.lock_);
         uint32_t id = ent->value.exchange(kEntryIDInvalid);
@@ -129,7 +144,8 @@ void StaticMetaBase::destroy(EntryID* ent) {
         }
         meta.freeIds_.push_back(id);
       }
-    // Delete elements outside the lock
+    }
+    // Delete elements outside the locks.
     for (ElementWrapper& elem : elements) {
       elem.dispose(TLPDestructionMode::ALL_THREADS);
     }