From be42394a1a5aae9689a7a02b7d9e9333829f37e6 Mon Sep 17 00:00:00 2001 From: Chip Turner Date: Wed, 3 Sep 2014 12:22:45 -0700 Subject: [PATCH] Fix bug in circular singleton creation detection Summary: We considered it circular if we tried to create a singleton while the singleton was being created. In a single threaded world, this is correct, but under concurrency, two threads can be in the singleton creation codepath and become confused about the state of the singleton. This change uses a condition variable to notify when creation completes so that other threads can wait on the creation to complete. Circular creation is detected via thread id. Test Plan: runtests (new test case; failed without the fix, passes with it) Reviewed By: hans@fb.com Subscribers: lins, anca, njormrod, rkroll FB internal diff: D1534081 --- folly/experimental/Singleton.cpp | 3 +- folly/experimental/Singleton.h | 44 ++++++++++++++++++----- folly/experimental/test/SingletonTest.cpp | 38 ++++++++++++++++++++ 3 files changed, 75 insertions(+), 10 deletions(-) diff --git a/folly/experimental/Singleton.cpp b/folly/experimental/Singleton.cpp index e02b8ca4..7ad7ec0a 100644 --- a/folly/experimental/Singleton.cpp +++ b/folly/experimental/Singleton.cpp @@ -33,7 +33,7 @@ void SingletonVault::destroyInstances() { auto it = singletons_.find(type); CHECK(it != singletons_.end()); auto& entry = it->second; - std::lock_guard entry_guard(entry->mutex_); + std::lock_guard entry_guard(entry->mutex); if (entry->instance.use_count() > 1) { LOG(ERROR) << "Singleton of type " << type.name() << " has a living " << "reference at destroyInstances time; beware! Raw pointer " @@ -42,6 +42,7 @@ void SingletonVault::destroyInstances() { } entry->instance.reset(); entry->state = SingletonEntryState::Dead; + entry->state_condvar.notify_all(); } creation_order_.clear(); diff --git a/folly/experimental/Singleton.h b/folly/experimental/Singleton.h index ef966dee..c861e1bc 100644 --- a/folly/experimental/Singleton.h +++ b/folly/experimental/Singleton.h @@ -81,6 +81,8 @@ #include #include +#include +#include #include #include #include @@ -181,7 +183,7 @@ class SingletonVault { entry.reset(new SingletonEntry); } - std::lock_guard entry_guard(entry->mutex_); + std::lock_guard entry_guard(entry->mutex); CHECK(entry->instance == nullptr); CHECK(create); CHECK(teardown); @@ -261,12 +263,21 @@ class SingletonVault { // its state as described above, and the create and teardown // functions. struct SingletonEntry { - std::mutex mutex_; + // mutex protects the entire entry + std::mutex mutex; + + // state changes notify state_condvar + SingletonEntryState state = SingletonEntryState::Dead; + std::condition_variable state_condvar; + + // the thread creating the singleton + std::thread::id creating_thread; + + // The singleton itself and related functions. std::shared_ptr instance; void* instance_ptr = nullptr; CreateFunc create = nullptr; TeardownFunc teardown = nullptr; - SingletonEntryState state = SingletonEntryState::Dead; SingletonEntry() = default; SingletonEntry(const SingletonEntry&) = delete; @@ -280,7 +291,7 @@ class SingletonVault { // necessary. SingletonEntry* get_entry(detail::TypeDescriptor type, std::unique_lock* lock) { - // mutex_ must be held when calling this function + // mutex must be held when calling this function if (state_ != SingletonVaultState::Running) { throw std::logic_error( "Attempt to load a singleton before " @@ -294,17 +305,31 @@ class SingletonVault { type.name()); } - auto& entry = it->second; - std::unique_lock entry_lock(entry->mutex_); + auto entry = it->second.get(); + std::unique_lock entry_lock(entry->mutex); if (entry->state == SingletonEntryState::BeingBorn) { - throw std::out_of_range(std::string("circular singleton dependency: ") + - type.name()); + // If this thread is trying to give birth to the singleton, it's + // a circular dependency and we must panic. + if (entry->creating_thread == std::this_thread::get_id()) { + throw std::out_of_range(std::string("circular singleton dependency: ") + + type.name()); + } + + // Otherwise, another thread is constructing the singleton; + // let's wait on a condvar to see it complete. We release and + // reaquire lock while waiting on the entry to resolve its state. + lock->unlock(); + entry->state_condvar.wait(entry_lock, [&entry]() { + return entry->state != SingletonEntryState::BeingBorn; + }); + lock->lock(); } if (entry->instance == nullptr) { CHECK(entry->state == SingletonEntryState::Dead); entry->state = SingletonEntryState::BeingBorn; + entry->creating_thread = std::this_thread::get_id(); entry_lock.unlock(); lock->unlock(); @@ -317,11 +342,12 @@ class SingletonVault { entry->instance = instance; entry->instance_ptr = instance.get(); entry->state = SingletonEntryState::Living; + entry->state_condvar.notify_all(); creation_order_.push_back(type); } CHECK(entry->state == SingletonEntryState::Living); - return entry.get(); + return entry; } mutable std::mutex mutex_; diff --git a/folly/experimental/test/SingletonTest.cpp b/folly/experimental/test/SingletonTest.cpp index 91c8bb79..3c3f1916 100644 --- a/folly/experimental/test/SingletonTest.cpp +++ b/folly/experimental/test/SingletonTest.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#include + #include #include @@ -307,6 +309,42 @@ TEST(Singleton, SingletonDependencies) { std::out_of_range); } +// A test to ensure multiple threads contending on singleton creation +// properly wait for creation rather than thinking it is a circular +// dependency. +class Slowpoke { + public: + Slowpoke() { std::this_thread::sleep_for(std::chrono::seconds(1)); } +}; + +TEST(Singleton, SingletonConcurrency) { + SingletonVault vault; + Singleton slowpoke_singleton(nullptr, nullptr, &vault); + vault.registrationComplete(); + + std::mutex gatekeeper; + gatekeeper.lock(); + auto func = [&vault, &gatekeeper]() { + gatekeeper.lock(); + gatekeeper.unlock(); + auto unused = Singleton::get(&vault); + }; + + EXPECT_EQ(vault.livingSingletonCount(), 0); + std::vector threads; + for (int i = 0; i < 100; ++i) { + threads.emplace_back(func); + } + // If circular dependency checks fail, the unlock would trigger a + // crash. Instead, it succeeds, and we have exactly one living + // singleton. + gatekeeper.unlock(); + for (auto& t : threads) { + t.join(); + } + EXPECT_EQ(vault.livingSingletonCount(), 1); +} + // Benchmarking a normal singleton vs a Meyers singleton vs a Folly // singleton. Meyers are insanely fast, but (hopefully) Folly // singletons are fast "enough." -- 2.34.1