From b22f89c9fa9e6ee95b552b27808df77f710caad6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 23 Nov 2019 22:18:45 -0500 Subject: DRTVWR-494: Improve thread safety of LLSingleton machinery. Remove warnings about LLSingleton not being thread-safe because, at this point, we have devoted considerable effort to trying to make it thread-safe. Add LLSingleton::Locker, a nested class which both provides a function- static mutex and a scoped lock that uses it. Instantiating Locker, which has a nullary constructor, replaces the somewhat cumbersome idiom of declaring a std::unique_lock lk(getMutex); This eliminates (or rather, absorbs) the typedefs and getMutex() method from LLParamSingleton. Replace explicit std::unique_lock declarations in LLParamSingleton methods with Locker declarations. Remove LLSingleton::SingletonInitializer nested struct. Instead of getInstance() relying on function-static initialization to protect (only) constructSingleton() calls, explicitly use a Locker instance to cover its whole scope, and make the UNINITIALIZED case call constructSingleton(). Rearrange cases so that after constructSingleton(), control falls through to the CONSTRUCTED case and the finishInitializing() call. Use a Locker instance in other public-facing methods too: instanceExists(), wasDeleted(), ~LLSingleton(). Make destructor protected so it can only be called via deleteSingleton() (but must be accessible to subclasses for overrides). Remove LLSingletonBase::get_master() and get_initializing(), which permitted directly manipulating the master list and the initializing stack without any locking mechanism. Replace with get_initializing_size(). Similarly, replace LLSingleton_manage_master::get_initializing() with get_initializing_size(). Use in constructSingleton() in place of get_initializing().size(). Remove LLSingletonBase::capture_dependency()'s list_t parameter, which accepted the list returned by get_initializing(). Encapsulate that retrieval within the scope of the new lock in capture_dependency(). Add LLSingleton_manage_master::capture_dependency(LLSingletonBase*, EInitState) to forward (or not) a call to LLSingletonBase::capture_dependency(). Nullary LLSingleton::capture_dependency() calls new LLSingleton_manage_master method. Equip LLSingletonBase::MasterList with a mutex of its own, separate from the one donated by the LLSingleton machinery, to serialize use of MasterList data members. Introduce MasterList::Lock nested class to lock the MasterList mutex while providing a reference to the MasterList instance. Introduce subclasses LockedMaster, which provides a reference to the actual mMaster master list while holding the MasterList lock; and LockedInitializing, which does the same for the initializing list. Make mMaster and get_initializing_() private so that consuming code can *only* access those lists via LockedInitializing and LockedMaster. Make MasterList::cleanup_initializing_() private, with a LockedInitializing public forwarding method. This avoids another call to MasterList::instance(), and also mandates that the lock is currently held during every call. Similarly, move LLSingletonBase::log_initializing() to a LockedInitializing log() method. (transplanted from dca0f16266c7bddedb51ae7d7dca468ba87060d5) --- indra/llcommon/llsingleton.h | 164 +++++++++++++++++++++---------------------- 1 file changed, 80 insertions(+), 84 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 0da6d548ab..8dec8bfb3b 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -51,10 +51,9 @@ public: private: // All existing LLSingleton instances are tracked in this master list. typedef std::list list_t; - static list_t& get_master(); - // This, on the other hand, is a stack whose top indicates the LLSingleton - // currently being initialized. - static list_t& get_initializing(); + // Size of stack whose top indicates the LLSingleton currently being + // initialized. + static list_t::size_type get_initializing_size(); // Produce a vector of master list, in dependency order. typedef std::vector vec_t; static vec_t dep_sort(); @@ -115,13 +114,10 @@ protected: // 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); protected: // If a given call to B::getInstance() happens during either A::A() or // A::initSingleton(), record that A directly depends on B. - void capture_dependency(list_t& initializing, EInitState); + void capture_dependency(EInitState); // delegate LL_ERRS() logging to llsingleton.cpp static void logerrs(const char* p1, const char* p2="", @@ -203,9 +199,16 @@ struct LLSingleton_manage_master { 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(); } + // For any LLSingleton subclass except the MasterList, obtain the size of + // the init stack from the MasterList singleton instance. + LLSingletonBase::list_t::size_type get_initializing_size() + { + return LLSingletonBase::get_initializing_size(); + } + void capture_dependency(LLSingletonBase* sb, LLSingletonBase::EInitState state) + { + sb->capture_dependency(state); + } }; // But for the specific case of LLSingletonBase::MasterList, don't. @@ -218,13 +221,8 @@ struct LLSingleton_manage_master void pop_initializing (LLSingletonBase*) {} // since we never pushed, no need to clean up void reset_initializing(LLSingletonBase::list_t::size_type size) {} - LLSingletonBase::list_t& get_initializing() - { - // 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; - } + LLSingletonBase::list_t::size_type get_initializing_size() { return 0; } + void capture_dependency(LLSingletonBase*, LLSingletonBase::EInitState) {} }; // Now we can implement LLSingletonBase's template constructor. @@ -304,8 +302,6 @@ class LLParamSingleton; * 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 class LLSingleton : public LLSingletonBase @@ -315,6 +311,47 @@ private: // access our private members. friend class LLParamSingleton; + // Scoped lock on the mutex associated with this LLSingleton + class Locker + { + public: + Locker(): mLock(getMutex()) {} + + private: + // Use a recursive_mutex in case of constructor circularity. With a + // non-recursive mutex, that would result in deadlock. + typedef std::recursive_mutex mutex_t; + + // LLSingleton must have a distinct instance of sMutex for every + // distinct T. It's tempting to consider hoisting Locker up into + // LLSingletonBase. Don't do it. + // + // 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 -- but a default-constructed + // std::atomic 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; + } + + std::unique_lock mLock; + }; + // LLSingleton only supports a nullary constructor. However, the specific // purpose for its subclass LLParamSingleton is to support Singletons // requiring constructor arguments. constructSingleton() supports both use @@ -322,7 +359,7 @@ private: template static void constructSingleton(Args&&... args) { - auto prev_size = LLSingleton_manage_master().get_initializing().size(); + auto prev_size = LLSingleton_manage_master().get_initializing_size(); // getInstance() calls are from within constructor sData.mInitState = CONSTRUCTING; try @@ -386,9 +423,6 @@ private: LLSingleton_manage_master().pop_initializing(sData.mInstance); } - // 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 @@ -396,9 +430,8 @@ private: // 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().get_initializing(), - sData.mInitState); + LLSingleton_manage_master().capture_dependency( + sData.mInstance, sData.mInitState); } // We know of no way to instruct the compiler that every subclass @@ -411,20 +444,6 @@ private: // subclass body. virtual void you_must_use_LLSINGLETON_macro() = 0; - // The purpose of this struct is to engage the C++11 guarantee that static - // variables declared in function scope are initialized exactly once, even - // if multiple threads concurrently reach the same declaration. - // https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables - // Since getInstance() declares a static instance of SingletonInitializer, - // only the first call to getInstance() calls constructSingleton(). - struct SingletonInitializer - { - SingletonInitializer() - { - constructSingleton(); - } - }; - protected: // Pass DERIVED_TYPE explicitly to LLSingletonBase's constructor because, // until our subclass constructor completes, *this isn't yet a @@ -439,15 +458,20 @@ protected: LLSingleton_manage_master().add(this); } -public: +protected: virtual ~LLSingleton() { + // In case racing threads call getInstance() at the same moment as + // this destructor, serialize the calls. + Locker lk; + // remove this instance from the master list LLSingleton_manage_master().remove(this); sData.mInstance = NULL; sData.mInitState = DELETED; } +public: /** * @brief Immediately delete the singleton. * @@ -477,17 +501,12 @@ public: static DERIVED_TYPE* getInstance() { - // call constructSingleton() only the first time we get here - static SingletonInitializer sInitializer; + // In case racing threads call getInstance() at the same moment, + // serialize the calls. + Locker lk; switch (sData.mInitState) { - case UNINITIALIZED: - // should never be uninitialized at this point - logerrs("Uninitialized singleton ", - classname().c_str()); - return NULL; - case CONSTRUCTING: // here if DERIVED_TYPE's constructor (directly or indirectly) // calls DERIVED_TYPE::getInstance() @@ -496,9 +515,11 @@ public: " from singleton constructor!"); return NULL; + case UNINITIALIZED: + constructSingleton(); + // fall through... + case CONSTRUCTED: - // first time through: set to CONSTRUCTED by - // constructSingleton(), called by sInitializer's constructor; // still have to call initSingleton() finishInitializing(); break; @@ -515,8 +536,6 @@ public: logwarns("Trying to access deleted singleton ", classname().c_str(), " -- creating new instance"); - // This recovery sequence is NOT thread-safe! We would need a - // recursive_mutex a la LLParamSingleton. constructSingleton(); finishInitializing(); break; @@ -539,6 +558,8 @@ public: // Use this to avoid accessing singletons before they can safely be constructed. static bool instanceExists() { + // defend any access to sData from racing threads + Locker lk; return sData.mInitState == INITIALIZED; } @@ -547,6 +568,8 @@ public: // cleaned up. static bool wasDeleted() { + // defend any access to sData from racing threads + Locker lk; return sData.mInitState == DELETED; } @@ -588,10 +611,7 @@ class LLParamSingleton : public LLSingleton { private: typedef LLSingleton 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; + using typename super::Locker; public: using super::deleteSingleton; @@ -605,7 +625,7 @@ 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 lk(getMutex()); + Locker lk; // For organizational purposes this function shouldn't be called twice if (super::sData.mInitState != super::UNINITIALIZED) { @@ -624,7 +644,7 @@ public: { // In case racing threads call getInstance() at the same moment as // initParamSingleton(), serialize the calls. - std::unique_lock lk(getMutex()); + Locker lk; switch (super::sData.mInitState) { @@ -677,30 +697,6 @@ public: { return *getInstance(); } - -private: - // 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 -- but a default-constructed - // std::atomic 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; - } }; /** -- cgit v1.2.3 From b080b06b422db6405982bee603118ee68e6c2500 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 3 Dec 2019 11:45:14 -0500 Subject: DRTVWR-494: Encapsulate redundant VS boilerplate around . --- indra/llcommon/llsingleton.h | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 8dec8bfb3b..4efffde43a 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -30,18 +30,7 @@ #include #include #include - -#if LL_WINDOWS -#pragma warning (push) -#pragma warning (disable:4265) -#endif -// warning C4265: 'std::_Pad' : class has virtual functions, but destructor is not virtual - -#include - -#if LL_WINDOWS -#pragma warning (pop) -#endif +#include "mutex.h" class LLSingletonBase: private boost::noncopyable { -- cgit v1.2.3 From 794072c1415e986b95cab65f8217857263d7468a Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 3 Dec 2019 12:49:18 -0500 Subject: DRTVWR-494: Streamline LLSingleton state machine. The CONSTRUCTED state was only briefly set between constructSingleton() and finishInitializing(). But as no consumer code is executed between setting CONSTRUCTED and setting INITIALIZING, it was impossible to reach the switch statement in either getInstance() method in state CONSTRUCTED. So there was no point in state CONSTRUCTED. Remove it. With CONSTRUCTED gone, we only ever call finishInitializing() right after constructSingleton(). Merge finishInitializing() into constructSingleton(). --- indra/llcommon/llsingleton.h | 24 ------------------------ 1 file changed, 24 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 4efffde43a..ebae601029 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -57,7 +57,6 @@ protected: { UNINITIALIZED = 0, // must be default-initialized state CONSTRUCTING, // within DERIVED_TYPE constructor - CONSTRUCTED, // finished DERIVED_TYPE constructor INITIALIZING, // within DERIVED_TYPE::initSingleton() INITIALIZED, // normal case DELETED // deleteSingleton() or deleteAll() called @@ -355,7 +354,6 @@ private: { sData.mInstance = new DERIVED_TYPE(std::forward(args)...); // we have called constructor, have not yet called initSingleton() - sData.mInitState = CONSTRUCTED; } catch (const std::exception& err) { @@ -373,10 +371,7 @@ private: // propagate the exception throw; } - } - static void finishInitializing() - { // getInstance() calls are from within initSingleton() sData.mInitState = INITIALIZING; try @@ -506,11 +501,6 @@ public: case UNINITIALIZED: constructSingleton(); - // fall through... - - case CONSTRUCTED: - // still have to call initSingleton() - finishInitializing(); break; case INITIALIZING: @@ -526,7 +516,6 @@ public: classname().c_str(), " -- creating new instance"); constructSingleton(); - finishInitializing(); break; } @@ -625,7 +614,6 @@ public: else { super::constructSingleton(std::forward(args)...); - super::finishInitializing(); } } @@ -648,18 +636,6 @@ public: " 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().c_str(), - "::initSingleton() not yet called"); - break; - case super::INITIALIZING: // As with LLSingleton, explicitly permit circular calls from // within initSingleton() -- cgit v1.2.3 From a6f5e55d42f417ea8bc565e473354b64c192802d Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 12 Dec 2019 16:14:38 -0500 Subject: DRTVWR-494: Dispatch all LLSingleton construction to the main thread. Given the viewer's mutually-dependent LLSingletons, given that different threads might simultaneously request different LLSingletons from such a chain of circular dependencies, the key to avoiding deadlock is to serialize all LLSingleton construction on one thread: the main thread. Add comments to LLSingleton::getInstance() explaining the problem and the solution. Recast LLSingleton's static SingletonData to use LockStatic. Instead of using Locker, and simply trusting that every reference to sData is within the dynamic scope of a Locker instance, LockStatic enforces that: you can only access SingletonData members via LockStatic. Reorganize the switch in getInstance() to group the CONSTRUCTING error, the INITIALIZING/INITIALIZED success case, and the DELETED/UNINITIALIZED construction case. When [re]constructing an instance, on the main thread, retain the lock and call constructSingleton() (and capture_dependency()) directly. On a secondary thread, unlock LockStatic and use LLMainThreadTask::dispatch() to call getInstance() on the main thread. Since we might end up enqueuing multiple such tasks, it's important to let getInstance() notice when the instance has already been constructed and simply return the existing pointer. Add loginfos() method, sibling to logerrs(), logwarns() and logdebugs(). Produce loginfos() messages when dispatching to the main thread, when actually running on the main thread and when resuming the suspended requesting thread. Make deleteSingleton() manage all associated state, instead of delegating some of that work to ~LLSingleton(). Now, within LockStatic, extract the instance pointer and set state to DELETED; that lets subsequent code, which retains the only remaining pointer to the instance, remove the master-list entry, call the subclass cleanupSingleton() and destructor without needing to hold the lock. In fact, entirely remove ~LLSingleton(). Import LLSingletonBase::cleanup_() method to wrap the call to subclass cleanupSingleton() in try/catch. Remove cleanupAll() calls from llsingleton_test.cpp, and reorder the success cases to reflect the fact that T::cleanupSingleton() is called immediately before ~T() for each distinct LLSingleton subclass T. When getInstance() on a secondary thread dispatches to the main thread, it necessarily unlocks its LockStatic lock. But an LLSingleton dependency chain strongly depends on the function stack on which getInstance() is invoked -- the task dispatched to the main thread doesn't know the dependencies tracked on the requesting thread stack. So, once the main thread delivers the instance pointer, the requesting thread captures its own dependencies for that instance. Back in the requesting thread, obtaining the current EInitState to pass to capture_dependencies() would have required relocking LockStatic. Instead, I've convinced myself that (a) capture_dependencies() only wanted to know EInitState to produce an error for CONSTRUCTING, and (b) in CONSTRUCTING state, we never get as far as capture_dependencies() because getInstance() produces an error first. Eliminate the EInitState parameter from all capture_dependencies() methods. Remove the LLSingletonBase::capture_dependency() stanza that tested EInitState. Make the capture_dependencies() variants that accepted LockStatic instead accept LLSingletonBase*. That lets getInstance(), in the LLMainThreadTask case, pass the newly-returned instance pointer. For symmetry, make pop_initializing() accept LLSingletonBase* as well, instead of accepting LockStatic and extracting mInstance. --- indra/llcommon/llsingleton.h | 334 +++++++++++++++++++++++++------------------ 1 file changed, 194 insertions(+), 140 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index ebae601029..4f3b8ceb38 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -31,6 +31,9 @@ #include #include #include "mutex.h" +#include "lockstatic.h" +#include "llthread.h" // on_main_thread() +#include "llmainthreadtask.h" class LLSingletonBase: private boost::noncopyable { @@ -105,7 +108,7 @@ protected: protected: // If a given call to B::getInstance() happens during either A::A() or // A::initSingleton(), record that A directly depends on B. - void capture_dependency(EInitState); + void capture_dependency(); // delegate LL_ERRS() logging to llsingleton.cpp static void logerrs(const char* p1, const char* p2="", @@ -113,6 +116,9 @@ protected: // delegate LL_WARNS() logging to llsingleton.cpp static void logwarns(const char* p1, const char* p2="", const char* p3="", const char* p4=""); + // delegate LL_INFOS() logging to llsingleton.cpp + static void loginfos(const char* p1, const char* p2="", + const char* p3="", const char* p4=""); static std::string demangle(const char* mangled); template static std::string classname() { return demangle(typeid(T).name()); } @@ -123,6 +129,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 @@ -193,9 +202,9 @@ struct LLSingleton_manage_master { return LLSingletonBase::get_initializing_size(); } - void capture_dependency(LLSingletonBase* sb, LLSingletonBase::EInitState state) + void capture_dependency(LLSingletonBase* sb) { - sb->capture_dependency(state); + sb->capture_dependency(); } }; @@ -210,14 +219,14 @@ struct LLSingleton_manage_master // since we never pushed, no need to clean up void reset_initializing(LLSingletonBase::list_t::size_type size) {} LLSingletonBase::list_t::size_type get_initializing_size() { return 0; } - void capture_dependency(LLSingletonBase*, LLSingletonBase::EInitState) {} + void capture_dependency(LLSingletonBase*) {} }; // Now we can implement LLSingletonBase's template constructor. template LLSingletonBase::LLSingletonBase(tag): mCleaned(false), - mDeleteSingleton(NULL) + mDeleteSingleton(nullptr) { // This is the earliest possible point at which we can push this new // instance onto the init stack. LLSingleton::constructSingleton() can't @@ -295,65 +304,40 @@ template class LLSingleton : public LLSingletonBase { private: - // Allow LLParamSingleton subclass -- but NOT DERIVED_TYPE itself -- to - // access our private members. - friend class LLParamSingleton; - - // Scoped lock on the mutex associated with this LLSingleton - class Locker + // LLSingleton must have a distinct instance of + // SingletonData for every distinct DERIVED_TYPE. It's tempting to + // consider hoisting SingletonData up into LLSingletonBase. Don't do it. + struct SingletonData { - public: - Locker(): mLock(getMutex()) {} - - private: // Use a recursive_mutex in case of constructor circularity. With a // non-recursive mutex, that would result in deadlock. typedef std::recursive_mutex mutex_t; + mutex_t mMutex; // LockStatic looks for mMutex - // LLSingleton must have a distinct instance of sMutex for every - // distinct T. It's tempting to consider hoisting Locker up into - // LLSingletonBase. Don't do it. - // - // 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 -- but a default-constructed - // std::atomic 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; - } - - std::unique_lock mLock; + EInitState mInitState{UNINITIALIZED}; + DERIVED_TYPE* mInstance{nullptr}; }; + typedef llthread::LockStatic LockStatic; + + // Allow LLParamSingleton subclass -- but NOT DERIVED_TYPE itself -- to + // access our private members. + friend class LLParamSingleton; // LLSingleton only supports a nullary constructor. However, the specific // purpose for its subclass LLParamSingleton is to support Singletons // requiring constructor arguments. constructSingleton() supports both use // cases. + // Accepting LockStatic& requires that the caller has already locked our + // static data before calling. template - static void constructSingleton(Args&&... args) + static void constructSingleton(LockStatic& lk, Args&&... args) { auto prev_size = LLSingleton_manage_master().get_initializing_size(); - // getInstance() calls are from within constructor - sData.mInitState = CONSTRUCTING; + // Any getInstance() calls after this point are from within constructor + lk->mInitState = CONSTRUCTING; try { - sData.mInstance = new DERIVED_TYPE(std::forward(args)...); - // we have called constructor, have not yet called initSingleton() + lk->mInstance = new DERIVED_TYPE(std::forward(args)...); } catch (const std::exception& err) { @@ -367,55 +351,56 @@ private: // 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; + lk->mInitState = DELETED; // propagate the exception throw; } - // getInstance() calls are from within initSingleton() - sData.mInitState = INITIALIZING; + // Any getInstance() calls after this point are from within initSingleton() + lk->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; + lk->mInstance->initSingleton(); + lk->mInitState = INITIALIZED; // pop this off stack of initializing singletons - pop_initializing(); + pop_initializing(lk->mInstance); } 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(); + pop_initializing(lk->mInstance); logwarns("Error in ", classname().c_str(), "::initSingleton(): ", err.what()); - // and get rid of the instance entirely + // Get rid of the instance entirely. This call depends on our + // recursive_mutex. We could have a deleteSingleton(LockStatic&) + // overload and pass lk, but we don't strictly need it. deleteSingleton(); // propagate the exception throw; } } - static void pop_initializing() + static void pop_initializing(LLSingletonBase* sb) { // route through LLSingleton_manage_master so we Do The Right Thing // (namely, nothing) for MasterList - LLSingleton_manage_master().pop_initializing(sData.mInstance); + LLSingleton_manage_master().pop_initializing(sb); } - static void capture_dependency() + static void capture_dependency(LLSingletonBase* sb) { // 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. - LLSingleton_manage_master().capture_dependency( - sData.mInstance, sData.mInitState); + LLSingleton_manage_master().capture_dependency(sb); } // We know of no way to instruct the compiler that every subclass @@ -442,19 +427,6 @@ protected: LLSingleton_manage_master().add(this); } -protected: - virtual ~LLSingleton() - { - // In case racing threads call getInstance() at the same moment as - // this destructor, serialize the calls. - Locker lk; - - // remove this instance from the master list - LLSingleton_manage_master().remove(this); - sData.mInstance = NULL; - sData.mInitState = DELETED; - } - public: /** * @brief Immediately delete the singleton. @@ -479,54 +451,149 @@ public: */ static void deleteSingleton() { - delete sData.mInstance; - // SingletonData state handled by destructor, above + DERIVED_TYPE* lameduck; + { + LockStatic lk; + // Capture the instance and clear SingletonData. This sequence + // guards against the chance that the destructor throws, somebody + // catches it and there's a subsequent call to getInstance(). + lameduck = lk->mInstance; + lk->mInstance = nullptr; + lk->mInitState = DELETED; + // At this point we can safely unlock SingletonData during the + // remaining cleanup. If another thread calls deleteSingleton() (or + // getInstance(), or whatever) it won't find our instance, now + // referenced only as 'lameduck'. + } + // of course, only cleanup and delete if there's something there + if (lameduck) + { + // remove this instance from the master list BEFORE attempting + // cleanup so possible destructor exception won't leave the master + // list confused + LLSingleton_manage_master().remove(lameduck); + lameduck->cleanup_(); + delete lameduck; + } } static DERIVED_TYPE* getInstance() { - // In case racing threads call getInstance() at the same moment, - // serialize the calls. - Locker lk; - - switch (sData.mInitState) - { - case CONSTRUCTING: - // here if DERIVED_TYPE's constructor (directly or indirectly) - // calls DERIVED_TYPE::getInstance() - logerrs("Tried to access singleton ", - classname().c_str(), - " from singleton constructor!"); - return NULL; - - case UNINITIALIZED: - constructSingleton(); - 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; - - case DELETED: - // called after deleteSingleton() - logwarns("Trying to access deleted singleton ", - classname().c_str(), - " -- creating new instance"); - constructSingleton(); - break; - } - - // record the dependency, if any: check if we got here from another - // LLSingleton's constructor or initSingleton() method - capture_dependency(); - return sData.mInstance; + // We know the viewer has LLSingleton dependency circularities. If you + // feel strongly motivated to eliminate them, cheers and good luck. + // (At that point we could consider a much simpler locking mechanism.) + + // If A and B depend on each other, and thread T1 requests A at the + // same moment thread T2 requests B, you could get a sequence like this: + // - T1 locks A + // - T2 locks B + // - T1, having constructed A, calls A::initSingleton(), which calls + // B::getInstance() and blocks on B's lock + // - T2, having constructed B, calls B::initSingleton(), which calls + // A::getInstance() and blocks on A's lock + // In other words, classic deadlock. + + // Avoid that by constructing and initializing every LLSingleton on + // the main thread. In that scenario: + // - T1 locks A + // - T2 locks B + // - T1 discovers A is UNINITIALIZED, so it queues a task for the main + // thread, unlocks A and blocks on the std::future. + // - T2 discovers B is UNINITIALIZED, so it queues a task for the main + // thread, unlocks B and blocks on the std::future. + // - The main thread executes T1's request for A. It locks A and + // starts to construct it. + // - A::initSingleton() calls B::getInstance(). Fine: nobody's holding + // B's lock. + // - The main thread locks B, constructs B, calls B::initSingleton(), + // which calls A::getInstance(), which returns A. + // - B::getInstance() returns B to A::initSingleton(), unlocking B. + // - A::getInstance() returns A to the task wrapper, unlocking A. + // - The task wrapper passes A to T1 via the future. T1 resumes. + // - The main thread executes T2's request for B. Oh look, B already + // exists. The task wrapper passes B to T2 via the future. T2 + // resumes. + // This still works even if one of T1 or T2 *is* the main thread. + // This still works even if thread T3 requests B at the same moment as + // T2. Finding B still UNINITIALIZED, T3 also queues a task for the + // main thread, unlocks B and blocks on a (distinct) std::future. By + // the time the main thread executes T3's request for B, B already + // exists, and is simply delivered via the future. + + { // nested scope for 'lk' + // In case racing threads call getInstance() at the same moment, + // serialize the calls. + LockStatic lk; + + switch (lk->mInitState) + { + case CONSTRUCTING: + // here if DERIVED_TYPE's constructor (directly or indirectly) + // calls DERIVED_TYPE::getInstance() + logerrs("Tried to access singleton ", + classname().c_str(), + " from singleton constructor!"); + return nullptr; + + case INITIALIZING: + // here if DERIVED_TYPE::initSingleton() (directly or indirectly) + // calls DERIVED_TYPE::getInstance(): go ahead and allow it + case INITIALIZED: + // normal subsequent calls + // record the dependency, if any: check if we got here from another + // LLSingleton's constructor or initSingleton() method + capture_dependency(lk->mInstance); + return lk->mInstance; + + case DELETED: + // called after deleteSingleton() + logwarns("Trying to access deleted singleton ", + classname().c_str(), + " -- creating new instance"); + // fall through + case UNINITIALIZED: + break; + } + + // Here we need to construct a new instance. + if (on_main_thread()) + { + // On the main thread, directly construct the instance while + // holding the lock. + constructSingleton(lk); + capture_dependency(lk->mInstance); + return lk->mInstance; + } + } // unlock 'lk' + + // Here we need to construct a new instance, but we're on a secondary + // thread. Per the comment block above, dispatch to the main thread. + loginfos(classname().c_str(), + "::getInstance() dispatching to main thread"); + auto instance = LLMainThreadTask::dispatch( + [](){ + // VERY IMPORTANT to call getInstance() on the main thread, + // rather than going straight to constructSingleton()! + // During the time window before mInitState is INITIALIZED, + // multiple requests might be queued. It's essential that, as + // the main thread processes them, only the FIRST such request + // actually constructs the instance -- every subsequent one + // simply returns the existing instance. + loginfos(classname().c_str(), + "::getInstance() on main thread"); + return getInstance(); + }); + // record the dependency chain tracked on THIS thread, not the main + // thread (consider a getInstance() overload with a tag param that + // suppresses dep tracking when dispatched to the main thread) + capture_dependency(instance); + loginfos(classname().c_str(), + "::getInstance() returning on invoking thread"); + return instance; } // Reference version of getInstance() - // Preferred over getInstance() as it disallows checking for NULL + // Preferred over getInstance() as it disallows checking for nullptr static DERIVED_TYPE& instance() { return *getInstance(); @@ -537,8 +604,8 @@ public: static bool instanceExists() { // defend any access to sData from racing threads - Locker lk; - return sData.mInitState == INITIALIZED; + LockStatic lk; + return lk->mInitState == INITIALIZED; } // Has this singleton been deleted? This can be useful during shutdown @@ -547,24 +614,11 @@ public: static bool wasDeleted() { // defend any access to sData from racing threads - Locker lk; - return sData.mInitState == DELETED; + LockStatic lk; + return lk->mInitState == DELETED; } - -private: - struct SingletonData - { - // explicitly has a default constructor so that member variables are zero initialized in BSS - // and only changed by singleton logic, not constructor running during startup - EInitState mInitState; - DERIVED_TYPE* mInstance; - }; - static SingletonData sData; }; -template -typename LLSingleton::SingletonData LLSingleton::sData; - /** * LLParamSingleton is like LLSingleton, except in the following ways: @@ -589,7 +643,7 @@ class LLParamSingleton : public LLSingleton { private: typedef LLSingleton super; - using typename super::Locker; + using typename super::LockStatic; public: using super::deleteSingleton; @@ -603,9 +657,9 @@ public: // In case racing threads both call initParamSingleton() at the same // time, serialize them. One should initialize; the other should see // mInitState already set. - Locker lk; + LockStatic lk; // For organizational purposes this function shouldn't be called twice - if (super::sData.mInitState != super::UNINITIALIZED) + if (lk->mInitState != super::UNINITIALIZED) { super::logerrs("Tried to initialize singleton ", super::template classname().c_str(), @@ -613,7 +667,7 @@ public: } else { - super::constructSingleton(std::forward(args)...); + super::constructSingleton(lk, std::forward(args)...); } } @@ -621,9 +675,9 @@ public: { // In case racing threads call getInstance() at the same moment as // initParamSingleton(), serialize the calls. - Locker lk; + LockStatic lk; - switch (super::sData.mInitState) + switch (lk->mInitState) { case super::UNINITIALIZED: super::logerrs("Uninitialized param singleton ", @@ -641,8 +695,8 @@ public: // within initSingleton() case super::INITIALIZED: // for any valid call, capture dependencies - super::capture_dependency(); - return super::sData.mInstance; + super::capture_dependency(lk->mInstance); + return lk->mInstance; case super::DELETED: super::logerrs("Trying to access deleted param singleton ", -- cgit v1.2.3 From 68505edf25f5013edfe7197405228bee9a299b68 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 12 Dec 2019 16:19:23 -0500 Subject: DRTVWR-494: LLParamSingleton::initParamSingleton() now returns T*. So does LLLockedSingleton::construct(). --- indra/llcommon/llsingleton.h | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 4f3b8ceb38..39d0e9b013 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -650,9 +650,15 @@ public: using super::instanceExists; using super::wasDeleted; - // Passes arguments to DERIVED_TYPE's constructor and sets appropriate states + // Passes arguments to DERIVED_TYPE's constructor and sets appropriate + // states. We'd rather return a reference than a pointer, but the test for + // redundant calls makes that awkward. The compiler, unaware that + // logerrs() won't return, requires that that alternative return + // *something*. But what? It can't be a dummy static instance because + // there should be only one instance of any LLSingleton subclass! Easier + // to allow that case to return nullptr. template - static void initParamSingleton(Args&&... args) + static DERIVED_TYPE* initParamSingleton(Args&&... args) { // In case racing threads both call initParamSingleton() at the same // time, serialize them. One should initialize; the other should see @@ -664,10 +670,12 @@ public: super::logerrs("Tried to initialize singleton ", super::template classname().c_str(), " twice!"); + return nullptr; } else { super::constructSingleton(lk, std::forward(args)...); + return lk->mInstance; } } @@ -740,9 +748,9 @@ public: using super::instanceExists; using super::wasDeleted; - static void construct() + static DT* construct() { - super::initParamSingleton(); + return super::initParamSingleton(); } }; -- cgit v1.2.3 From 7e9c5dd0a3e583a9bec4e99d99b2efca358c6612 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 12 Dec 2019 17:17:06 -0500 Subject: DRTVWR-494: LLParamSingleton::initParamSingleton() on main thread. When calling LLParamSingleton::initParamSingleton() on a secondary thread, use LLMainThreadTask::dispatch() to construct the instance on the main thread -- as with LLSingleton::getInstance(). --- indra/llcommon/llsingleton.h | 47 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 39d0e9b013..314ee2caa8 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -59,6 +59,7 @@ protected: typedef enum e_init_state { UNINITIALIZED = 0, // must be default-initialized state + QUEUED, // construction queued, not yet executing CONSTRUCTING, // within DERIVED_TYPE constructor INITIALIZING, // within DERIVED_TYPE::initSingleton() INITIALIZED, // normal case @@ -552,6 +553,11 @@ public: " -- creating new instance"); // fall through case UNINITIALIZED: + case QUEUED: + // QUEUED means some secondary thread has already requested an + // instance, but for present purposes that's semantically + // identical to UNINITIALIZED: either way, we must ourselves + // request an instance. break; } @@ -564,10 +570,13 @@ public: capture_dependency(lk->mInstance); return lk->mInstance; } + + // Here we need to construct a new instance, but we're on a secondary + // thread. + lk->mInitState = QUEUED; } // unlock 'lk' - // Here we need to construct a new instance, but we're on a secondary - // thread. Per the comment block above, dispatch to the main thread. + // Per the comment block above, dispatch to the main thread. loginfos(classname().c_str(), "::getInstance() dispatching to main thread"); auto instance = LLMainThreadTask::dispatch( @@ -588,7 +597,7 @@ public: // suppresses dep tracking when dispatched to the main thread) capture_dependency(instance); loginfos(classname().c_str(), - "::getInstance() returning on invoking thread"); + "::getInstance() returning on requesting thread"); return instance; } @@ -672,11 +681,40 @@ public: " twice!"); return nullptr; } - else + else if (on_main_thread()) { + // on the main thread, simply construct instance while holding lock super::constructSingleton(lk, std::forward(args)...); return lk->mInstance; } + else + { + // on secondary thread, dispatch to main thread -- + // set state so we catch any other calls before the main thread + // picks up the task + lk->mInitState = super::QUEUED; + // very important to unlock here so main thread can actually process + lk.unlock(); + super::loginfos(super::template classname().c_str(), + "::initParamSingleton() dispatching to main thread"); + // Normally it would be the height of folly to reference-bind + // 'args' into a lambda to be executed on some other thread! By + // the time that thread executed the lambda, the references would + // all be dangling, and Bad Things would result. But + // LLMainThreadTask::dispatch() promises to block until the passed + // task has completed. So in this case we know the references will + // remain valid until the lambda has run, so we dare to bind + // references. + auto instance = LLMainThreadTask::dispatch( + [&](){ + super::loginfos(super::template classname().c_str(), + "::initParamSingleton() on main thread"); + return initParamSingleton(std::forward(args)...); + }); + super::loginfos(super::template classname().c_str(), + "::initParamSingleton() returning on requesting thread"); + return instance; + } } static DERIVED_TYPE* getInstance() @@ -688,6 +726,7 @@ public: switch (lk->mInitState) { case super::UNINITIALIZED: + case super::QUEUED: super::logerrs("Uninitialized param singleton ", super::template classname().c_str()); break; -- cgit v1.2.3 From 0a9a20a5df5ea7fd8f542e6f865a795a07ce5160 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 12 Dec 2019 17:23:54 -0500 Subject: DRTVWR-494: LLParamSingleton::initParamSingleton() returns reference. --- indra/llcommon/llsingleton.h | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 314ee2caa8..0c11e54910 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -654,20 +654,10 @@ private: typedef LLSingleton super; using typename super::LockStatic; -public: - using super::deleteSingleton; - using super::instanceExists; - using super::wasDeleted; - // Passes arguments to DERIVED_TYPE's constructor and sets appropriate - // states. We'd rather return a reference than a pointer, but the test for - // redundant calls makes that awkward. The compiler, unaware that - // logerrs() won't return, requires that that alternative return - // *something*. But what? It can't be a dummy static instance because - // there should be only one instance of any LLSingleton subclass! Easier - // to allow that case to return nullptr. + // states, returning a pointer to the new instance. template - static DERIVED_TYPE* initParamSingleton(Args&&... args) + static DERIVED_TYPE* initParamSingleton_(Args&&... args) { // In case racing threads both call initParamSingleton() at the same // time, serialize them. One should initialize; the other should see @@ -709,7 +699,7 @@ public: [&](){ super::loginfos(super::template classname().c_str(), "::initParamSingleton() on main thread"); - return initParamSingleton(std::forward(args)...); + return initParamSingleton_(std::forward(args)...); }); super::loginfos(super::template classname().c_str(), "::initParamSingleton() returning on requesting thread"); @@ -717,6 +707,19 @@ public: } } +public: + using super::deleteSingleton; + using super::instanceExists; + using super::wasDeleted; + + /// initParamSingleton() constructs the instance, returning a reference. + /// Pass whatever arguments are required to construct DERIVED_TYPE. + template + static DERIVED_TYPE& initParamSingleton(Args&&... args) + { + return *initParamSingleton_(std::forward(args)...); + } + static DERIVED_TYPE* getInstance() { // In case racing threads call getInstance() at the same moment as -- cgit v1.2.3 From 31863d833c7b573f3608e3353b9e5f694b611627 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 17 Dec 2019 15:42:34 -0500 Subject: DRTVWR-494: Move most LLSingleton cleanup back to destructor instead of deleteSingleton(). Specifically, clear static SingletonData and remove the instance from the MasterList in the destructor. Empirically, some consumers are manually deleting LLSingleton instances, instead of calling deleteSingleton(). If deleteSingleton() handles cleanup rather than the destructor, we're left with dangling pointers in the Master List. We don't also call cleanupSingleton() from the destructor because only deleteSingleton() promises to call cleanupSingleton(). Hopefully whoever is directly deleting an LLSingleton subclass instance isn't relying on cleanupSingleton(). --- indra/llcommon/llsingleton.h | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 0c11e54910..5ee40a658a 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -428,6 +428,21 @@ protected: LLSingleton_manage_master().add(this); } +protected: + virtual ~LLSingleton() + { + // This phase of cleanup is performed in the destructor rather than in + // deleteSingleton() to defend against manual deletion. When we moved + // cleanup to deleteSingleton(), we hit crashes due to dangling + // pointers in the MasterList. + LockStatic lk; + lk->mInstance = nullptr; + lk->mInitState = DELETED; + + // Remove this instance from the master list. + LLSingleton_manage_master().remove(this); + } + public: /** * @brief Immediately delete the singleton. @@ -452,29 +467,16 @@ public: */ static void deleteSingleton() { - DERIVED_TYPE* lameduck; - { - LockStatic lk; - // Capture the instance and clear SingletonData. This sequence - // guards against the chance that the destructor throws, somebody - // catches it and there's a subsequent call to getInstance(). - lameduck = lk->mInstance; - lk->mInstance = nullptr; - lk->mInitState = DELETED; - // At this point we can safely unlock SingletonData during the - // remaining cleanup. If another thread calls deleteSingleton() (or - // getInstance(), or whatever) it won't find our instance, now - // referenced only as 'lameduck'. - } + // Hold the lock while we call cleanupSingleton() and the destructor. + // Our destructor also instantiates LockStatic, requiring a recursive + // mutex. + LockStatic lk; // of course, only cleanup and delete if there's something there - if (lameduck) + if (lk->mInstance) { - // remove this instance from the master list BEFORE attempting - // cleanup so possible destructor exception won't leave the master - // list confused - LLSingleton_manage_master().remove(lameduck); - lameduck->cleanup_(); - delete lameduck; + lk->mInstance->cleanup_(); + delete lk->mInstance; + // destructor clears mInstance (and mInitState) } } -- cgit v1.2.3 From 47ec6ab3be5df5ee3f80a642d9c2ef7f4dac0d8a Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 30 May 2019 08:23:32 -0400 Subject: SL-11216: Remove LLSingletonBase::cleanupAll(). Remove call from LLAppViewer::cleanup(). Instead, make each LLSingleton::deleteSingleton() call cleanupSingleton() just before destroying the instance. Since deleteSingleton() is not a destructor, it's fine to call cleanupSingleton() from there; and since deleteAll() calls deleteSingleton() on every remaining instance, the former cleanupAll() functionality has been subsumed into deleteAll(). Since cleanupSingleton() is now called at exactly one point in the instance's lifetime, we no longer need a bool indicating whether it has been called. The previous protocol of calling cleanupAll() before deleteAll() implemented a two-phase cleanup strategy for the application. That is no longer needed. Moreover, the cleanupAll() / deleteAll() sequence created a time window during which individual LLSingleton instances weren't usable (to the extent that their cleanupSingleton() methods released essential resources) but still existed -- so a getInstance() call would return the crippled instance rather than recreating it. Remove cleanupAll() calls from tests; adjust to new order of expected side effects: instead of A::cleanupSingleton(), B::cleanupSingleton(), ~A(), ~B(), now we get A::cleanupSingleton(), ~A(), B::cleanupSingleton(), ~B(). --- indra/llcommon/llsingleton.h | 105 ++++++++++++++++--------------------------- 1 file changed, 38 insertions(+), 67 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 5ee40a658a..65dd332afb 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -50,7 +50,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; @@ -142,32 +141,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 @@ -175,9 +157,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(); }; @@ -226,7 +208,6 @@ struct LLSingleton_manage_master // Now we can implement LLSingletonBase's template constructor. template LLSingletonBase::LLSingletonBase(tag): - mCleaned(false), mDeleteSingleton(nullptr) { // This is the earliest possible point at which we can push this new @@ -269,10 +250,19 @@ class LLParamSingleton; * 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 @@ -280,26 +270,14 @@ class LLParamSingleton; * 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.) */ template class LLSingleton : public LLSingletonBase @@ -445,25 +423,18 @@ protected: public: /** - * @brief Immediately delete the singleton. + * @brief Cleanup and destroy the singleton instance. * - * A subsequent call to LLProxy::getInstance() will construct a new - * instance of the class. + * deleteSingleton() calls this instance's cleanupSingleton() method and + * then destroys the instance. * - * 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. + * A subsequent call to LLSingleton::getInstance() will construct a new + * instance of the class. * - * 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() { -- cgit v1.2.3 From d1175e8fa1eb873e6accbb2f220686077cad38dc Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 13 Dec 2019 15:24:56 -0500 Subject: DRTVWR-476: Log calls to LLParamSingleton::initParamSingleton(). --- indra/llcommon/llsingleton.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 65dd332afb..27c2ceb3b6 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -110,15 +110,15 @@ protected: // A::initSingleton(), record that A directly depends on B. void capture_dependency(); - // delegate LL_ERRS() logging to llsingleton.cpp + // delegate logging calls to llsingleton.cpp static void logerrs(const char* p1, const char* p2="", const char* p3="", const char* p4=""); - // delegate LL_WARNS() logging to llsingleton.cpp static void logwarns(const char* p1, const char* p2="", const char* p3="", const char* p4=""); - // delegate LL_INFOS() logging to llsingleton.cpp static void loginfos(const char* p1, const char* p2="", const char* p3="", const char* p4=""); + static void logdebugs(const char* p1, const char* p2="", + const char* p3="", const char* p4=""); static std::string demangle(const char* mangled); template static std::string classname() { return demangle(typeid(T).name()); } @@ -647,6 +647,8 @@ private: else if (on_main_thread()) { // on the main thread, simply construct instance while holding lock + super::logdebugs(super::template classname().c_str(), + "::initParamSingleton()"); super::constructSingleton(lk, std::forward(args)...); return lk->mInstance; } -- cgit v1.2.3 From dc07509f296661cf7a4d4bceb88a1a897757de98 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 3 Apr 2020 10:38:53 -0400 Subject: DRTVWR-476: Cherry-pick debug aids from commit 77b0c53 (fiber-mutex) --- indra/llcommon/llsingleton.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'indra/llcommon/llsingleton.h') diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 27c2ceb3b6..ccd2e48bf2 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -120,6 +120,8 @@ protected: static void logdebugs(const char* p1, const char* p2="", const char* p3="", const char* p4=""); static std::string demangle(const char* mangled); + // these classname() declarations restate template functions declared in + // llerror.h because we avoid #including that here template static std::string classname() { return demangle(typeid(T).name()); } template -- cgit v1.2.3