From d792baf9f7220a374788b35789335a7ae2e62369 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 24 Jun 2015 21:14:55 -0400 Subject: MAINT-5232: Introduce inter-LLSingleton dependency tracking. Introduce LLSingleton::cleanupSingleton() canonical method as the place to put any subclass cleanup logic that might take nontrivial realtime or throw an exception. Neither is appropriate in a destructor. Track all extant LLSingleton subclass instances on a master list, which permits adding LLSingletonBase::cleanupAll() and deleteAll() methods. Also notice when any LLSingleton subclass constructor (or initSingleton() method) calls instance() or getInstance() for another LLSingleton, and capture that other LLSingleton instance as a dependency of the first. This permits cleanupAll() and deleteAll() to perform a dependency sort on the master list, thus cleaning up (or deleting) leaf LLSingletons AFTER the LLSingletons that depend on them. Make C++ runtime's final static destructor call LLSingletonBase::deleteAll() instead of deleting individual LLSingleton instances in arbitrary order. Eliminate "llerror.h" from llsingleton.h, a longstanding TODO. --- indra/llcommon/llsingleton.cpp | 290 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 289 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/llsingleton.cpp') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 9b49e52377..204c0d24d0 100755 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -25,7 +25,295 @@ */ #include "linden_common.h" - #include "llsingleton.h" +#include "llerror.h" +#include "lldependencies.h" +#include +#include +#include +#include + +// Our master list of all LLSingletons is itself an LLSingleton. We used to +// store it in a function-local static, but that could get destroyed before +// the last of the LLSingletons -- and ~LLSingletonBase() definitely wants to +// remove itself from the master list. Since the whole point of this master +// list is to help track inter-LLSingleton dependencies, and since we have +// this implicit dependency from every LLSingleton to the master list, make it +// an LLSingleton. +class LLSingletonBase::MasterList: + public LLSingleton +{ +private: + friend class LLSingleton; + +public: + // No need to make this private with accessors; nobody outside this source + // file can see it. + LLSingletonBase::list_t mList; +}; + +//static +LLSingletonBase::list_t& LLSingletonBase::get_master() +{ + return LLSingletonBase::MasterList::instance().mList; +} + +void LLSingletonBase::add_master() +{ + // As each new LLSingleton is constructed, add to the master list. + get_master().push_back(this); +} + +void LLSingletonBase::remove_master() +{ + // When an LLSingleton is destroyed, remove from master list. + // add_master() used to capture the iterator to the newly-added list item + // so we could directly erase() it from the master list. Unfortunately + // that runs afoul of destruction-dependency order problems. So search the + // master list, and remove this item IF FOUND. We have few enough + // LLSingletons, and they are so rarely destroyed (once per run), that the + // cost of a linear search should not be an issue. + get_master().remove(this); +} + +// Wrapping our initializing list in a static method ensures that it will be +// constructed on demand. This list doesn't also need to be in an LLSingleton +// because (a) it should be empty by program shutdown and (b) none of our +// destructors reference it. +//static +LLSingletonBase::list_t& LLSingletonBase::get_initializing() +{ + static list_t sList; + return sList; +} + +LLSingletonBase::LLSingletonBase(): + mCleaned(false), + mDeleteSingleton(NULL) +{ + // Make this the currently-initializing LLSingleton. + push_initializing(); +} + +LLSingletonBase::~LLSingletonBase() {} + +void LLSingletonBase::push_initializing() +{ + get_initializing().push_back(this); +} + +void LLSingletonBase::pop_initializing() +{ + list_t& list(get_initializing()); + if (list.empty()) + { + LL_ERRS() << "Underflow in stack of currently-initializing LLSingletons at " + << typeid(*this).name() << "::getInstance()" << LL_ENDL; + } + if (list.back() != this) + { + LL_ERRS() << "Push/pop mismatch in stack of currently-initializing LLSingletons: " + << typeid(*this).name() << "::getInstance() trying to pop " + << typeid(*list.back()).name() << LL_ENDL; + } + // Here we're sure that list.back() == this. Whew, pop it. + list.pop_back(); +} + +void LLSingletonBase::capture_dependency() +{ + // Did this getInstance() call come from another LLSingleton, or from + // vanilla application code? Note that although this is a nontrivial + // method, the vast majority of its calls arrive here with initializing + // empty(). + list_t& initializing(get_initializing()); + if (! initializing.empty()) + { + // getInstance() is being called by some other LLSingleton. But -- is + // this a circularity? That is, does 'this' already appear in the + // initializing stack? + // For what it's worth, normally 'initializing' should contain very + // few elements. + list_t::const_iterator found = + std::find(initializing.begin(), initializing.end(), this); + if (found != initializing.end()) + { + // Report the circularity. Requiring the coder to dig through the + // logic to diagnose exactly how we got here is less than helpful. + std::ostringstream out; + for ( ; found != initializing.end(); ++found) + { + // 'found' is an iterator; *found is an LLSingletonBase*; **found + // is the actual LLSingletonBase instance. + out << typeid(**found).name() << " -> "; + } + // DEBUGGING: Initially, make this crump. We want to know how bad + // the problem is before we add it to the long, sad list of + // ominous warnings that everyone always ignores. + LL_ERRS() << "LLSingleton circularity: " << out.str() + << typeid(*this).name() << LL_ENDL; + } + else + { + // Here 'this' is NOT already in the 'initializing' stack. Great! + // Record the dependency. + // initializing.back() is the LLSingletonBase* currently being + // initialized. Store 'this' in its mDepends set. + initializing.back()->mDepends.insert(this); + } + } +} + +//static +LLSingletonBase::vec_t LLSingletonBase::dep_sort() +{ + // While it would theoretically be possible to maintain a static + // 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(). + typedef LLDependencies SingletonDeps; + SingletonDeps sdeps; + list_t& master(get_master()); + BOOST_FOREACH(LLSingletonBase* sp, master) + { + // Build the SingletonDeps structure by adding, for each + // LLSingletonBase* sp in the master list, sp itself. It has no + // associated value type in our SingletonDeps, hence the 0. We don't + // record the LLSingletons it must follow; rather, we record the ones + // it must precede. Copy its mDepends to a KeyList to express that. + sdeps.add(sp, 0, + SingletonDeps::KeyList(), + SingletonDeps::KeyList(sp->mDepends.begin(), sp->mDepends.end())); + } + vec_t ret; + ret.reserve(master.size()); + // We should be able to effect this with a transform_iterator that + // extracts just the first (key) element from each sorted_iterator, then + // uses vec_t's range constructor... but frankly this is more + // straightforward, as long as we remember the above reserve() call! + BOOST_FOREACH(SingletonDeps::sorted_iterator::value_type pair, sdeps.sort()) + { + ret.push_back(pair.first); + } + // The master list is not itself pushed onto the master list. Add it as + // the very last entry -- it is the LLSingleton on which ALL others + // depend! -- so our caller will process it. + ret.push_back(MasterList::getInstance()); + return ret; +} + +//static +void LLSingletonBase::cleanupAll() +{ + // It's essential to traverse these in dependency order. + BOOST_FOREACH(LLSingletonBase* sp, dep_sort()) + { + // Call cleanupSingleton() only if we haven't already done so for this + // instance. + if (! sp->mCleaned) + { + sp->mCleaned = true; + + try + { + sp->cleanupSingleton(); + } + catch (const std::exception& e) + { + LL_WARNS() << "Exception in " << typeid(*sp).name() + << "::cleanupSingleton(): " << e.what() << LL_ENDL; + } + catch (...) + { + LL_WARNS() << "Unknown exception in " << typeid(*sp).name() + << "::cleanupSingleton()" << LL_ENDL; + } + } + } +} + +//static +void LLSingletonBase::deleteAll() +{ + // It's essential to traverse these in dependency order. + BOOST_FOREACH(LLSingletonBase* sp, dep_sort()) + { + // Capture the class name first: in case of exception, don't count on + // being able to extract it later. + const char* name = typeid(*sp).name(); + try + { + // Call static method through instance function pointer. + if (! sp->mDeleteSingleton) + { + // This Should Not Happen... but carry on. + LL_WARNS() << name << "::mDeleteSingleton not initialized!" << LL_ENDL; + } + else + { + // properly initialized: call it. + // From this point on, DO NOT DEREFERENCE sp! + sp->mDeleteSingleton(); + } + } + catch (const std::exception& e) + { + LL_WARNS() << "Exception in " << name + << "::deleteSingleton(): " << e.what() << LL_ENDL; + } + catch (...) + { + LL_WARNS() << "Unknown exception in " << name + << "::deleteSingleton()" << LL_ENDL; + } + } +} + +/*------------------------ Final cleanup management ------------------------*/ +class LLSingletonBase::MasterRefcount +{ +public: + // store a POD int so it will be statically initialized to 0 + int refcount; +}; +static LLSingletonBase::MasterRefcount sMasterRefcount; + +LLSingletonBase::ref_ptr_t LLSingletonBase::get_master_refcount() +{ + // Calling this method constructs a new ref_ptr_t, which implicitly calls + // intrusive_ptr_add_ref(MasterRefcount*). + return &sMasterRefcount; +} + +void intrusive_ptr_add_ref(LLSingletonBase::MasterRefcount* mrc) +{ + // Count outstanding SingletonLifetimeManager instances. + ++mrc->refcount; +} + +void intrusive_ptr_release(LLSingletonBase::MasterRefcount* mrc) +{ + // Notice when each SingletonLifetimeManager instance is destroyed. + if (! --mrc->refcount) + { + // The last instance was destroyed. Time to kill any remaining + // LLSingletons -- but in dependency order. + LLSingletonBase::deleteAll(); + } +} + +/*---------------------------- Logging helpers -----------------------------*/ +//static +void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3) +{ + LL_ERRS() << p1 << p2 << p3 << LL_ENDL; +} +//static +void LLSingletonBase::logwarns(const char* p1, const char* p2, const char* p3) +{ + LL_WARNS() << p1 << p2 << p3 << LL_ENDL; +} -- cgit v1.2.3 From 0ea1b2a164e0d985c80c8a1afea4b31670efc907 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 25 Jun 2015 16:04:01 -0400 Subject: MAINT-5232: Try to avoid circularity between LLError and LLSingleton. Part of LLError's logging infrastructure is implemented with an LLSingleton. Therefore, attempts to log from within LLSingleton machinery could potentially go south if LLError's LLSingleton is not yet initialized. Introduce LLError::is_available() in llerrorcontrol.h and llerror.cpp. Make LLSingletonBase::logwarns() and logerrs() consult LLError::is_available() before attempting to use LL_WARNS or LL_ERRS, respectively. Moreover, make all LLSingleton internal logging use logwarns() and logerrs() instead of directly engaging LL_ERRS or LL_WARNS. --- indra/llcommon/llsingleton.cpp | 64 +++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 20 deletions(-) (limited to 'indra/llcommon/llsingleton.cpp') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 204c0d24d0..a3edf925ad 100755 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -28,9 +28,11 @@ #include "llsingleton.h" #include "llerror.h" +#include "llerrorcontrol.h" // LLError::is_available() #include "lldependencies.h" #include #include +#include // std::cerr in dire emergency #include #include @@ -108,14 +110,14 @@ void LLSingletonBase::pop_initializing() list_t& list(get_initializing()); if (list.empty()) { - LL_ERRS() << "Underflow in stack of currently-initializing LLSingletons at " - << typeid(*this).name() << "::getInstance()" << LL_ENDL; + logerrs("Underflow in stack of currently-initializing LLSingletons at ", + typeid(*this).name(), "::getInstance()"); } if (list.back() != this) { - LL_ERRS() << "Push/pop mismatch in stack of currently-initializing LLSingletons: " - << typeid(*this).name() << "::getInstance() trying to pop " - << typeid(*list.back()).name() << LL_ENDL; + logerrs("Push/pop mismatch in stack of currently-initializing LLSingletons: ", + typeid(*this).name(), "::getInstance() trying to pop ", + typeid(*list.back()).name()); } // Here we're sure that list.back() == this. Whew, pop it. list.pop_back(); @@ -151,8 +153,8 @@ void LLSingletonBase::capture_dependency() // DEBUGGING: Initially, make this crump. We want to know how bad // the problem is before we add it to the long, sad list of // ominous warnings that everyone always ignores. - LL_ERRS() << "LLSingleton circularity: " << out.str() - << typeid(*this).name() << LL_ENDL; + logerrs("LLSingleton circularity: ", out.str().c_str(), + typeid(*this).name()); } else { @@ -223,13 +225,13 @@ void LLSingletonBase::cleanupAll() } catch (const std::exception& e) { - LL_WARNS() << "Exception in " << typeid(*sp).name() - << "::cleanupSingleton(): " << e.what() << LL_ENDL; + logwarns("Exception in ", typeid(*sp).name(), + "::cleanupSingleton(): ", e.what()); } catch (...) { - LL_WARNS() << "Unknown exception in " << typeid(*sp).name() - << "::cleanupSingleton()" << LL_ENDL; + logwarns("Unknown exception in ", typeid(*sp).name(), + "::cleanupSingleton()"); } } } @@ -250,7 +252,7 @@ void LLSingletonBase::deleteAll() if (! sp->mDeleteSingleton) { // This Should Not Happen... but carry on. - LL_WARNS() << name << "::mDeleteSingleton not initialized!" << LL_ENDL; + logwarns(name, "::mDeleteSingleton not initialized!"); } else { @@ -261,13 +263,11 @@ void LLSingletonBase::deleteAll() } catch (const std::exception& e) { - LL_WARNS() << "Exception in " << name - << "::deleteSingleton(): " << e.what() << LL_ENDL; + logwarns("Exception in ", name, "::deleteSingleton(): ", e.what()); } catch (...) { - LL_WARNS() << "Unknown exception in " << name - << "::deleteSingleton()" << LL_ENDL; + logwarns("Unknown exception in ", name, "::deleteSingleton()"); } } } @@ -307,13 +307,37 @@ void intrusive_ptr_release(LLSingletonBase::MasterRefcount* mrc) /*---------------------------- Logging helpers -----------------------------*/ //static -void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3) +void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, const char* p4) { - LL_ERRS() << p1 << p2 << p3 << LL_ENDL; + // Check LLError::is_available() because some of LLError's infrastructure + // is itself an LLSingleton. If that LLSingleton has not yet been + // initialized, trying to log will engage LLSingleton machinery... and + // around and around we go. + if (LLError::is_available()) + { + LL_ERRS() << p1 << p2 << p3 << p4 << LL_ENDL; + } + else + { + // Caller may be a test program, or something else whose stderr is + // visible to the user. + std::cerr << p1 << p2 << p3 << p4 << std::endl; + // The other important side effect of LL_ERRS() is + // https://www.youtube.com/watch?v=OMG7paGJqhQ (emphasis on OMG) + LLError::crashAndLoop(std::string()); + } } //static -void LLSingletonBase::logwarns(const char* p1, const char* p2, const char* p3) +void LLSingletonBase::logwarns(const char* p1, const char* p2, const char* p3, const char* p4) { - LL_WARNS() << p1 << p2 << p3 << LL_ENDL; + // See logerrs() remarks about is_available(). + if (LLError::is_available()) + { + LL_WARNS() << p1 << p2 << p3 << p4 << LL_ENDL; + } + else + { + std::cerr << p1 << p2 << p3 << p4 << std::endl; + } } -- cgit v1.2.3 From 687efd84eabc524e339e61458b0cbf53f9a38f8a Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 26 Jun 2015 13:03:59 -0400 Subject: MAINT-5232: Loosen LLSingleton circularity constraints slightly. LLSingleton explicitly supports circular dependencies: initialization performed during an LLSingleton subclass's initSingleton() method may recursively call that same subclass's getInstance() method. On the other hand, circularity from a subclass constructor cannot be permitted, else getInstance() would have to return a partially-constructed object. Our dependency tracking circularity check initially forbade both. Loosen it to permit references from within initSingleton(). --- indra/llcommon/llsingleton.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) (limited to 'indra/llcommon/llsingleton.cpp') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index a3edf925ad..2813814ae1 100755 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -123,7 +123,7 @@ void LLSingletonBase::pop_initializing() list.pop_back(); } -void LLSingletonBase::capture_dependency() +void LLSingletonBase::capture_dependency(EInitState initState) { // Did this getInstance() call come from another LLSingleton, or from // vanilla application code? Note that although this is a nontrivial @@ -150,11 +150,18 @@ void LLSingletonBase::capture_dependency() // is the actual LLSingletonBase instance. out << typeid(**found).name() << " -> "; } - // DEBUGGING: Initially, make this crump. We want to know how bad - // the problem is before we add it to the long, sad list of - // ominous warnings that everyone always ignores. - logerrs("LLSingleton circularity: ", out.str().c_str(), - typeid(*this).name()); + // We promise to capture dependencies from both the constructor + // and the initSingleton() method, so an LLSingleton's instance + // pointer is on the initializing list during both. Now that we've + // detected circularity, though, we must distinguish the two. If + // the recursive call is from the constructor, we CAN'T honor it: + // otherwise we'd be returning a pointer to a partially- + // constructed object! But from initSingleton() is okay: that + // method exists specifically to support circularity. + // Decide which log helper to call based on initState. They have + // identical signatures. + ((initState == CONSTRUCTING)? logerrs : logwarns) + ("LLSingleton circularity: ", out.str().c_str(), typeid(*this).name(), ""); } else { -- cgit v1.2.3 From 8fee1565eb310081a7f3e26237ddd776c5a9aaaa Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 26 Jun 2015 15:28:57 -0400 Subject: MAINT-5232: Use LLError::Log::demangle() to log LLSingleton classes. --- indra/llcommon/llsingleton.cpp | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) (limited to 'indra/llcommon/llsingleton.cpp') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 2813814ae1..b78110296f 100755 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -111,13 +111,13 @@ void LLSingletonBase::pop_initializing() if (list.empty()) { logerrs("Underflow in stack of currently-initializing LLSingletons at ", - typeid(*this).name(), "::getInstance()"); + demangle(typeid(*this).name()).c_str(), "::getInstance()"); } if (list.back() != this) { logerrs("Push/pop mismatch in stack of currently-initializing LLSingletons: ", - typeid(*this).name(), "::getInstance() trying to pop ", - typeid(*list.back()).name()); + demangle(typeid(*this).name()).c_str(), "::getInstance() trying to pop ", + demangle(typeid(*list.back()).name()).c_str()); } // Here we're sure that list.back() == this. Whew, pop it. list.pop_back(); @@ -148,7 +148,7 @@ void LLSingletonBase::capture_dependency(EInitState initState) { // 'found' is an iterator; *found is an LLSingletonBase*; **found // is the actual LLSingletonBase instance. - out << typeid(**found).name() << " -> "; + out << demangle(typeid(**found).name()) << " -> "; } // We promise to capture dependencies from both the constructor // and the initSingleton() method, so an LLSingleton's instance @@ -161,7 +161,8 @@ void LLSingletonBase::capture_dependency(EInitState initState) // Decide which log helper to call based on initState. They have // identical signatures. ((initState == CONSTRUCTING)? logerrs : logwarns) - ("LLSingleton circularity: ", out.str().c_str(), typeid(*this).name(), ""); + ("LLSingleton circularity: ", out.str().c_str(), + demangle(typeid(*this).name()).c_str(), ""); } else { @@ -232,12 +233,12 @@ void LLSingletonBase::cleanupAll() } catch (const std::exception& e) { - logwarns("Exception in ", typeid(*sp).name(), + logwarns("Exception in ", demangle(typeid(*sp).name()).c_str(), "::cleanupSingleton(): ", e.what()); } catch (...) { - logwarns("Unknown exception in ", typeid(*sp).name(), + logwarns("Unknown exception in ", demangle(typeid(*sp).name()).c_str(), "::cleanupSingleton()"); } } @@ -252,14 +253,14 @@ void LLSingletonBase::deleteAll() { // Capture the class name first: in case of exception, don't count on // being able to extract it later. - const char* name = typeid(*sp).name(); + const std::string name = demangle(typeid(*sp).name()); try { // Call static method through instance function pointer. if (! sp->mDeleteSingleton) { // This Should Not Happen... but carry on. - logwarns(name, "::mDeleteSingleton not initialized!"); + logwarns(name.c_str(), "::mDeleteSingleton not initialized!"); } else { @@ -270,11 +271,11 @@ void LLSingletonBase::deleteAll() } catch (const std::exception& e) { - logwarns("Exception in ", name, "::deleteSingleton(): ", e.what()); + logwarns("Exception in ", name.c_str(), "::deleteSingleton(): ", e.what()); } catch (...) { - logwarns("Unknown exception in ", name, "::deleteSingleton()"); + logwarns("Unknown exception in ", name.c_str(), "::deleteSingleton()"); } } } @@ -348,3 +349,8 @@ void LLSingletonBase::logwarns(const char* p1, const char* p2, const char* p3, c std::cerr << p1 << p2 << p3 << p4 << std::endl; } } + +std::string LLSingletonBase::demangle(const char* mangled) +{ + return LLError::Log::demangle(mangled); +} -- cgit v1.2.3