From 32ab0252eb4d382896575ba2ae69f8ac41030005 Mon Sep 17 00:00:00 2001 From: Andrii Grynenko Date: Tue, 27 Jan 2015 20:09:27 -0800 Subject: [PATCH] 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 --- folly/experimental/Singleton.cpp | 20 ++++++++++++----- folly/experimental/Singleton.h | 14 +++++++++++- folly/experimental/test/SingletonTest.cpp | 27 +++++++++++++++++++++-- 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/folly/experimental/Singleton.cpp b/folly/experimental/Singleton.cpp index f77c5c52..f60fb78c 100644 --- 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 11ff198f..7c9d6e07 100644 --- a/folly/experimental/Singleton.h +++ b/folly/experimental/Singleton.h @@ -92,6 +92,7 @@ // should call reenableInstances. #pragma once +#include #include #include #include @@ -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 instance_weak; + // Time we wait on destroy_baton after releasing Singleton shared_ptr. + std::shared_ptr> 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>(); + auto teardown = entry->teardown; + // Can't use make_shared -- no support for a custom deleter, sadly. - auto instance = std::shared_ptr(entry->create(), entry->teardown); + auto instance = std::shared_ptr( + 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 5d43c1be..25b270a6 100644 --- 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::get(&vault); EXPECT_NE(new_s1->serial_number, old_serial); + + auto new_s1_weak = Singleton::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 -- 2.34.1