diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp
index c45c14457048ecc65da4fc5593f4a5d54972c211..812fd3171959fc768d3bb028902752ff18dbe33d 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 @@ class LLSingletonBase::MasterList:
// 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 @@ class LLSingletonBase::MasterList:
}
};
-//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 0da6d548ab007aaf1719acba4a4093919d7f9b3b..8dec8bfb3bc339ed1fe470645c0831e1101d9503 100644
--- a/indra/llcommon/llsingleton.h
+++ b/indra/llcommon/llsingleton.h
@@ -51,10 +51,9 @@ class LLSingletonBase: private boost::noncopyable
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 @@ class LLSingletonBase: private boost::noncopyable
// 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 @@ class LLSingleton : public LLSingletonBase
// 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 @@ class LLSingleton : public LLSingletonBase
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 @@ class LLSingleton : public LLSingletonBase
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 @@ class LLSingleton : public LLSingletonBase
// 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 @@ class LLSingleton : public LLSingletonBase
// 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 @@ class LLSingleton : public LLSingletonBase
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 @@ class LLSingleton : public LLSingletonBase
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 @@ class LLSingleton : public LLSingletonBase
" 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 @@ class LLSingleton : public LLSingletonBase
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 @@ class LLSingleton : public LLSingletonBase
// 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 @@ class LLSingleton : public LLSingletonBase
// 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 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE>
// 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 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE>
{
// 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 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE>
{
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;
- }
};
/**