Commit 0416e1ea authored by Andrii Grynenko's avatar Andrii Grynenko Committed by Facebook Github Bot

Fix wrong use of upgrade lock

Reviewed By: yfeldblum, nbronson

Differential Revision: D4149681

fbshipit-source-id: 37bd1b0b7d1ad6e6fa813228307abebfe772012f
parent db9e283c
...@@ -72,12 +72,11 @@ void SingletonHolder<T>::registerSingletonMock(CreateFunc c, TeardownFunc t) { ...@@ -72,12 +72,11 @@ void SingletonHolder<T>::registerSingletonMock(CreateFunc c, TeardownFunc t) {
destroyInstance(); destroyInstance();
{ {
RWSpinLock::WriteHolder wh(&vault_.mutex_); auto creationOrder = vault_.creationOrder_.wlock();
auto it = std::find( auto it = std::find(creationOrder->begin(), creationOrder->end(), type());
vault_.creation_order_.begin(), vault_.creation_order_.end(), type()); if (it != creationOrder->end()) {
if (it != vault_.creation_order_.end()) { creationOrder->erase(it);
vault_.creation_order_.erase(it);
} }
} }
...@@ -224,8 +223,8 @@ void SingletonHolder<T>::createInstance() { ...@@ -224,8 +223,8 @@ void SingletonHolder<T>::createInstance() {
creating_thread_.store(std::this_thread::get_id(), std::memory_order_release); creating_thread_.store(std::this_thread::get_id(), std::memory_order_release);
RWSpinLock::ReadHolder rh(&vault_.stateMutex_); auto state = vault_.state_.rlock();
if (vault_.state_ == SingletonVault::SingletonVaultState::Quiescing) { if (state->state == SingletonVault::SingletonVaultState::Quiescing) {
if (vault_.type_ != SingletonVault::Type::Relaxed) { if (vault_.type_ != SingletonVault::Type::Relaxed) {
LOG(FATAL) << "Requesting singleton after vault was destroyed."; LOG(FATAL) << "Requesting singleton after vault was destroyed.";
} }
...@@ -278,10 +277,7 @@ void SingletonHolder<T>::createInstance() { ...@@ -278,10 +277,7 @@ void SingletonHolder<T>::createInstance() {
// may access instance and instance_weak w/o synchronization. // may access instance and instance_weak w/o synchronization.
state_.store(SingletonHolderState::Living, std::memory_order_release); state_.store(SingletonHolderState::Living, std::memory_order_release);
{ vault_.creationOrder_.wlock()->push_back(type());
RWSpinLock::WriteHolder wh(&vault_.mutex_);
vault_.creation_order_.push_back(type());
}
} }
} }
......
...@@ -73,48 +73,41 @@ FatalHelper __attribute__ ((__init_priority__ (101))) fatalHelper; ...@@ -73,48 +73,41 @@ FatalHelper __attribute__ ((__init_priority__ (101))) fatalHelper;
SingletonVault::~SingletonVault() { destroyInstances(); } SingletonVault::~SingletonVault() { destroyInstances(); }
void SingletonVault::registerSingleton(detail::SingletonHolderBase* entry) { void SingletonVault::registerSingleton(detail::SingletonHolderBase* entry) {
RWSpinLock::ReadHolder rh(&stateMutex_); auto state = state_.rlock();
stateCheck(SingletonVaultState::Running, *state);
stateCheck(SingletonVaultState::Running); if (UNLIKELY(state->registrationComplete)) {
if (UNLIKELY(registrationComplete_)) {
LOG(ERROR) << "Registering singleton after registrationComplete()."; LOG(ERROR) << "Registering singleton after registrationComplete().";
} }
RWSpinLock::ReadHolder rhMutex(&mutex_); auto singletons = singletons_.wlock();
CHECK_THROW(singletons_.find(entry->type()) == singletons_.end(), CHECK_THROW(
std::logic_error); singletons->emplace(entry->type(), entry).second, std::logic_error);
RWSpinLock::UpgradedHolder wh(&mutex_);
singletons_[entry->type()] = entry;
} }
void SingletonVault::addEagerInitSingleton(detail::SingletonHolderBase* entry) { void SingletonVault::addEagerInitSingleton(detail::SingletonHolderBase* entry) {
RWSpinLock::ReadHolder rh(&stateMutex_); auto state = state_.rlock();
stateCheck(SingletonVaultState::Running, *state);
stateCheck(SingletonVaultState::Running);
if (UNLIKELY(registrationComplete_)) { if (UNLIKELY(state->registrationComplete)) {
LOG(ERROR) << "Registering for eager-load after registrationComplete()."; LOG(ERROR) << "Registering for eager-load after registrationComplete().";
} }
RWSpinLock::ReadHolder rhMutex(&mutex_); CHECK_THROW(singletons_.rlock()->count(entry->type()), std::logic_error);
CHECK_THROW(singletons_.find(entry->type()) != singletons_.end(),
std::logic_error);
RWSpinLock::UpgradedHolder wh(&mutex_); auto eagerInitSingletons = eagerInitSingletons_.wlock();
eagerInitSingletons_.insert(entry); eagerInitSingletons->insert(entry);
} }
void SingletonVault::registrationComplete() { void SingletonVault::registrationComplete() {
std::atexit([](){ SingletonVault::singleton()->destroyInstances(); }); std::atexit([](){ SingletonVault::singleton()->destroyInstances(); });
RWSpinLock::WriteHolder wh(&stateMutex_); auto state = state_.wlock();
stateCheck(SingletonVaultState::Running, *state);
stateCheck(SingletonVaultState::Running);
auto singletons = singletons_.rlock();
if (type_ == Type::Strict) { if (type_ == Type::Strict) {
for (const auto& p : singletons_) { for (const auto& p : *singletons) {
if (p.second->hasLiveInstance()) { if (p.second->hasLiveInstance()) {
throw std::runtime_error( throw std::runtime_error(
"Singleton created before registration was complete."); "Singleton created before registration was complete.");
...@@ -122,38 +115,37 @@ void SingletonVault::registrationComplete() { ...@@ -122,38 +115,37 @@ void SingletonVault::registrationComplete() {
} }
} }
registrationComplete_ = true; state->registrationComplete = true;
} }
void SingletonVault::doEagerInit() { void SingletonVault::doEagerInit() {
std::unordered_set<detail::SingletonHolderBase*> singletonSet;
{ {
RWSpinLock::ReadHolder rh(&stateMutex_); auto state = state_.rlock();
stateCheck(SingletonVaultState::Running); stateCheck(SingletonVaultState::Running, *state);
if (UNLIKELY(!registrationComplete_)) { if (UNLIKELY(!state->registrationComplete)) {
throw std::logic_error("registrationComplete() not yet called"); throw std::logic_error("registrationComplete() not yet called");
} }
singletonSet = eagerInitSingletons_; // copy set of pointers
} }
for (auto *single : singletonSet) { auto eagerInitSingletons = eagerInitSingletons_.rlock();
for (auto* single : *eagerInitSingletons) {
single->createInstance(); single->createInstance();
} }
} }
void SingletonVault::doEagerInitVia(Executor& exe, folly::Baton<>* done) { void SingletonVault::doEagerInitVia(Executor& exe, folly::Baton<>* done) {
std::unordered_set<detail::SingletonHolderBase*> singletonSet;
{ {
RWSpinLock::ReadHolder rh(&stateMutex_); auto state = state_.rlock();
stateCheck(SingletonVaultState::Running); stateCheck(SingletonVaultState::Running, *state);
if (UNLIKELY(!registrationComplete_)) { if (UNLIKELY(!state->registrationComplete)) {
throw std::logic_error("registrationComplete() not yet called"); throw std::logic_error("registrationComplete() not yet called");
} }
singletonSet = eagerInitSingletons_; // copy set of pointers
} }
auto countdown = std::make_shared<std::atomic<size_t>>(singletonSet.size()); auto eagerInitSingletons = eagerInitSingletons_.rlock();
for (auto* single : singletonSet) { auto countdown =
std::make_shared<std::atomic<size_t>>(eagerInitSingletons->size());
for (auto* single : *eagerInitSingletons) {
// countdown is retained by shared_ptr, and will be alive until last lambda // countdown is retained by shared_ptr, and will be alive until last lambda
// is done. notifyBaton is provided by the caller, and expected to remain // is done. notifyBaton is provided by the caller, and expected to remain
// present (if it's non-nullptr). singletonSet can go out of scope but // present (if it's non-nullptr). singletonSet can go out of scope but
...@@ -179,36 +171,35 @@ void SingletonVault::doEagerInitVia(Executor& exe, folly::Baton<>* done) { ...@@ -179,36 +171,35 @@ void SingletonVault::doEagerInitVia(Executor& exe, folly::Baton<>* done) {
} }
void SingletonVault::destroyInstances() { void SingletonVault::destroyInstances() {
RWSpinLock::WriteHolder state_wh(&stateMutex_); auto stateW = state_.wlock();
if (stateW->state == SingletonVaultState::Quiescing) {
if (state_ == SingletonVaultState::Quiescing) {
return; return;
} }
state_ = SingletonVaultState::Quiescing; stateW->state = SingletonVaultState::Quiescing;
RWSpinLock::ReadHolder state_rh(std::move(state_wh));
auto stateR = stateW.moveFromWriteToRead();
{ {
RWSpinLock::ReadHolder rh(&mutex_); auto singletons = singletons_.rlock();
auto creationOrder = creationOrder_.rlock();
CHECK_GE(singletons_.size(), creation_order_.size()); CHECK_GE(singletons->size(), creationOrder->size());
// Release all ReadMostlyMainPtrs at once // Release all ReadMostlyMainPtrs at once
{ {
ReadMostlyMainPtrDeleter<> deleter; ReadMostlyMainPtrDeleter<> deleter;
for (auto& singleton_type : creation_order_) { for (auto& singleton_type : *creationOrder) {
singletons_[singleton_type]->preDestroyInstance(deleter); singletons->at(singleton_type)->preDestroyInstance(deleter);
} }
} }
for (auto type_iter = creation_order_.rbegin(); for (auto type_iter = creationOrder->rbegin();
type_iter != creation_order_.rend(); type_iter != creationOrder->rend();
++type_iter) { ++type_iter) {
singletons_[*type_iter]->destroyInstance(); singletons->at(*type_iter)->destroyInstance();
} }
for (auto& singleton_type: creation_order_) { for (auto& singleton_type : *creationOrder) {
auto singleton = singletons_[singleton_type]; auto singleton = singletons->at(singleton_type);
if (!singleton->hasLiveInstance()) { if (!singleton->hasLiveInstance()) {
continue; continue;
} }
...@@ -218,17 +209,17 @@ void SingletonVault::destroyInstances() { ...@@ -218,17 +209,17 @@ void SingletonVault::destroyInstances() {
} }
{ {
RWSpinLock::WriteHolder wh(&mutex_); auto creationOrder = creationOrder_.wlock();
creation_order_.clear(); creationOrder->clear();
} }
} }
void SingletonVault::reenableInstances() { void SingletonVault::reenableInstances() {
RWSpinLock::WriteHolder state_wh(&stateMutex_); auto state = state_.wlock();
stateCheck(SingletonVaultState::Quiescing); stateCheck(SingletonVaultState::Quiescing, *state);
state_ = SingletonVaultState::Running; state->state = SingletonVaultState::Running;
} }
void SingletonVault::scheduleDestroyInstances() { void SingletonVault::scheduleDestroyInstances() {
......
...@@ -112,14 +112,15 @@ ...@@ -112,14 +112,15 @@
#pragma once #pragma once
#include <folly/Baton.h> #include <folly/Baton.h>
#include <folly/Demangle.h>
#include <folly/Exception.h> #include <folly/Exception.h>
#include <folly/Executor.h>
#include <folly/Hash.h> #include <folly/Hash.h>
#include <folly/Memory.h> #include <folly/Memory.h>
#include <folly/RWSpinLock.h> #include <folly/RWSpinLock.h>
#include <folly/Demangle.h> #include <folly/Synchronized.h>
#include <folly/Executor.h>
#include <folly/experimental/ReadMostlySharedPtr.h>
#include <folly/detail/StaticSingletonManager.h> #include <folly/detail/StaticSingletonManager.h>
#include <folly/experimental/ReadMostlySharedPtr.h>
#include <algorithm> #include <algorithm>
#include <atomic> #include <atomic>
...@@ -409,9 +410,7 @@ class SingletonVault { ...@@ -409,9 +410,7 @@ class SingletonVault {
// For testing; how many registered and living singletons we have. // For testing; how many registered and living singletons we have.
size_t registeredSingletonCount() const { size_t registeredSingletonCount() const {
RWSpinLock::ReadHolder rh(&mutex_); return singletons_.rlock()->size();
return singletons_.size();
} }
/** /**
...@@ -421,10 +420,10 @@ class SingletonVault { ...@@ -421,10 +420,10 @@ class SingletonVault {
bool eagerInitComplete() const; bool eagerInitComplete() const;
size_t livingSingletonCount() const { size_t livingSingletonCount() const {
RWSpinLock::ReadHolder rh(&mutex_); auto singletons = singletons_.rlock();
size_t ret = 0; size_t ret = 0;
for (const auto& p : singletons_) { for (const auto& p : *singletons) {
if (p.second->hasLiveInstance()) { if (p.second->hasLiveInstance()) {
++ret; ++ret;
} }
...@@ -470,13 +469,20 @@ class SingletonVault { ...@@ -470,13 +469,20 @@ class SingletonVault {
Quiescing, Quiescing,
}; };
struct State {
SingletonVaultState state{SingletonVaultState::Running};
bool registrationComplete{false};
};
// Each singleton in the vault can be in two states: dead // Each singleton in the vault can be in two states: dead
// (registered but never created), living (CreateFunc returned an instance). // (registered but never created), living (CreateFunc returned an instance).
void stateCheck(SingletonVaultState expected, static void stateCheck(
const char* msg="Unexpected singleton state change") { SingletonVaultState expected,
if (expected != state_) { const State& state,
throw std::logic_error(msg); const char* msg = "Unexpected singleton state change") {
if (expected != state.state) {
throw std::logic_error(msg);
} }
} }
...@@ -496,14 +502,13 @@ class SingletonVault { ...@@ -496,14 +502,13 @@ class SingletonVault {
typedef std::unordered_map<detail::TypeDescriptor, typedef std::unordered_map<detail::TypeDescriptor,
detail::SingletonHolderBase*, detail::SingletonHolderBase*,
detail::TypeDescriptorHasher> SingletonMap; detail::TypeDescriptorHasher> SingletonMap;
folly::Synchronized<SingletonMap> singletons_;
folly::Synchronized<std::unordered_set<detail::SingletonHolderBase*>>
eagerInitSingletons_;
folly::Synchronized<std::vector<detail::TypeDescriptor>> creationOrder_;
folly::Synchronized<State> state_;
mutable folly::RWSpinLock mutex_;
SingletonMap singletons_;
std::unordered_set<detail::SingletonHolderBase*> eagerInitSingletons_;
std::vector<detail::TypeDescriptor> creation_order_;
SingletonVaultState state_{SingletonVaultState::Running};
bool registrationComplete_{false};
folly::RWSpinLock stateMutex_;
Type type_{Type::Relaxed}; Type type_{Type::Relaxed};
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment