folly Singleton: clear some state if creator function fails

Summary: The creator thread ID is saved to indicate the singleton is already being built (to help detect dependency cycles).  However if the creation function throws an error, that thread ID persists, and then if the same thread tries again to build the singleton it will be falsely detected as a dependency cycle.  This clears that state in the case of failure.

Reviewed By: @chipturner

Differential Revision: D2256441

diff --git a/folly/Singleton-inl.h b/folly/Singleton-inl.h
index ecfeda1ab5061fed88d04826d673eadeb3c67506..6bc43ad27d6af4e51d0a295ab1219ce82a33dcb7 100644 (file)
--- a/folly/Singleton-inl.h
+++ b/folly/Singleton-inl.h
@@ -139,11 +139,17 @@ void SingletonHolder<T>::createInstance() {
     return;
   }
 
+  SCOPE_EXIT {
+    // Clean up creator thread when complete, and also, in case of errors here,
+    // so that subsequent attempts don't think this is still in the process of
+    // being built.
+    creating_thread_ = std::thread::id();
+  };
+
   creating_thread_ = std::this_thread::get_id();
 
   RWSpinLock::ReadHolder rh(&vault_.stateMutex_);
   if (vault_.state_ == SingletonVault::SingletonVaultState::Quiescing) {
-    creating_thread_ = std::thread::id();
     return;
   }
 
@@ -184,7 +190,6 @@ void SingletonHolder<T>::createInstance() {
 
   instance_weak_ = instance_;
   instance_ptr_ = instance_.get();
-  creating_thread_ = std::thread::id();
   destroy_baton_ = std::move(destroy_baton);
   print_destructor_stack_trace_ = std::move(print_destructor_stack_trace);
 

diff --git a/folly/test/SingletonTest.cpp b/folly/test/SingletonTest.cpp
index b4ffdbf6b486fb49691a4f15fc2bd3ab0f698587..8505d0686ffcb8d8a99ddd7b3f44a1db5be8cc5b 100644 (file)
--- a/folly/test/SingletonTest.cpp
+++ b/folly/test/SingletonTest.cpp
@@ -414,6 +414,32 @@ TEST(Singleton, SingletonConcurrency) {
   EXPECT_EQ(vault.livingSingletonCount(), 1);
 }
 
+struct ErrorConstructor {
+  static size_t constructCount_;
+  ErrorConstructor() {
+    if ((constructCount_++) == 0) {
+      throw std::runtime_error("first time fails");
+    }
+  }
+};
+size_t ErrorConstructor::constructCount_(0);
+
+struct CreationErrorTag {};
+template <typename T, typename Tag = detail::DefaultTag>
+using SingletonCreationError = Singleton<T, Tag, CreationErrorTag>;
+
+TEST(Singleton, SingletonCreationError) {
+  auto& vault = *SingletonVault::singleton<CreationErrorTag>();
+  SingletonCreationError<ErrorConstructor> error_once_singleton;
+
+  // first time should error out
+  EXPECT_THROW(error_once_singleton.get_weak().lock(), std::runtime_error);
+
+  // second time it'll work fine
+  error_once_singleton.get_weak().lock();
+  SUCCEED();
+}
+
 struct ConcurrencyStressTag {};
 template <typename T, typename Tag = detail::DefaultTag>
 using SingletonConcurrencyStress = Singleton <T, Tag, ConcurrencyStressTag>;