diff options
| -rw-r--r-- | indra/llcommon/llsingleton.cpp | 71 | ||||
| -rw-r--r-- | indra/llcommon/llsingleton.h | 213 | ||||
| -rw-r--r-- | indra/newview/llimagefiltersmanager.cpp | 2 | ||||
| -rw-r--r-- | indra/test/test.cpp | 4 |
4 files changed, 220 insertions, 70 deletions
diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 9fbd78a000..c45c144570 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -134,12 +134,6 @@ LLSingletonBase::list_t& LLSingletonBase::get_initializing() return LLSingletonBase::MasterList::instance().get_initializing_(); } -//static -LLSingletonBase::list_t& LLSingletonBase::get_initializing_from(MasterList* master) -{ - return master->get_initializing_(); -} - LLSingletonBase::~LLSingletonBase() {} void LLSingletonBase::push_initializing(const char* name) @@ -156,7 +150,7 @@ void LLSingletonBase::pop_initializing() if (list.empty()) { logerrs("Underflow in stack of currently-initializing LLSingletons at ", - demangle(typeid(*this).name()).c_str(), "::getInstance()"); + classname(this).c_str(), "::getInstance()"); } // Now we know list.back() exists: capture it @@ -178,14 +172,39 @@ void LLSingletonBase::pop_initializing() if (back != this) { logerrs("Push/pop mismatch in stack of currently-initializing LLSingletons: ", - demangle(typeid(*this).name()).c_str(), "::getInstance() trying to pop ", - demangle(typeid(*back).name()).c_str()); + classname(this).c_str(), "::getInstance() trying to pop ", + classname(back).c_str()); } // log AFTER popping so logging singletons don't cry circularity log_initializing("Popping", typeid(*back).name()); } +void LLSingletonBase::reset_initializing(list_t::size_type size) +{ + // called for cleanup in case the LLSingleton subclass constructor throws + // an exception + + // The tricky thing about this, the reason we have a separate method + // instead of just calling pop_initializing(), is (hopefully remote) + // possibility that the exception happened *before* the + // push_initializing() call in LLSingletonBase's constructor. So only + // remove the stack top if in fact we've pushed something more than the + // previous size. + list_t& list(get_initializing()); + + while (list.size() > size) + { + list.pop_back(); + } + + // as in pop_initializing() + if (list.empty()) + { + MasterList::instance().cleanup_initializing_(); + } +} + //static void LLSingletonBase::log_initializing(const char* verb, const char* name) { @@ -197,7 +216,7 @@ void LLSingletonBase::log_initializing(const char* verb, const char* name) ri != rend; ++ri) { LLSingletonBase* sb(*ri); - LL_CONT << ' ' << demangle(typeid(*sb).name()); + LL_CONT << ' ' << classname(sb); } LL_ENDL; } @@ -231,7 +250,7 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt // 'found' is an iterator; *found is an LLSingletonBase*; **found // is the actual LLSingletonBase instance. LLSingletonBase* foundp(*found); - out << demangle(typeid(*foundp).name()) << " -> "; + out << classname(foundp) << " -> "; } // We promise to capture dependencies from both the constructor // and the initSingleton() method, so an LLSingleton's instance @@ -245,7 +264,7 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt if (initState == CONSTRUCTING) { logerrs("LLSingleton circularity in Constructor: ", out.str().c_str(), - demangle(typeid(*this).name()).c_str(), ""); + classname(this).c_str(), ""); } else if (it_next == initializing.end()) { @@ -256,14 +275,14 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt // Example: LLNotifications singleton initializes default channels. // Channels register themselves with singleton once done. logdebugs("LLSingleton circularity: ", out.str().c_str(), - demangle(typeid(*this).name()).c_str(), ""); + classname(this).c_str(), ""); } else { // Actual circularity with other singleton (or single singleton is used extensively). // Dependency can be unclear. logwarns("LLSingleton circularity: ", out.str().c_str(), - demangle(typeid(*this).name()).c_str(), ""); + classname(this).c_str(), ""); } } else @@ -276,8 +295,8 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt if (current->mDepends.insert(this).second) { // only log the FIRST time we hit this dependency! - logdebugs(demangle(typeid(*current).name()).c_str(), - " depends on ", demangle(typeid(*this).name()).c_str()); + logdebugs(classname(current).c_str(), + " depends on ", classname(this).c_str()); } } } @@ -336,19 +355,19 @@ void LLSingletonBase::cleanupAll() sp->mCleaned = true; logdebugs("calling ", - demangle(typeid(*sp).name()).c_str(), "::cleanupSingleton()"); + classname(sp).c_str(), "::cleanupSingleton()"); try { sp->cleanupSingleton(); } catch (const std::exception& e) { - logwarns("Exception in ", demangle(typeid(*sp).name()).c_str(), + logwarns("Exception in ", classname(sp).c_str(), "::cleanupSingleton(): ", e.what()); } catch (...) { - logwarns("Unknown exception in ", demangle(typeid(*sp).name()).c_str(), + logwarns("Unknown exception in ", classname(sp).c_str(), "::cleanupSingleton()"); } } @@ -363,7 +382,7 @@ void LLSingletonBase::deleteAll() { // Capture the class name first: in case of exception, don't count on // being able to extract it later. - const std::string name = demangle(typeid(*sp).name()); + const std::string name = classname(sp); try { // Call static method through instance function pointer. @@ -440,7 +459,17 @@ void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, co log(LLError::LEVEL_ERROR, p1, p2, p3, p4); // The other important side effect of LL_ERRS() is // https://www.youtube.com/watch?v=OMG7paGJqhQ (emphasis on OMG) - LLError::crashAndLoop(std::string()); + std::ostringstream out; + out << p1 << p2 << p3 << p4; + auto crash = LLError::getFatalFunction(); + if (crash) + { + crash(out.str()); + } + else + { + LLError::crashAndLoop(out.str()); + } } std::string LLSingletonBase::demangle(const char* mangled) diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 03b7d5a349..0da6d548ab 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -55,7 +55,6 @@ private: // This, on the other hand, is a stack whose top indicates the LLSingleton // currently being initialized. static list_t& get_initializing(); - static list_t& get_initializing_from(MasterList*); // Produce a vector<LLSingletonBase*> of master list, in dependency order. typedef std::vector<LLSingletonBase*> vec_t; static vec_t dep_sort(); @@ -69,10 +68,11 @@ protected: typedef enum e_init_state { UNINITIALIZED = 0, // must be default-initialized state - CONSTRUCTING, - INITIALIZING, - INITIALIZED, - DELETED + CONSTRUCTING, // within DERIVED_TYPE constructor + CONSTRUCTED, // finished DERIVED_TYPE constructor + INITIALIZING, // within DERIVED_TYPE::initSingleton() + INITIALIZED, // normal case + DELETED // deleteSingleton() or deleteAll() called } EInitState; // Define tag<T> to pass to our template constructor. You can't explicitly @@ -112,6 +112,9 @@ protected: // That being the case, we control exactly when it happens -- and we can // pop the stack immediately thereafter. void pop_initializing(); + // Remove 'this' from the init stack in case of exception in the + // LLSingleton subclass constructor. + static void reset_initializing(list_t::size_type size); private: // logging static void log_initializing(const char* verb, const char* name); @@ -127,6 +130,10 @@ protected: static void logwarns(const char* p1, const char* p2="", const char* p3="", const char* p4=""); static std::string demangle(const char* mangled); + template <typename T> + static std::string classname() { return demangle(typeid(T).name()); } + template <typename T> + static std::string classname(T* ptr) { return demangle(typeid(*ptr).name()); } // Default methods in case subclass doesn't declare them. virtual void initSingleton() {} @@ -190,7 +197,15 @@ struct LLSingleton_manage_master void remove(LLSingletonBase* sb) { sb->remove_master(); } void push_initializing(LLSingletonBase* sb) { sb->push_initializing(typeid(T).name()); } void pop_initializing (LLSingletonBase* sb) { sb->pop_initializing(); } - LLSingletonBase::list_t& get_initializing(T*) { return LLSingletonBase::get_initializing(); } + // used for init stack cleanup in case an LLSingleton subclass constructor + // throws an exception + void reset_initializing(LLSingletonBase::list_t::size_type size) + { + LLSingletonBase::reset_initializing(size); + } + // For any LLSingleton subclass except the MasterList, obtain the init + // stack from the MasterList singleton instance. + LLSingletonBase::list_t& get_initializing() { return LLSingletonBase::get_initializing(); } }; // But for the specific case of LLSingletonBase::MasterList, don't. @@ -201,9 +216,14 @@ struct LLSingleton_manage_master<LLSingletonBase::MasterList> void remove(LLSingletonBase*) {} void push_initializing(LLSingletonBase*) {} void pop_initializing (LLSingletonBase*) {} - LLSingletonBase::list_t& get_initializing(LLSingletonBase::MasterList* instance) + // since we never pushed, no need to clean up + void reset_initializing(LLSingletonBase::list_t::size_type size) {} + LLSingletonBase::list_t& get_initializing() { - return LLSingletonBase::get_initializing_from(instance); + // The MasterList shouldn't depend on any other LLSingletons. We'd + // get into trouble if we tried to recursively engage that machinery. + static LLSingletonBase::list_t sDummyList; + return sDummyList; } }; @@ -213,7 +233,12 @@ LLSingletonBase::LLSingletonBase(tag<DERIVED_TYPE>): mCleaned(false), mDeleteSingleton(NULL) { - // Make this the currently-initializing LLSingleton. + // This is the earliest possible point at which we can push this new + // instance onto the init stack. LLSingleton::constructSingleton() can't + // do it before calling the constructor, because it doesn't have an + // instance pointer until the constructor returns. Fortunately this + // constructor is guaranteed to be called before any subclass constructor. + // Make this new instance the currently-initializing LLSingleton. LLSingleton_manage_master<DERIVED_TYPE>().push_initializing(this); } @@ -297,29 +322,82 @@ private: template <typename... Args> static void constructSingleton(Args&&... args) { + auto prev_size = LLSingleton_manage_master<DERIVED_TYPE>().get_initializing().size(); + // getInstance() calls are from within constructor sData.mInitState = CONSTRUCTING; - sData.mInstance = new DERIVED_TYPE(std::forward<Args>(args)...); - sData.mInitState = INITIALIZING; + try + { + sData.mInstance = new DERIVED_TYPE(std::forward<Args>(args)...); + // we have called constructor, have not yet called initSingleton() + sData.mInitState = CONSTRUCTED; + } + catch (const std::exception& err) + { + // LLSingletonBase might -- or might not -- have pushed the new + // instance onto the init stack before the exception. Reset the + // init stack to its previous size BEFORE logging so log-machinery + // LLSingletons don't record a dependency on DERIVED_TYPE! + LLSingleton_manage_master<DERIVED_TYPE>().reset_initializing(prev_size); + logwarns("Error constructing ", classname<DERIVED_TYPE>().c_str(), + ": ", err.what()); + // There isn't a separate EInitState value meaning "we attempted + // to construct this LLSingleton subclass but could not," so use + // DELETED. That seems slightly more appropriate than UNINITIALIZED. + sData.mInitState = DELETED; + // propagate the exception + throw; + } } static void finishInitializing() { - // go ahead and flag ourselves as initialized so we can be - // reentrant during initialization - sData.mInitState = INITIALIZED; - // initialize singleton after constructing it so that it can - // reference other singletons which in turn depend on it, thus - // breaking cyclic dependencies - sData.mInstance->initSingleton(); - // pop this off stack of initializing singletons + // getInstance() calls are from within initSingleton() + sData.mInitState = INITIALIZING; + try + { + // initialize singleton after constructing it so that it can + // reference other singletons which in turn depend on it, thus + // breaking cyclic dependencies + sData.mInstance->initSingleton(); + sData.mInitState = INITIALIZED; + + // pop this off stack of initializing singletons + pop_initializing(); + } + catch (const std::exception& err) + { + // pop this off stack of initializing singletons here, too -- + // BEFORE logging, so log-machinery LLSingletons don't record a + // dependency on DERIVED_TYPE! + pop_initializing(); + logwarns("Error in ", classname<DERIVED_TYPE>().c_str(), + "::initSingleton(): ", err.what()); + // and get rid of the instance entirely + deleteSingleton(); + // propagate the exception + throw; + } + } + + static void pop_initializing() + { + // route through LLSingleton_manage_master so we Do The Right Thing + // (namely, nothing) for MasterList LLSingleton_manage_master<DERIVED_TYPE>().pop_initializing(sData.mInstance); + } - // The remaining top of that stack, if any, is an LLSingleton that - // directly depends on DERIVED_TYPE. If getInstance() was called by - // another LLSingleton, rather than from vanilla application code, - // record the dependency. + // Without this 'using' declaration, the static method we're declaring + // here would hide the base-class method we want it to call. + using LLSingletonBase::capture_dependency; + static void capture_dependency() + { + // By this point, if DERIVED_TYPE was pushed onto the initializing + // stack, it has been popped off. So the top of that stack, if any, is + // an LLSingleton that directly depends on DERIVED_TYPE. If + // getInstance() was called by another LLSingleton, rather than from + // vanilla application code, record the dependency. sData.mInstance->capture_dependency( - LLSingleton_manage_master<DERIVED_TYPE>().get_initializing(sData.mInstance), + LLSingleton_manage_master<DERIVED_TYPE>().get_initializing(), sData.mInitState); } @@ -394,8 +472,7 @@ public: static void deleteSingleton() { delete sData.mInstance; - sData.mInstance = NULL; - sData.mInitState = DELETED; + // SingletonData state handled by destructor, above } static DERIVED_TYPE* getInstance() @@ -408,23 +485,27 @@ public: case UNINITIALIZED: // should never be uninitialized at this point logerrs("Uninitialized singleton ", - demangle(typeid(DERIVED_TYPE).name()).c_str()); + classname<DERIVED_TYPE>().c_str()); return NULL; case CONSTRUCTING: // here if DERIVED_TYPE's constructor (directly or indirectly) // calls DERIVED_TYPE::getInstance() logerrs("Tried to access singleton ", - demangle(typeid(DERIVED_TYPE).name()).c_str(), + classname<DERIVED_TYPE>().c_str(), " from singleton constructor!"); return NULL; - case INITIALIZING: - // first time through: set to INITIALIZING by - // constructSingleton(), called by sInitializer's constructor + case CONSTRUCTED: + // first time through: set to CONSTRUCTED by + // constructSingleton(), called by sInitializer's constructor; + // still have to call initSingleton() finishInitializing(); break; + case INITIALIZING: + // here if DERIVED_TYPE::initSingleton() (directly or indirectly) + // calls DERIVED_TYPE::getInstance(): go ahead and allow it case INITIALIZED: // normal subsequent calls break; @@ -432,13 +513,18 @@ public: case DELETED: // called after deleteSingleton() logwarns("Trying to access deleted singleton ", - demangle(typeid(DERIVED_TYPE).name()).c_str(), + classname<DERIVED_TYPE>().c_str(), " -- creating new instance"); + // This recovery sequence is NOT thread-safe! We would need a + // recursive_mutex a la LLParamSingleton. constructSingleton(); finishInitializing(); break; } + // record the dependency, if any: check if we got here from another + // LLSingleton's constructor or initSingleton() method + capture_dependency(); return sData.mInstance; } @@ -502,6 +588,10 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE> { private: typedef LLSingleton<DERIVED_TYPE> super; + // Use a recursive_mutex in case of constructor circularity. With a + // non-recursive mutex, that would result in deadlock rather than the + // logerrs() call in getInstance(). + typedef std::recursive_mutex mutex_t; public: using super::deleteSingleton; @@ -515,12 +605,12 @@ public: // In case racing threads both call initParamSingleton() at the same // time, serialize them. One should initialize; the other should see // mInitState already set. - std::unique_lock<decltype(mMutex)> lk(mMutex); + std::unique_lock<mutex_t> lk(getMutex()); // For organizational purposes this function shouldn't be called twice if (super::sData.mInitState != super::UNINITIALIZED) { super::logerrs("Tried to initialize singleton ", - super::demangle(typeid(DERIVED_TYPE).name()).c_str(), + super::template classname<DERIVED_TYPE>().c_str(), " twice!"); } else @@ -534,28 +624,44 @@ public: { // In case racing threads call getInstance() at the same moment as // initParamSingleton(), serialize the calls. - std::unique_lock<decltype(mMutex)> lk(mMutex); + std::unique_lock<mutex_t> lk(getMutex()); switch (super::sData.mInitState) { case super::UNINITIALIZED: super::logerrs("Uninitialized param singleton ", - super::demangle(typeid(DERIVED_TYPE).name()).c_str()); + super::template classname<DERIVED_TYPE>().c_str()); break; case super::CONSTRUCTING: super::logerrs("Tried to access param singleton ", - super::demangle(typeid(DERIVED_TYPE).name()).c_str(), + super::template classname<DERIVED_TYPE>().c_str(), " from singleton constructor!"); break; + case super::CONSTRUCTED: + // Should never happen!? The CONSTRUCTED state is specifically to + // navigate through LLSingleton::SingletonInitializer getting + // constructed (once) before LLSingleton::getInstance()'s switch + // on mInitState. But our initParamSingleton() method calls + // constructSingleton() and then calls finishInitializing(), which + // immediately sets INITIALIZING. Why are we here? + super::logerrs("Param singleton ", + super::template classname<DERIVED_TYPE>().c_str(), + "::initSingleton() not yet called"); + break; + case super::INITIALIZING: + // As with LLSingleton, explicitly permit circular calls from + // within initSingleton() case super::INITIALIZED: + // for any valid call, capture dependencies + super::capture_dependency(); return super::sData.mInstance; case super::DELETED: super::logerrs("Trying to access deleted param singleton ", - super::demangle(typeid(DERIVED_TYPE).name()).c_str()); + super::template classname<DERIVED_TYPE>().c_str()); break; } @@ -573,15 +679,30 @@ public: } private: - // Use a recursive_mutex in case of constructor circularity. With a - // non-recursive mutex, that would result in deadlock rather than the - // logerrs() call coded above. - static std::recursive_mutex mMutex; + // sMutex must be a function-local static rather than a static member. One + // of the essential features of LLSingleton and friends is that they must + // support getInstance() even when the containing module's static + // variables have not yet been runtime-initialized. A mutex requires + // construction. A static class member might not yet have been + // constructed. + // + // We could store a dumb mutex_t*, notice when it's NULL and allocate a + // heap mutex -- but that's vulnerable to race conditions. And we can't + // defend the dumb pointer with another mutex. + // + // We could store a std::atomic<mutex_t*> -- but a default-constructed + // std::atomic<T> does not contain a valid T, even a default-constructed + // T! Which means std::atomic, too, requires runtime initialization. + // + // But a function-local static is guaranteed to be initialized exactly + // once, the first time control reaches that declaration. + static mutex_t& getMutex() + { + static mutex_t sMutex; + return sMutex; + } }; -template<typename T> -typename std::recursive_mutex LLParamSingleton<T>::mMutex; - /** * Initialization locked singleton, only derived class can decide when to initialize. * Starts locked. @@ -634,7 +755,7 @@ public: * file, use 'inline' (unless it's a template class) to avoid duplicate-symbol * errors at link time. */ -#define LLSINGLETON(DERIVED_CLASS, ...) \ +#define LLSINGLETON(DERIVED_CLASS, ...) \ private: \ /* implement LLSingleton pure virtual method whose sole purpose */ \ /* is to remind people to use this macro */ \ diff --git a/indra/newview/llimagefiltersmanager.cpp b/indra/newview/llimagefiltersmanager.cpp index ee6b39efac..c23cdc8103 100644 --- a/indra/newview/llimagefiltersmanager.cpp +++ b/indra/newview/llimagefiltersmanager.cpp @@ -48,7 +48,7 @@ LLImageFiltersManager::~LLImageFiltersManager() { } -// virtual static +// virtual void LLImageFiltersManager::initSingleton() { loadAllFilters(); diff --git a/indra/test/test.cpp b/indra/test/test.cpp index d4cd4b951e..b14c2eb255 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -253,7 +253,7 @@ public: break; case tut::test_result::ex: ++mFailedTests; - out << "exception"; + out << "exception: " << tr.exception_typeid; break; case tut::test_result::warn: ++mFailedTests; @@ -264,7 +264,7 @@ public: out << "abnormal termination"; break; case tut::test_result::skip: - ++mSkippedTests; + ++mSkippedTests; out << "skipped known failure"; break; default: |