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)

1 files changed, 80 insertions, 84 deletions

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;
-    }
 };
 
 /**