diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 9fbd78a000..a9ab219051 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -290,8 +290,7 @@ LLSingletonBase::vec_t LLSingletonBase::dep_sort() // SingletonDeps through the life of the program, dynamically adding and // removing LLSingletons as they are created and destroyed, in practice // it's less messy to construct it on demand. The overhead of doing so - // should happen basically twice: once for cleanupAll(), once for - // deleteAll(). + // should happen basically once: for deleteAll(). typedef LLDependencies SingletonDeps; SingletonDeps sdeps; list_t& master(get_master()); @@ -323,35 +322,23 @@ LLSingletonBase::vec_t LLSingletonBase::dep_sort() return ret; } -//static -void LLSingletonBase::cleanupAll() +void LLSingletonBase::cleanup_() { - // It's essential to traverse these in dependency order. - BOOST_FOREACH(LLSingletonBase* sp, dep_sort()) + logdebugs("calling ", + demangle(typeid(*this).name()).c_str(), "::cleanupSingleton()"); + try { - // Call cleanupSingleton() only if we haven't already done so for this - // instance. - if (! sp->mCleaned) - { - sp->mCleaned = true; - - logdebugs("calling ", - demangle(typeid(*sp).name()).c_str(), "::cleanupSingleton()"); - try - { - sp->cleanupSingleton(); - } - catch (const std::exception& e) - { - logwarns("Exception in ", demangle(typeid(*sp).name()).c_str(), - "::cleanupSingleton(): ", e.what()); - } - catch (...) - { - logwarns("Unknown exception in ", demangle(typeid(*sp).name()).c_str(), - "::cleanupSingleton()"); - } - } + cleanupSingleton(); + } + catch (const std::exception& e) + { + logwarns("Exception in ", demangle(typeid(*this).name()).c_str(), + "::cleanupSingleton(): ", e.what()); + } + catch (...) + { + logwarns("Unknown exception in ", demangle(typeid(*this).name()).c_str(), + "::cleanupSingleton()"); } } diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 859e271e26..2b4e3e8895 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -48,7 +48,6 @@ private: typedef std::vector vec_t; static vec_t dep_sort(); - bool mCleaned; // cleanupSingleton() has been called // we directly depend on these other LLSingletons typedef boost::unordered_set set_t; set_t mDepends; @@ -120,6 +119,9 @@ protected: virtual void initSingleton() {} virtual void cleanupSingleton() {} + // internal wrapper around calls to cleanupSingleton() + void cleanup_(); + // deleteSingleton() isn't -- and shouldn't be -- a virtual method. It's a // class static. However, given only Foo*, deleteAll() does need to be // able to reach Foo::deleteSingleton(). Make LLSingleton (which declares @@ -129,32 +131,15 @@ protected: public: /** - * Call this to call the cleanupSingleton() method for every LLSingleton - * constructed since the start of the last cleanupAll() call. (Any - * LLSingleton constructed DURING a cleanupAll() call won't be cleaned up - * until the next cleanupAll() call.) cleanupSingleton() neither deletes - * nor destroys its LLSingleton; therefore it's safe to include logic that - * might take significant realtime or even throw an exception. - * - * The most important property of cleanupAll() is that cleanupSingleton() - * methods are called in dependency order, leaf classes last. Thus, given - * two LLSingleton subclasses A and B, if A's dependency on B is properly - * expressed as a B::getInstance() or B::instance() call during either - * A::A() or A::initSingleton(), B will be cleaned up after A. - * - * If a cleanupSingleton() method throws an exception, the exception is - * logged, but cleanupAll() attempts to continue calling the rest of the - * cleanupSingleton() methods. - */ - static void cleanupAll(); - /** - * Call this to call the deleteSingleton() method for every LLSingleton - * constructed since the start of the last deleteAll() call. (Any - * LLSingleton constructed DURING a deleteAll() call won't be cleaned up - * until the next deleteAll() call.) deleteSingleton() deletes and - * destroys its LLSingleton. Any cleanup logic that might take significant - * realtime -- or throw an exception -- must not be placed in your - * LLSingleton's destructor, but rather in its cleanupSingleton() method. + * deleteAll() calls the cleanupSingleton() and deleteSingleton() methods + * for every LLSingleton constructed since the start of the last + * deleteAll() call. (Any LLSingleton constructed DURING a deleteAll() + * call won't be cleaned up until the next deleteAll() call.) + * deleteSingleton() deletes and destroys its LLSingleton. Any cleanup + * logic that might take significant realtime -- or throw an exception -- + * must not be placed in your LLSingleton's destructor, but rather in its + * cleanupSingleton() method, which is called implicitly by + * deleteSingleton(). * * The most important property of deleteAll() is that deleteSingleton() * methods are called in dependency order, leaf classes last. Thus, given @@ -162,9 +147,9 @@ public: * expressed as a B::getInstance() or B::instance() call during either * A::A() or A::initSingleton(), B will be cleaned up after A. * - * If a deleteSingleton() method throws an exception, the exception is - * logged, but deleteAll() attempts to continue calling the rest of the - * deleteSingleton() methods. + * If a cleanupSingleton() or deleteSingleton() method throws an + * exception, the exception is logged, but deleteAll() attempts to + * continue calling the rest of the deleteSingleton() methods. */ static void deleteAll(); }; @@ -198,7 +183,6 @@ struct LLSingleton_manage_master // Now we can implement LLSingletonBase's template constructor. template LLSingletonBase::LLSingletonBase(tag): - mCleaned(false), mDeleteSingleton(NULL) { // Make this the currently-initializing LLSingleton. @@ -232,10 +216,19 @@ LLSingletonBase::LLSingletonBase(tag): * leading back to yours, move the instance reference from your constructor to * your initSingleton() method. * - * If you override LLSingleton::cleanupSingleton(), your method will be - * called if someone calls LLSingletonBase::cleanupAll(). The significant part - * of this promise is that cleanupAll() will call individual - * cleanupSingleton() methods in reverse dependency order. + * If you override LLSingleton::cleanupSingleton(), your method will + * implicitly be called by LLSingleton::deleteSingleton() just before the + * instance is destroyed. We introduce a special cleanupSingleton() method + * because cleanupSingleton() operations can involve nontrivial realtime, or + * throw an exception. A destructor should do neither! + * + * If your cleanupSingleton() method throws an exception, we log that + * exception but carry on. + * + * If at some point you call LLSingletonBase::deleteAll(), all remaining + * LLSingleton instances will be destroyed in reverse dependency order. (Or + * call MySubclass::deleteSingleton() to specifically destroy the canonical + * MySubclass instance.) * * That is, consider LLSingleton subclasses C, B and A. A depends on B, which * in turn depends on C. These dependencies are expressed as calls to @@ -243,27 +236,15 @@ LLSingletonBase::LLSingletonBase(tag): * It shouldn't matter whether these calls appear in A::A() or * A::initSingleton(), likewise B::B() or B::initSingleton(). * - * We promise that if you later call LLSingletonBase::cleanupAll(): - * 1. A::cleanupSingleton() will be called before - * 2. B::cleanupSingleton(), which will be called before - * 3. C::cleanupSingleton(). + * We promise that if you later call LLSingletonBase::deleteAll(): + * 1. A::deleteSingleton() will be called before + * 2. B::deleteSingleton(), which will be called before + * 3. C::deleteSingleton(). * Put differently, if your LLSingleton subclass constructor or * initSingleton() method explicitly depends on some other LLSingleton * subclass, you may continue to rely on that other subclass in your * cleanupSingleton() method. * - * We introduce a special cleanupSingleton() method because cleanupSingleton() - * operations can involve nontrivial realtime, or might throw an exception. A - * destructor should do neither! - * - * If your cleanupSingleton() method throws an exception, we log that - * exception but proceed with the remaining cleanupSingleton() calls. - * - * Similarly, if at some point you call LLSingletonBase::deleteAll(), all - * remaining LLSingleton instances will be destroyed in dependency order. (Or - * call MySubclass::deleteSingleton() to specifically destroy the canonical - * MySubclass instance.) - * * As currently written, LLSingleton is not thread-safe. */ template @@ -340,31 +321,34 @@ public: } /** - * @brief Immediately delete the singleton. + * @brief Cleanup and destroy the singleton instance. * - * A subsequent call to LLProxy::getInstance() will construct a new + * deleteSingleton() calls this instance's cleanupSingleton() method and + * then destroys the instance. + * + * A subsequent call to LLSingleton::getInstance() will construct a new * instance of the class. * - * Without an explicit call to LLSingletonBase::deleteAll(), LLSingletons - * are implicitly destroyed after main() has exited and the C++ runtime is - * cleaning up statically-constructed objects. Some classes derived from - * LLSingleton have objects that are part of a runtime system that is - * terminated before main() exits. Calling the destructor of those objects - * after the termination of their respective systems can cause crashes and - * other problems during termination of the project. Using this method to - * destroy the singleton early can prevent these crashes. - * - * An example where this is needed is for a LLSingleton that has an APR - * object as a member that makes APR calls on destruction. The APR system is - * shut down explicitly before main() exits. This causes a crash on exit. - * Using this method before the call to apr_terminate() and NOT calling - * getInstance() again will prevent the crash. + * Without an explicit call to LLSingletonBase::deleteAll(), or + * LLSingleton::deleteSingleton(), LLSingleton instances are simply + * leaked. (Allowing implicit destruction at shutdown caused too many + * problems.) */ static void deleteSingleton() { - delete sData.mInstance; + // first call cleanupSingleton() + if (sData.mInstance) + { + sData.mInstance->cleanup_(); + } + // capture the instance and clear SingletonData + auto lameduck = sData.mInstance; sData.mInstance = NULL; sData.mInitState = DELETED; + // Now delete the instance. This sequence guards against the chance + // that the destructor throws, somebody catches it and there's a + // subsequent call to getInstance(). + delete lameduck; } static DERIVED_TYPE* getInstance() diff --git a/indra/llcommon/tests/llsingleton_test.cpp b/indra/llcommon/tests/llsingleton_test.cpp index 56886bc73f..865541be61 100644 --- a/indra/llcommon/tests/llsingleton_test.cpp +++ b/indra/llcommon/tests/llsingleton_test.cpp @@ -142,8 +142,6 @@ namespace tut \ (void)CLS::instance(); \ ensure_equals(sLog, #CLS "i" #CLS); \ - LLSingletonBase::cleanupAll(); \ - ensure_equals(sLog, #CLS "i" #CLS "x" #CLS); \ LLSingletonBase::deleteAll(); \ ensure_equals(sLog, #CLS "i" #CLS "x" #CLS "~" #CLS); \ } \ @@ -158,10 +156,8 @@ namespace tut \ (void)CLS::instance(); \ ensure_equals(sLog, #CLS #OTHER "i" #OTHER "i" #CLS); \ - LLSingletonBase::cleanupAll(); \ - ensure_equals(sLog, #CLS #OTHER "i" #OTHER "i" #CLS "x" #CLS "x" #OTHER); \ LLSingletonBase::deleteAll(); \ - ensure_equals(sLog, #CLS #OTHER "i" #OTHER "i" #CLS "x" #CLS "x" #OTHER "~" #CLS "~" #OTHER); \ + ensure_equals(sLog, #CLS #OTHER "i" #OTHER "i" #CLS "x" #CLS "~" #CLS "x" #OTHER "~" #OTHER); \ } \ \ template<> template<> \ @@ -174,10 +170,8 @@ namespace tut \ (void)CLS::instance(); \ ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER); \ - LLSingletonBase::cleanupAll(); \ - ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "x" #OTHER); \ LLSingletonBase::deleteAll(); \ - ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "x" #OTHER "~" #CLS "~" #OTHER); \ + ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "~" #CLS "x" #OTHER "~" #OTHER); \ } \ \ template<> template<> \ @@ -190,10 +184,8 @@ namespace tut \ (void)CLS::instance(); \ ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER); \ - LLSingletonBase::cleanupAll(); \ - ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "x" #OTHER); \ LLSingletonBase::deleteAll(); \ - ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "x" #OTHER "~" #CLS "~" #OTHER); \ + ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "~" #CLS "x" #OTHER "~" #OTHER); \ } TESTS(A, B, 4, 5, 6, 7) diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index 56e8cc8ff4..398c5eff65 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -2113,25 +2113,19 @@ bool LLAppViewer::cleanup() removeMarkerFiles(); - // It's not at first obvious where, in this long sequence, generic cleanup - // calls OUGHT to go. So let's say this: as we migrate cleanup from + // It's not at first obvious where, in this long sequence, a generic cleanup + // call OUGHT to go. So let's say this: as we migrate cleanup from // explicit hand-placed calls into the generic mechanism, eventually - // all cleanup will get subsumed into the generic calls. So the calls you + // all cleanup will get subsumed into the generic call. So the calls you // still see above are calls that MUST happen before the generic cleanup // kicks in. - // This calls every remaining LLSingleton's cleanupSingleton() method. - // This method should perform any cleanup that might take significant - // realtime, or might throw an exception. - LLSingletonBase::cleanupAll(); - // The logging subsystem depends on an LLSingleton. Any logging after // LLSingletonBase::deleteAll() won't be recorded. LL_INFOS() << "Goodbye!" << LL_ENDL; - // This calls every remaining LLSingleton's deleteSingleton() method. - // No class destructor should perform any cleanup that might take - // significant realtime, or throw an exception. + // This calls every remaining LLSingleton's cleanupSingleton() and + // deleteSingleton() methods. LLSingletonBase::deleteAll(); removeDumpDir();