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

diff --git a/folly/experimental/Singleton.cpp b/folly/experimental/Singleton.cpp
index e02b8ca45d97bd12a11c62c4f3fd59cedc4e8d30..7ad7ec0af0edda87ba37478a95c74d8af7d03f48 100644 (file)
--- 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<std::mutex> entry_guard(entry->mutex_);
+    std::lock_guard<std::mutex> 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 ef966dee273d161531f62f1f7509b8cc3328d95b..c861e1bc18cdb11110c401facf1bcf9ff74660df 100644 (file)
--- a/folly/experimental/Singleton.h
+++ b/folly/experimental/Singleton.h
@@ -81,6 +81,8 @@
 
 #include <vector>
 #include <mutex>
+#include <thread>
+#include <condition_variable>
 #include <string>
 #include <unordered_map>
 #include <functional>
@@ -181,7 +183,7 @@ class SingletonVault {
       entry.reset(new SingletonEntry);
     }
 
-    std::lock_guard<std::mutex> entry_guard(entry->mutex_);
+    std::lock_guard<std::mutex> 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<void> 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<std::mutex>* 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<std::mutex> entry_lock(entry->mutex_);
+    auto entry = it->second.get();
+    std::unique_lock<std::mutex> 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 91c8bb79b8ffdde5bf6d078de483d46be0721782..3c3f19164cd77c9a8abbd058ac1b6c37a022c816 100644 (file)
--- a/folly/experimental/test/SingletonTest.cpp
+++ b/folly/experimental/test/SingletonTest.cpp
@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+#include <thread>
+
 #include <folly/experimental/Singleton.h>
 
 #include <folly/Benchmark.h>
@@ -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> slowpoke_singleton(nullptr, nullptr, &vault);
+  vault.registrationComplete();
+
+  std::mutex gatekeeper;
+  gatekeeper.lock();
+  auto func = [&vault, &gatekeeper]() {
+    gatekeeper.lock();
+    gatekeeper.unlock();
+    auto unused = Singleton<Slowpoke>::get(&vault);
+  };
+
+  EXPECT_EQ(vault.livingSingletonCount(), 0);
+  std::vector<std::thread> 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."