diff options
author | Nat Goodspeed <nat@lindenlab.com> | 2019-11-23 22:18:45 -0500 |
---|---|---|
committer | Nat Goodspeed <nat@lindenlab.com> | 2020-03-25 15:28:17 -0400 |
commit | b22f89c9fa9e6ee95b552b27808df77f710caad6 (patch) | |
tree | 117dd5082c7fa8568ba0c310769e23cb1248c54e | |
parent | 7915dc45624e714706c497b45b5f2b663fa0cdc2 (diff) |
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<T>::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<std::recursive_mutex> 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<T>::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<T>::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)
-rw-r--r-- | indra/llcommon/llsingleton.cpp | 151 | ||||
-rw-r--r-- | indra/llcommon/llsingleton.h | 164 |
2 files changed, 197 insertions, 118 deletions
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 <boost/foreach.hpp> #include <boost/unordered_map.hpp> #include <algorithm> @@ -57,17 +58,59 @@ bool oktolog(); class LLSingletonBase::MasterList: public LLSingleton<LLSingletonBase::MasterList> { +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<mutex_t> 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<llcoro::id, LLSingletonBase::list_t> InitializingMap; + typedef boost::unordered_map<llcoro::id, list_t> 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<LLSingletonBase*> 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 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<LLSingletonBase*> 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<LLSingletonBase*> of master list, in dependency order. typedef std::vector<LLSingletonBase*> 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<LLSingletonBase::MasterList> 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 <typename DERIVED_TYPE> class LLSingleton : public LLSingletonBase @@ -315,6 +311,47 @@ private: // access our private members. friend class LLParamSingleton<DERIVED_TYPE>; + // Scoped lock on the mutex associated with this LLSingleton<T> + 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<T> 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<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; + } + + std::unique_lock<mutex_t> 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 <typename... Args> static void constructSingleton(Args&&... args) { - auto prev_size = LLSingleton_manage_master<DERIVED_TYPE>().get_initializing().size(); + auto prev_size = LLSingleton_manage_master<DERIVED_TYPE>().get_initializing_size(); // getInstance() calls are from within constructor sData.mInitState = CONSTRUCTING; try @@ -386,9 +423,6 @@ private: LLSingleton_manage_master<DERIVED_TYPE>().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<DERIVED_TYPE>().get_initializing(), - sData.mInitState); + LLSingleton_manage_master<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<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; @@ -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<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; + 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<mutex_t> 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<mutex_t> 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<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; - } }; /** |