Wait for some time if Singleton isn't destroyed immediately

Summary: This diff introduces 5 seconds wait time to let other threads release Singleton which may be temporarily locked. This helps prevent most of "Singleton object alive after destruction" warnings in cases where weak_ptr API is used correctly. Abusive use of folly::Singletons, where dependencies between singletons are not properly defined will still cause a warning.

Test Plan: unit test

Reviewed By: chip@fb.com

Subscribers: trunkagent, folly-diffs@

FB internal diff: D1808371

Signature: t1:1808371:1422487261:573eb40b6a260e428d96be476659335250c7ea76

diff --git a/folly/experimental/Singleton.cpp b/folly/experimental/Singleton.cpp
index f77c5c520bf0f39d3f33a0d7b27252a883dab749..f60fb78cd37945e523101cc1f055a85aa95422a1 100644 (file)
--- a/folly/experimental/Singleton.cpp
+++ b/folly/experimental/Singleton.cpp
@@ -20,6 +20,12 @@
 
 namespace folly {
 
+namespace {
+
+static constexpr std::chrono::seconds kDestroyWaitTime{5};
+
+}
+
 SingletonVault::~SingletonVault() { destroyInstances(); }
 
 void SingletonVault::destroyInstances() {
@@ -57,14 +63,18 @@ void SingletonVault::destroyInstances() {
 void SingletonVault::destroyInstance(SingletonMap::iterator entry_it) {
   const auto& type = entry_it->first;
   auto& entry = *(entry_it->second);
-  if (entry.instance.use_count() > 1) {
+
+  entry.state = detail::SingletonEntryState::Dead;
+  entry.instance.reset();
+  auto wait_result = entry.destroy_baton->timed_wait(
+    std::chrono::steady_clock::now() + kDestroyWaitTime);
+  if (!wait_result) {
     LOG(ERROR) << "Singleton of type " << type.name() << " has a living "
                << "reference at destroyInstances time; beware! Raw pointer "
-               << "is " << entry.instance.get() << " with use_count of "
-               << entry.instance.use_count();
+               << "is " << entry.instance_ptr << ". It is very likely that "
+               << "some other singleton is holding a shared_ptr to it. Make "
+               << "dependencies between these singletons are properly defined.";
   }
-  entry.state = detail::SingletonEntryState::Dead;
-  entry.instance.reset();
 }
 
 void SingletonVault::reenableInstances() {

diff --git a/folly/experimental/Singleton.h b/folly/experimental/Singleton.h
index 11ff198f7a86d24e5f0cf7432909ca126da5b1c6..7c9d6e0704bcb7128577f6a8b68887d69a97c5aa 100644 (file)
--- a/folly/experimental/Singleton.h
+++ b/folly/experimental/Singleton.h
@@ -92,6 +92,7 @@
 // should call reenableInstances.
 
 #pragma once
+#include <folly/Baton.h>
 #include <folly/Exception.h>
 #include <folly/Hash.h>
 #include <folly/Memory.h>
@@ -223,6 +224,8 @@ struct SingletonEntry {
   // safe to read it from different threads w/o synchronization if we know
   // that state is set to Living
   std::weak_ptr<void> instance_weak;
+  // Time we wait on destroy_baton after releasing Singleton shared_ptr.
+  std::shared_ptr<folly::Baton<>> destroy_baton;
   void* instance_ptr = nullptr;
   CreateFunc create = nullptr;
   TeardownFunc teardown = nullptr;
@@ -471,8 +474,16 @@ class SingletonVault {
       return entry;
     }
 
+    auto destroy_baton = std::make_shared<folly::Baton<>>();
+    auto teardown = entry->teardown;
+
     // Can't use make_shared -- no support for a custom deleter, sadly.
-    auto instance = std::shared_ptr<void>(entry->create(), entry->teardown);
+    auto instance = std::shared_ptr<void>(
+      entry->create(),
+      [destroy_baton, teardown](void* instance_ptr) mutable {
+        teardown(instance_ptr);
+        destroy_baton->post();
+      });
 
     // We should schedule destroyInstances() only after the singleton was
     // created. This will ensure it will be destroyed before singletons,
@@ -484,6 +495,7 @@ class SingletonVault {
     entry->instance_weak = instance;
     entry->instance_ptr = instance.get();
     entry->creating_thread = std::thread::id();
+    entry->destroy_baton = std::move(destroy_baton);
 
     // This has to be the last step, because once state is Living other threads
     // may access instance and instance_weak w/o synchronization.

diff --git a/folly/experimental/test/SingletonTest.cpp b/folly/experimental/test/SingletonTest.cpp
index 5d43c1be65924ee0b0fc8545e4d2ae597850602c..25b270a6eb8f3cb5a96521db8197b6b692beb4c7 100644 (file)
--- a/folly/experimental/test/SingletonTest.cpp
+++ b/folly/experimental/test/SingletonTest.cpp
@@ -241,8 +241,14 @@ TEST(Singleton, SharedPtrUsage) {
     EXPECT_NE(locked.get(), shared_s1.get());
   }
 
-  LOG(ERROR) << "The following log message regarding ref counts is expected";
-  vault.destroyInstances();
+  LOG(ERROR) << "The following log message regarding shared_ptr is expected";
+  {
+    auto start_time = std::chrono::steady_clock::now();
+    vault.destroyInstances();
+    auto duration = std::chrono::steady_clock::now() - start_time;
+    EXPECT_TRUE(duration > std::chrono::seconds{4} &&
+                duration < std::chrono::seconds{6});
+  }
   EXPECT_EQ(vault.registeredSingletonCount(), 3);
   EXPECT_EQ(vault.livingSingletonCount(), 0);
 
@@ -270,6 +276,23 @@ TEST(Singleton, SharedPtrUsage) {
   // Singleton should be re-created only after reenableInstances() was called.
   Watchdog* new_s1 = Singleton<Watchdog>::get(&vault);
   EXPECT_NE(new_s1->serial_number, old_serial);
+
+  auto new_s1_weak = Singleton<Watchdog>::get_weak(&vault);
+  auto new_s1_shared = new_s1_weak.lock();
+  std::thread t([new_s1_shared]() mutable {
+      std::this_thread::sleep_for(std::chrono::seconds{2});
+      new_s1_shared.reset();
+    });
+  new_s1_shared.reset();
+  {
+    auto start_time = std::chrono::steady_clock::now();
+    vault.destroyInstances();
+    auto duration = std::chrono::steady_clock::now() - start_time;
+    EXPECT_TRUE(duration > std::chrono::seconds{1} &&
+                duration < std::chrono::seconds{3});
+  }
+  EXPECT_TRUE(new_s1_weak.expired());
+  t.join();
 }
 
 // Some classes to test singleton dependencies.  NeedySingleton has a