From f321edf8a5e1d750e975ffe7e986aacc4d736253 Mon Sep 17 00:00:00 2001 From: Andrii Grynenko Date: Tue, 7 Oct 2014 18:12:33 -0700 Subject: [PATCH] Don't re-create singletons Summary: This forbids singleton creation/re-creation after destroyInstances() was called. Only after reset() is called singletons can be created again. registrationComplete() behavior is also slightly change. We disallow singleton registration after registrationComplete() is called even in Relaxed mode. Strict mode now only controlls whether singletons can be constructed before registation is complete. Test Plan: unit test Reviewed By: chip@fb.com Subscribers: hphp-diffs@, ps, trunkagent, njormrod, mshneer, lins FB internal diff: D1605136 --- folly/experimental/Singleton.cpp | 17 ++++++ folly/experimental/Singleton.h | 65 +++++++++++++++++------ folly/experimental/test/SingletonTest.cpp | 9 ++-- 3 files changed, 71 insertions(+), 20 deletions(-) diff --git a/folly/experimental/Singleton.cpp b/folly/experimental/Singleton.cpp index 4cfacd5a..27573fd2 100644 --- a/folly/experimental/Singleton.cpp +++ b/folly/experimental/Singleton.cpp @@ -23,6 +23,15 @@ namespace folly { SingletonVault::~SingletonVault() { destroyInstances(); } void SingletonVault::destroyInstances() { + RWSpinLock::WriteHolder state_wh(&stateMutex_); + + if (state_ == SingletonVaultState::Quiescing) { + return; + } + state_ = SingletonVaultState::Quiescing; + + RWSpinLock::ReadHolder state_rh(std::move(state_wh)); + { RWSpinLock::ReadHolder rh(&mutex_); @@ -54,6 +63,14 @@ void SingletonVault::destroyInstances() { } } +void SingletonVault::reenableInstances() { + RWSpinLock::WriteHolder state_wh(&stateMutex_); + + stateCheck(SingletonVaultState::Quiescing); + + state_ = SingletonVaultState::Running; +} + SingletonVault* SingletonVault::singleton() { static SingletonVault* vault = new SingletonVault(); diff --git a/folly/experimental/Singleton.h b/folly/experimental/Singleton.h index ed912abe..99b633f0 100644 --- a/folly/experimental/Singleton.h +++ b/folly/experimental/Singleton.h @@ -74,6 +74,9 @@ // TeardownFunc for each singleton, in the reverse order they were // created. It is your responsibility to ensure your singletons can // handle cases where the singletons they depend on go away, however. +// Singletons won't be recreated after destroyInstances call. If you +// want to re-enable singleton creation (say after fork was called) you +// should call reenableInstances. #pragma once #include @@ -179,9 +182,16 @@ class SingletonVault { void registerSingleton(detail::TypeDescriptor type, CreateFunc create, TeardownFunc teardown) { + RWSpinLock::ReadHolder rh(&stateMutex_); + + stateCheck(SingletonVaultState::Running); + if (UNLIKELY(registrationComplete_)) { + throw std::logic_error( + "Registering singleton after registrationComplete()."); + } + RWSpinLock::WriteHolder wh(&mutex_); - stateCheck(SingletonVaultState::Registering); CHECK_THROW(singletons_.find(type) == singletons_.end(), std::logic_error); auto& entry = singletons_[type]; entry.reset(new SingletonEntry); @@ -198,19 +208,36 @@ class SingletonVault { // Mark registration is complete; no more singletons can be // registered at this point. void registrationComplete() { - RWSpinLock::WriteHolder wh(&mutex_); + RWSpinLock::WriteHolder wh(&stateMutex_); + + stateCheck(SingletonVaultState::Running); - stateCheck(SingletonVaultState::Registering); - state_ = SingletonVaultState::Running; + if (type_ == Type::Strict) { + for (const auto& id_singleton_entry: singletons_) { + const auto& singleton_entry = *id_singleton_entry.second; + if (singleton_entry.state != SingletonEntryState::Dead) { + throw std::runtime_error( + "Singleton created before registration was complete."); + } + } + } + + registrationComplete_ = true; } - // Destroy all singletons; when complete, the vault can create - // singletons once again, or remain dormant. + // Destroy all singletons; when complete, the vault can't create + // singletons once again until reenableInstances() is called. void destroyInstances(); + // Enable re-creating singletons after destroyInstances() was called. + void reenableInstances(); + // Retrieve a singleton from the vault, creating it if necessary. std::shared_ptr get_shared(detail::TypeDescriptor type) { auto entry = get_entry_create(type); + if (UNLIKELY(!entry)) { + return std::shared_ptr(); + } return entry->instance; } @@ -220,6 +247,10 @@ class SingletonVault { // the weak_ptr interface for true safety. void* get_ptr(detail::TypeDescriptor type) { auto entry = get_entry_create(type); + if (UNLIKELY(!entry)) { + throw std::runtime_error( + "Raw pointer to a singleton requested after its destruction."); + } return entry->instance_ptr; } @@ -251,8 +282,8 @@ class SingletonVault { private: // The two stages of life for a vault, as mentioned in the class comment. enum class SingletonVaultState { - Registering, Running, + Quiescing, }; // Each singleton in the vault can be in three states: dead @@ -266,7 +297,7 @@ class SingletonVault { void stateCheck(SingletonVaultState expected, const char* msg="Unexpected singleton state change") { - if (type_ == Type::Strict && expected != state_) { + if (expected != state_) { throw std::logic_error(msg); } } @@ -301,13 +332,6 @@ class SingletonVault { SingletonEntry* get_entry(detail::TypeDescriptor type) { RWSpinLock::ReadHolder rh(&mutex_); - // mutex must be held when calling this function - stateCheck( - SingletonVaultState::Running, - "Attempt to load a singleton before " - "SingletonVault::registrationComplete was called (hint: you probably " - "didn't call initFacebook)"); - auto it = singletons_.find(type); if (it == singletons_.end()) { throw std::out_of_range(std::string("non-existent singleton: ") + @@ -339,6 +363,11 @@ class SingletonVault { } if (entry->instance == nullptr) { + RWSpinLock::ReadHolder rh(&stateMutex_); + if (state_ == SingletonVaultState::Quiescing) { + return nullptr; + } + CHECK(entry->state == SingletonEntryState::Dead); entry->state = SingletonEntryState::BeingBorn; entry->creating_thread = std::this_thread::get_id(); @@ -370,8 +399,10 @@ class SingletonVault { SingletonEntryPtr, detail::TypeDescriptorHasher> singletons_; std::vector creation_order_; - SingletonVaultState state_ = SingletonVaultState::Registering; - Type type_ = Type::Relaxed; + SingletonVaultState state_{SingletonVaultState::Running}; + bool registrationComplete_{false}; + folly::RWSpinLock stateMutex_; + Type type_{Type::Relaxed}; }; // This is the wrapper class that most users actually interact with. diff --git a/folly/experimental/test/SingletonTest.cpp b/folly/experimental/test/SingletonTest.cpp index 9c28e5ec..3a42202f 100644 --- a/folly/experimental/test/SingletonTest.cpp +++ b/folly/experimental/test/SingletonTest.cpp @@ -188,9 +188,6 @@ TEST(Singleton, NaughtyUsage) { }(), std::logic_error); - // Default vault is non-strict; this should work. - Singleton global_watchdog_singleton; - SingletonVault vault_2(SingletonVault::Type::Strict); EXPECT_THROW(Singleton::get(&vault_2), std::logic_error); Singleton watchdog_singleton(nullptr, nullptr, &vault_2); @@ -263,6 +260,12 @@ TEST(Singleton, SharedPtrUsage) { locked_s1 = weak_s1.lock(); EXPECT_TRUE(weak_s1.expired()); + auto empty_s1 = Singleton::get_weak(&vault); + EXPECT_FALSE(empty_s1.lock()); + + vault.reenableInstances(); + + // Singleton should be re-created only after reenableInstances() was called. Watchdog* new_s1 = Singleton::get(&vault); EXPECT_NE(new_s1->serial_number, old_serial); } -- 2.34.1