From: Andrey Obraztsov Date: Thu, 20 Aug 2015 02:18:36 +0000 (-0700) Subject: Replace Singleton::get() with Singleton::try_get and make it obsolete X-Git-Tag: v0.55.0~11 X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=e9a78a1865e8725cca62ea2c2a5b470193a8cdbe Replace Singleton::get() with Singleton::try_get and make it obsolete Summary: BREAKING CHANGE! Deprecate Singleton::get() and replace it with Singleton::try_get() that return smart pointer that allows to manage lifetime of a reference and prevents it from being deleted if the reference is still in use Reviewed By: @chipturner Differential Revision: D2268791 --- diff --git a/folly/Singleton.h b/folly/Singleton.h index 80c9bf2d..da3fd565 100644 --- a/folly/Singleton.h +++ b/folly/Singleton.h @@ -104,6 +104,11 @@ #include +// use this guard to handleSingleton breaking change in 3rd party code +#ifndef FOLLY_SINGLETON_TRY_GET +#define FOLLY_SINGLETON_TRY_GET +#endif + namespace folly { // For actual usage, please see the Singleton class at the bottom @@ -423,7 +428,7 @@ class SingletonVault { // It allows for simple access to registering and instantiating // singletons. Create instances of this class in the global scope of // type Singleton to register your singleton for later access via -// Singleton::get(). +// Singleton::try_get(). template @@ -435,7 +440,7 @@ class Singleton { // Generally your program life cycle should be fine with calling // get() repeatedly rather than saving the reference, and then not // call get() during process shutdown. - static T* get() { + static T* get() __attribute__ ((__deprecated__("Replaced by try_get"))) { return getEntry().get(); } @@ -447,10 +452,20 @@ class Singleton { return getEntry().get_weak(); } - // Allow the Singleton instance to also retrieve the underlying - // singleton, if desired. - T& operator*() { return *get(); } - T* operator->() { return get(); } + // Preferred alternative to get_weak, it returns shared_ptr that can be + // stored; a singleton won't be destroyed unless shared_ptr is destroyed. + // Avoid holding these shared_ptrs beyond the scope of a function; + // don't put them in member variables, always use try_get() instead + static std::shared_ptr try_get() { + auto ret = get_weak().lock(); + if (!ret) { + LOG(DFATAL) << + "folly::Singleton<" << getEntry().type().name() << + ">::get_weak() called on destructed singleton; " + "returning nullptr, possible segfault coming"; + } + return ret; + } explicit Singleton(std::nullptr_t _ = nullptr, typename Singleton::TeardownFunc t = nullptr) : diff --git a/folly/test/SingletonTest.cpp b/folly/test/SingletonTest.cpp index 8505d068..f1847d84 100644 --- a/folly/test/SingletonTest.cpp +++ b/folly/test/SingletonTest.cpp @@ -67,19 +67,24 @@ TEST(Singleton, BasicGlobalUsage) { EXPECT_EQ(Watchdog::creation_order.size(), 0); EXPECT_EQ(SingletonVault::singleton()->registeredSingletonCount(), 1); EXPECT_EQ(SingletonVault::singleton()->livingSingletonCount(), 0); - auto wd1 = Singleton::get(); - EXPECT_NE(wd1, nullptr); - EXPECT_EQ(Watchdog::creation_order.size(), 1); - auto wd2 = Singleton::get(); - EXPECT_NE(wd2, nullptr); - EXPECT_EQ(wd1, wd2); - EXPECT_EQ(Watchdog::creation_order.size(), 1); + + { + std::shared_ptr wd1 = Singleton::try_get(); + EXPECT_NE(wd1, nullptr); + EXPECT_EQ(Watchdog::creation_order.size(), 1); + std::shared_ptr wd2 = Singleton::try_get(); + EXPECT_NE(wd2, nullptr); + EXPECT_EQ(wd1.get(), wd2.get()); + EXPECT_EQ(Watchdog::creation_order.size(), 1); + } + SingletonVault::singleton()->destroyInstances(); EXPECT_EQ(Watchdog::creation_order.size(), 0); } TEST(Singleton, MissingSingleton) { - EXPECT_DEATH([]() { auto u = Singleton::get(); }(), ""); + EXPECT_DEATH([]() { auto u = Singleton::try_get(); }(), + ""); } struct BasicUsageTag {}; @@ -101,20 +106,24 @@ TEST(Singleton, BasicUsage) { vault.registrationComplete(); - Watchdog* s1 = SingletonBasicUsage::get(); - EXPECT_NE(s1, nullptr); + // limit a scope to release references so we can destroy them later + { + std::shared_ptr s1 = SingletonBasicUsage::try_get(); + EXPECT_NE(s1, nullptr); - Watchdog* s2 = SingletonBasicUsage::get(); - EXPECT_NE(s2, nullptr); + std::shared_ptr s2 = SingletonBasicUsage::try_get(); + EXPECT_NE(s2, nullptr); - EXPECT_EQ(s1, s2); + EXPECT_EQ(s1, s2); - auto s3 = SingletonBasicUsage::get(); - EXPECT_NE(s3, nullptr); - EXPECT_NE(s2, s3); + std::shared_ptr s3 = + SingletonBasicUsage::try_get(); + EXPECT_NE(s3, nullptr); + EXPECT_NE(s2, s3); - EXPECT_EQ(vault.registeredSingletonCount(), 2); - EXPECT_EQ(vault.livingSingletonCount(), 2); + EXPECT_EQ(vault.registeredSingletonCount(), 2); + EXPECT_EQ(vault.livingSingletonCount(), 2); + } vault.destroyInstances(); EXPECT_EQ(vault.registeredSingletonCount(), 2); @@ -138,11 +147,10 @@ TEST(Singleton, DirectUsage) { EXPECT_EQ(vault.registeredSingletonCount(), 2); vault.registrationComplete(); - EXPECT_NE(watchdog.get(), nullptr); - EXPECT_EQ(watchdog.get(), SingletonDirectUsage::get()); - EXPECT_NE(watchdog.get(), named_watchdog.get()); - EXPECT_EQ(watchdog->livingWatchdogCount(), 2); - EXPECT_EQ((*watchdog).livingWatchdogCount(), 2); + EXPECT_NE(watchdog.try_get(), nullptr); + EXPECT_EQ(watchdog.try_get(), SingletonDirectUsage::try_get()); + EXPECT_NE(watchdog.try_get(), named_watchdog.try_get()); + EXPECT_EQ(watchdog.try_get()->livingWatchdogCount(), 2); vault.destroyInstances(); } @@ -168,22 +176,23 @@ TEST(Singleton, NamedUsage) { EXPECT_EQ(vault.registeredSingletonCount(), 3); vault.registrationComplete(); - - // Verify our three singletons are distinct and non-nullptr. - Watchdog* s1 = SingletonNamedUsage::get(); - EXPECT_EQ(s1, watchdog1_singleton.get()); - Watchdog* s2 = SingletonNamedUsage::get(); - EXPECT_EQ(s2, watchdog2_singleton.get()); - EXPECT_NE(s1, s2); - Watchdog* s3 = SingletonNamedUsage::get(); - EXPECT_EQ(s3, watchdog3_singleton.get()); - EXPECT_NE(s3, s1); - EXPECT_NE(s3, s2); - - // Verify the "default" singleton is the same as the DefaultTag-tagged - // singleton. - Watchdog* s4 = SingletonNamedUsage::get(); - EXPECT_EQ(s4, watchdog3_singleton.get()); + { + // Verify our three singletons are distinct and non-nullptr. + auto s1 = SingletonNamedUsage::try_get(); + EXPECT_EQ(s1, watchdog1_singleton.try_get()); + auto s2 = SingletonNamedUsage::try_get(); + EXPECT_EQ(s2, watchdog2_singleton.try_get()); + EXPECT_NE(s1, s2); + auto s3 = SingletonNamedUsage::try_get(); + EXPECT_EQ(s3, watchdog3_singleton.try_get()); + EXPECT_NE(s3, s1); + EXPECT_NE(s3, s2); + + // Verify the "default" singleton is the same as the DefaultTag-tagged + // singleton. + auto s4 = SingletonNamedUsage::try_get(); + EXPECT_EQ(s4, watchdog3_singleton.try_get()); + } vault.destroyInstances(); } @@ -203,14 +212,14 @@ TEST(Singleton, NaughtyUsage) { vault.registrationComplete(); // Unregistered. - EXPECT_DEATH(Singleton::get(), ""); - EXPECT_DEATH(SingletonNaughtyUsage::get(), ""); + EXPECT_DEATH(Singleton::try_get(), ""); + EXPECT_DEATH(SingletonNaughtyUsage::try_get(), ""); vault.destroyInstances(); auto& vault2 = *SingletonVault::singleton(); - EXPECT_DEATH(SingletonNaughtyUsage2::get(), ""); + EXPECT_DEATH(SingletonNaughtyUsage2::try_get(), ""); SingletonNaughtyUsage2 watchdog_singleton; // double registration @@ -227,6 +236,7 @@ struct SharedPtrUsageTag {}; template using SingletonSharedPtrUsage = Singleton ; +// TODO (anob): revisit this test TEST(Singleton, SharedPtrUsage) { struct WatchdogHolder { ~WatchdogHolder() { @@ -256,12 +266,12 @@ TEST(Singleton, SharedPtrUsage) { // Initilize holder singleton first, so that it's the last one to be // destroyed. - watchdog_holder_singleton.get(); + watchdog_holder_singleton.try_get(); - Watchdog* s1 = SingletonSharedPtrUsage::get(); + auto s1 = SingletonSharedPtrUsage::try_get().get(); EXPECT_NE(s1, nullptr); - Watchdog* s2 = SingletonSharedPtrUsage::get(); + auto s2 = SingletonSharedPtrUsage::try_get().get(); EXPECT_NE(s2, nullptr); EXPECT_EQ(s1, s2); @@ -283,7 +293,7 @@ TEST(Singleton, SharedPtrUsage) { // We should release externally locked shared_ptr, otherwise it will be // considered a leak - watchdog_holder_singleton->watchdog = std::move(shared_s1); + watchdog_holder_singleton.try_get()->watchdog = std::move(shared_s1); LOG(ERROR) << "The following log message regarding shared_ptr is expected"; { @@ -303,11 +313,13 @@ TEST(Singleton, SharedPtrUsage) { vault.reenableInstances(); - // Singleton should be re-created only after reenableInstances() was called. - Watchdog* new_s1 = SingletonSharedPtrUsage::get(); - // Track serial number rather than pointer since the memory could be - // re-used when we create new_s1. - EXPECT_NE(new_s1->serial_number, old_serial); + { + // Singleton should be re-created only after reenableInstances() was called. + auto new_s1 = SingletonSharedPtrUsage::try_get(); + // Track serial number rather than pointer since the memory could be + // re-used when we create new_s1. + EXPECT_NE(new_s1->serial_number, old_serial); + } auto new_s1_weak = SingletonSharedPtrUsage::get_weak(); auto new_s1_shared = new_s1_weak.lock(); @@ -337,7 +349,7 @@ using SingletonNeedy = Singleton ; struct NeededSingleton {}; struct NeedySingleton { NeedySingleton() { - auto unused = SingletonNeedy::get(); + auto unused = SingletonNeedy::try_get(); EXPECT_NE(unused, nullptr); } }; @@ -349,7 +361,7 @@ using SingletonSelfNeedy = Singleton ; struct SelfNeedySingleton { SelfNeedySingleton() { - auto unused = SingletonSelfNeedy::get(); + auto unused = SingletonSelfNeedy::try_get(); EXPECT_NE(unused, nullptr); } }; @@ -364,14 +376,15 @@ TEST(Singleton, SingletonDependencies) { EXPECT_EQ(needy_vault.registeredSingletonCount(), 2); EXPECT_EQ(needy_vault.livingSingletonCount(), 0); - auto needy = SingletonNeedy::get(); + auto needy = SingletonNeedy::try_get(); EXPECT_EQ(needy_vault.livingSingletonCount(), 2); SingletonSelfNeedy self_needy_singleton; auto& self_needy_vault = *SingletonVault::singleton(); self_needy_vault.registrationComplete(); - EXPECT_DEATH([]() { SingletonSelfNeedy::get(); }(), ""); + EXPECT_DEATH([]() { SingletonSelfNeedy::try_get(); }(), + ""); } // A test to ensure multiple threads contending on singleton creation @@ -396,7 +409,7 @@ TEST(Singleton, SingletonConcurrency) { auto func = [&gatekeeper]() { gatekeeper.lock(); gatekeeper.unlock(); - auto unused = SingletonConcurrency::get(); + auto unused = SingletonConcurrency::try_get(); }; EXPECT_EQ(vault.livingSingletonCount(), 0); @@ -498,13 +511,13 @@ TEST(Singleton, MockTest) { // Registring singletons after registrationComplete called works // with make_mock (but not with Singleton ctor). EXPECT_EQ(vault.registeredSingletonCount(), 1); - int serial_count_first = SingletonMock::get()->serial_number; + int serial_count_first = SingletonMock::try_get()->serial_number; // Override existing mock using make_mock. SingletonMock::make_mock(); EXPECT_EQ(vault.registeredSingletonCount(), 1); - int serial_count_mock = SingletonMock::get()->serial_number; + int serial_count_mock = SingletonMock::try_get()->serial_number; // If serial_count value is the same, then singleton was not replaced. EXPECT_NE(serial_count_first, serial_count_mock); @@ -538,7 +551,7 @@ SingletonBenchmark benchmark_singleton_get_weak; BENCHMARK_RELATIVE(FollySingleton, n) { for (size_t i = 0; i < n; ++i) { - doNotOptimizeAway(SingletonBenchmark::get()); + SingletonBenchmark::try_get(); } } diff --git a/folly/test/SingletonVaultCTest.cpp b/folly/test/SingletonVaultCTest.cpp index 1182278e..2be4114b 100644 --- a/folly/test/SingletonVaultCTest.cpp +++ b/folly/test/SingletonVaultCTest.cpp @@ -48,7 +48,7 @@ TEST(SingletonVault, singletonsAreCreatedAndDestroyed) { auto vault = folly::SingletonVault::singleton(); SingletonTest counter_singleton; SingletonVault_registrationComplete((SingletonVault_t*) vault); - InstanceCounter *counter = SingletonTest::get(); + SingletonTest::try_get(); EXPECT_EQ(instance_counter_instances, 1); SingletonVault_destroyInstances((SingletonVault_t*) vault); EXPECT_EQ(instance_counter_instances, 0);