diff --git a/folly/ThreadLocal.h b/folly/ThreadLocal.h index 5b42f5579df..114b30f203d 100644 --- a/folly/ThreadLocal.h +++ b/folly/ThreadLocal.h @@ -196,7 +196,6 @@ class ThreadLocalPtr { typename = typename std::enable_if< std::is_convertible::value>::type> void reset(std::unique_ptr source) { - auto rlocked = getForkGuard(); auto deleter = [delegate = source.get_deleter()]( T* ptr, TLPDestructionMode) { delegate(ptr); }; reset(source.release(), deleter); @@ -230,6 +229,7 @@ class ThreadLocalPtr { } }); + auto rlocked = getForkGuard(); threadlocal_detail::ThreadEntry* te = StaticMeta::getThreadEntry(&id_); uint32_t id = id_.getOrInvalid(); // Only valid index into the the elements array @@ -427,9 +427,9 @@ class ThreadLocalPtr { lock_(&meta_.lock_) { forkHandlerLock_->lock_shared(); accessAllThreadsLock_->lock(); - lock_->lock(); id_ = id; wlockedThreadEntrySet_ = meta_.allId2ThreadEntrySets_[id_].wlock(); + lock_->lock(); } void release() { diff --git a/folly/detail/ThreadLocalDetail.cpp b/folly/detail/ThreadLocalDetail.cpp index 1efc641e6ae..87d4d264c84 100644 --- a/folly/detail/ThreadLocalDetail.cpp +++ b/folly/detail/ThreadLocalDetail.cpp @@ -114,16 +114,19 @@ void StaticMetaBase::onThreadExit(void* ptr) { // Make sure this ThreadEntry is available if ThreadLocal A is accessed in // ThreadLocal B destructor. pthread_setspecific(meta.pthreadKey_, threadEntry); + + std::shared_lock forkRlock(meta.forkHandlerLock_); std::shared_lock rlock(meta.accessAllThreadsLock_, std::defer_lock); if (meta.strict_) { rlock.lock(); } + meta.removeThreadEntryFromAllInMap(threadEntry); + forkRlock.unlock(); { std::lock_guard g(meta.lock_); // mark it as removed threadEntry->removed_ = true; auto elementsCapacity = threadEntry->getElementsCapacity(); - meta.removeThreadEntryFromAllInMap(threadEntry); auto beforeCount = meta.totalElementWrappers_.fetch_sub(elementsCapacity); DCHECK_GE(beforeCount, elementsCapacity); // No need to hold the lock any longer; the ThreadEntry is private to this @@ -142,7 +145,7 @@ void StaticMetaBase::onThreadExit(void* ptr) { shouldRun = true; } } - DCHECK(threadEntry->meta->isThreadEntryRemovedFromAllInMap(threadEntry)); + DCHECK(meta.isThreadEntryRemovedFromAllInMap(threadEntry, !meta.strict_)); } pthread_setspecific(meta.pthreadKey_, nullptr); } @@ -203,7 +206,7 @@ void StaticMetaBase::cleanupThreadEntriesAndList( // Fail safe check to make sure that the ThreadEntry is not present // before issuing a delete. - DCHECK(tmp->meta->isThreadEntryRemovedFromAllInMap(tmp)); + DCHECK(tmp->meta->isThreadEntryRemovedFromAllInMap(tmp, true)); delete tmp; } @@ -246,6 +249,7 @@ void StaticMetaBase::destroy(EntryID* ent) { ThreadEntrySet tmpEntrySet; { + std::shared_lock forkRlock(meta.forkHandlerLock_); std::unique_lock wlock(meta.accessAllThreadsLock_, std::defer_lock); if (meta.strict_) { /* @@ -258,15 +262,16 @@ void StaticMetaBase::destroy(EntryID* ent) { wlock.lock(); } + uint32_t id = + ent->value.exchange(kEntryIDInvalid, std::memory_order_acquire); + if (id == kEntryIDInvalid) { + return; + } + meta.allId2ThreadEntrySets_[id].swap(tmpEntrySet); + forkRlock.unlock(); + { std::lock_guard g(meta.lock_); - uint32_t id = - ent->value.exchange(kEntryIDInvalid, std::memory_order_relaxed); - if (id == kEntryIDInvalid) { - return; - } - - meta.allId2ThreadEntrySets_[id].swap(tmpEntrySet); for (auto& e : tmpEntrySet.threadEntries) { auto elementsCapacity = e->getElementsCapacity(); if (id < elementsCapacity) { diff --git a/folly/detail/ThreadLocalDetail.h b/folly/detail/ThreadLocalDetail.h index 9e5587f0d0b..f256efcbe58 100644 --- a/folly/detail/ThreadLocalDetail.h +++ b/folly/detail/ThreadLocalDetail.h @@ -64,7 +64,7 @@ constexpr uint32_t kEntryIDInvalid = std::numeric_limits::max(); struct ElementWrapper { using DeleterFunType = void(void*, TLPDestructionMode); - bool dispose(TLPDestructionMode mode) { + bool dispose(TLPDestructionMode mode) noexcept { if (ptr == nullptr) { return false; } @@ -118,7 +118,7 @@ struct ElementWrapper { ptr = p; } - void cleanup() { + void cleanup() noexcept { if (ownsDeleter) { delete deleter2; } @@ -454,7 +454,12 @@ struct StaticMetaBase { /* * Check if ThreadEntry* is present in the map for all slots of @ids. */ - FOLLY_ALWAYS_INLINE bool isThreadEntryRemovedFromAllInMap(ThreadEntry* te) { + FOLLY_ALWAYS_INLINE bool isThreadEntryRemovedFromAllInMap( + ThreadEntry* te, bool needForkLock) { + std::shared_lock rlocked(forkHandlerLock_, std::defer_lock); + if (needForkLock) { + rlocked.lock(); + } uint32_t maxId = nextId_.load(); for (uint32_t i = 0; i < maxId; ++i) { if (allId2ThreadEntrySets_[i].rlock()->contains(te)) { @@ -497,6 +502,17 @@ struct StaticMetaBase { // This is a map of all thread entries mapped to index i with active // elements[i]; folly::atomic_grow_array allId2ThreadEntrySets_; + + // Note on locking rules. There are 4 locks involved in managing StaticMeta: + // fork handler lock (getStaticMetaGlobalForkMutex(), + // access all threads lock (accessAllThreadsLock_), + // per thread entry set lock implicit in SynchronizedThreadEntrySet and + // meta lock (lock_) + // + // If multiple locks need to be acquired in a call path, the above is also + // the order in which they should be acquired. Additionally, if per + // ThreadEntrySet locks are the only ones that are acquired in a path, it + // must also acquire shared lock on the fork handler lock. }; struct FakeUniqueInstance { @@ -687,8 +703,10 @@ struct FOLLY_EXPORT StaticMeta final : StaticMetaBase { } static void onForkChild() { - // only the current thread survives auto& meta = instance(); + // only the current thread survives + meta.lock_.unlock(); + meta.accessAllThreadsLock_.unlock(); auto threadEntry = meta.threadEntry_(); // Loop through allId2ThreadEntrySets_; Only keep ThreadEntry* in the map // for ThreadEntry::elements that are still in use by the current thread. @@ -703,8 +721,6 @@ struct FOLLY_EXPORT StaticMeta final : StaticMetaBase { wlockedSet->clear(); } } - meta.lock_.unlock(); - meta.accessAllThreadsLock_.unlock(); meta.forkHandlerLock_.unlock(); } };