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.cpp | 151 +++++++++++++++++++++++++++++++---------- 1 file changed, 117 insertions(+), 34 deletions(-) (limited to 'indra/llcommon/llsingleton.cpp') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index c45c144570..812fd31719 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -31,6 +31,7 @@ #include "llerrorcontrol.h" // LLError::is_available() #include "lldependencies.h" #include "llcoro_get_id.h" +#include "llexception.h" #include #include #include @@ -57,17 +58,59 @@ bool oktolog(); class LLSingletonBase::MasterList: public LLSingleton { +private: LLSINGLETON_EMPTY_CTOR(MasterList); -public: - // No need to make this private with accessors; nobody outside this source - // file can see it. + // Independently of the LLSingleton locks governing construction, + // destruction and other state changes of the MasterList instance itself, + // we must also defend each of the data structures owned by the + // MasterList. + // This must be a recursive_mutex because, while the lock is held for + // manipulating some data in the master list, we must also check whether + // it's safe to log -- which involves querying a different LLSingleton -- + // which requires accessing the master list. + typedef std::recursive_mutex mutex_t; + typedef std::unique_lock lock_t; + + mutex_t mMutex; +public: + // Instantiate this to both obtain a reference to MasterList::instance() + // and lock its mutex for the lifespan of this Lock instance. + class Lock + { + public: + Lock(): + mMasterList(MasterList::instance()), + mLock(mMasterList.mMutex) + {} + Lock(const Lock&) = delete; + Lock& operator=(const Lock&) = delete; + MasterList& get() const { return mMasterList; } + operator MasterList&() const { return get(); } + + protected: + MasterList& mMasterList; + MasterList::lock_t mLock; + }; + +private: // This is the master list of all instantiated LLSingletons (save the // MasterList itself) in arbitrary order. You MUST call dep_sort() before // traversing this list. - LLSingletonBase::list_t mMaster; + list_t mMaster; + +public: + // Instantiate this to obtain a reference to MasterList::mMaster and to + // hold the MasterList lock for the lifespan of this LockedMaster + // instance. + struct LockedMaster: public Lock + { + list_t& get() const { return mMasterList.mMaster; } + operator list_t&() const { return get(); } + }; +private: // We need to maintain a stack of LLSingletons currently being // initialized, either in the constructor or in initSingleton(). However, // managing that as a stack depends on having a DISTINCT 'initializing' @@ -83,10 +126,44 @@ public: // Instead, use a map of llcoro::id to select the appropriate // coro-specific 'initializing' stack. llcoro::get_id() is carefully // implemented to avoid requiring LLCoros. - typedef boost::unordered_map InitializingMap; + typedef boost::unordered_map InitializingMap; InitializingMap mInitializing; - // non-static method, cf. LLSingletonBase::get_initializing() +public: + // Instantiate this to obtain a reference to the coroutine-specific + // initializing list and to hold the MasterList lock for the lifespan of + // this LockedInitializing instance. + struct LockedInitializing: public Lock + { + public: + LockedInitializing(): + // only do the lookup once, cache the result + // note that the lock is already locked during this lookup + mList(&mMasterList.get_initializing_()) + {} + list_t& get() const + { + if (! mList) + { + LLTHROW(std::runtime_error("Trying to use LockedInitializing " + "after cleanup_initializing()")); + } + return *mList; + } + operator list_t&() const { return get(); } + void log(const char* verb, const char* name); + void cleanup_initializing() + { + mMasterList.cleanup_initializing_(); + mList = nullptr; + } + + private: + // Store pointer since cleanup_initializing() must clear it. + list_t* mList; + }; + +private: list_t& get_initializing_() { // map::operator[] has find-or-create semantics, exactly what we need @@ -104,16 +181,12 @@ public: } }; -//static -LLSingletonBase::list_t& LLSingletonBase::get_master() -{ - return LLSingletonBase::MasterList::instance().mMaster; -} - void LLSingletonBase::add_master() { // As each new LLSingleton is constructed, add to the master list. - get_master().push_back(this); + // This temporary LockedMaster should suffice to hold the MasterList lock + // during the push_back() call. + MasterList::LockedMaster().get().push_back(this); } void LLSingletonBase::remove_master() @@ -125,27 +198,32 @@ void LLSingletonBase::remove_master() // 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); + // This temporary LockedMaster should suffice to hold the MasterList lock + // during the remove() call. + MasterList::LockedMaster().get().remove(this); } //static -LLSingletonBase::list_t& LLSingletonBase::get_initializing() +LLSingletonBase::list_t::size_type LLSingletonBase::get_initializing_size() { - return LLSingletonBase::MasterList::instance().get_initializing_(); + return MasterList::LockedInitializing().get().size(); } LLSingletonBase::~LLSingletonBase() {} void LLSingletonBase::push_initializing(const char* name) { + MasterList::LockedInitializing locked_list; // log BEFORE pushing so logging singletons don't cry circularity - log_initializing("Pushing", name); - get_initializing().push_back(this); + locked_list.log("Pushing", name); + locked_list.get().push_back(this); } void LLSingletonBase::pop_initializing() { - list_t& list(get_initializing()); + // Lock the MasterList for the duration of this call + MasterList::LockedInitializing locked_list; + list_t& list(locked_list.get()); if (list.empty()) { @@ -165,7 +243,7 @@ void LLSingletonBase::pop_initializing() // entirely. if (list.empty()) { - MasterList::instance().cleanup_initializing_(); + locked_list.cleanup_initializing(); } // Now validate the newly-popped LLSingleton. @@ -177,7 +255,7 @@ void LLSingletonBase::pop_initializing() } // log AFTER popping so logging singletons don't cry circularity - log_initializing("Popping", typeid(*back).name()); + locked_list.log("Popping", typeid(*back).name()); } void LLSingletonBase::reset_initializing(list_t::size_type size) @@ -191,7 +269,8 @@ void LLSingletonBase::reset_initializing(list_t::size_type size) // 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()); + MasterList::LockedInitializing locked_list; + list_t& list(locked_list.get()); while (list.size() > size) { @@ -201,29 +280,32 @@ void LLSingletonBase::reset_initializing(list_t::size_type size) // as in pop_initializing() if (list.empty()) { - MasterList::instance().cleanup_initializing_(); + locked_list.cleanup_initializing(); } } -//static -void LLSingletonBase::log_initializing(const char* verb, const char* name) +void LLSingletonBase::MasterList::LockedInitializing::log(const char* verb, const char* name) { if (oktolog()) { LL_DEBUGS("LLSingleton") << verb << ' ' << demangle(name) << ';'; - list_t& list(get_initializing()); - for (list_t::const_reverse_iterator ri(list.rbegin()), rend(list.rend()); - ri != rend; ++ri) + if (mList) { - LLSingletonBase* sb(*ri); - LL_CONT << ' ' << classname(sb); + for (list_t::const_reverse_iterator ri(mList->rbegin()), rend(mList->rend()); + ri != rend; ++ri) + { + LLSingletonBase* sb(*ri); + LL_CONT << ' ' << classname(sb); + } } LL_ENDL; } } -void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initState) +void LLSingletonBase::capture_dependency(EInitState initState) { + MasterList::LockedInitializing locked_list; + list_t& initializing(locked_list.get()); // 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 @@ -313,8 +395,9 @@ LLSingletonBase::vec_t LLSingletonBase::dep_sort() // deleteAll(). typedef LLDependencies SingletonDeps; SingletonDeps sdeps; - list_t& master(get_master()); - BOOST_FOREACH(LLSingletonBase* sp, master) + // Lock while traversing the master list + MasterList::LockedMaster master; + BOOST_FOREACH(LLSingletonBase* sp, master.get()) { // Build the SingletonDeps structure by adding, for each // LLSingletonBase* sp in the master list, sp itself. It has no @@ -326,7 +409,7 @@ LLSingletonBase::vec_t LLSingletonBase::dep_sort() SingletonDeps::KeyList(sp->mDepends.begin(), sp->mDepends.end())); } vec_t ret; - ret.reserve(master.size()); + ret.reserve(master.get().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 -- cgit v1.2.3