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.

3 files changed, 218 insertions, 166 deletions

diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp
index 812fd31719..bf594f122c 100644
--- a/indra/llcommon/llsingleton.cpp
+++ b/indra/llcommon/llsingleton.cpp
@@ -302,7 +302,7 @@ void LLSingletonBase::MasterList::LockedInitializing::log(const char* verb, cons
     }
 }
 
-void LLSingletonBase::capture_dependency(EInitState initState)
+void LLSingletonBase::capture_dependency()
 {
     MasterList::LockedInitializing locked_list;
     list_t& initializing(locked_list.get());
@@ -334,21 +334,8 @@ void LLSingletonBase::capture_dependency(EInitState initState)
                 LLSingletonBase* foundp(*found);
                 out << classname(foundp) << " -> ";
             }
-            // We promise to capture dependencies from both the constructor
-            // and the initSingleton() method, so an LLSingleton's instance
-            // pointer is on the initializing list during both. Now that we've
-            // detected circularity, though, we must distinguish the two. If
-            // the recursive call is from the constructor, we CAN'T honor it:
-            // otherwise we'd be returning a pointer to a partially-
-            // constructed object! But from initSingleton() is okay: that
-            // method exists specifically to support circularity.
             // Decide which log helper to call.
-            if (initState == CONSTRUCTING)
-            {
-                logerrs("LLSingleton circularity in Constructor: ", out.str().c_str(),
-                    classname(this).c_str(), "");
-            }
-            else if (it_next == initializing.end())
+            if (it_next == initializing.end())
             {
                 // Points to self after construction, but during initialization.
                 // Singletons can initialize other classes that depend onto them,
@@ -457,6 +444,19 @@ void LLSingletonBase::cleanupAll()
     }
 }
 
+void LLSingletonBase::cleanup_()
+{
+    logdebugs("calling ", classname(this).c_str(), "::cleanupSingleton()");
+    try
+    {
+        cleanupSingleton();
+    }
+    catch (...)
+    {
+        LOG_UNHANDLED_EXCEPTION(classname(this) + "::cleanupSingleton()");
+    }
+}
+
 //static
 void LLSingletonBase::deleteAll()
 {
@@ -537,6 +537,12 @@ void LLSingletonBase::logwarns(const char* p1, const char* p2, const char* p3, c
 }
 
 //static
+void LLSingletonBase::loginfos(const char* p1, const char* p2, const char* p3, const char* p4)
+{
+    log(LLError::LEVEL_INFO, p1, p2, p3, p4);
+}
+
+//static
 void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, const char* p4)
 {
     log(LLError::LEVEL_ERROR, p1, p2, p3, p4);
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 <vector>
 #include <typeinfo>
 #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 <typename T>
     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<LLSingletonBase::MasterList>
     // 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 <typename DERIVED_TYPE>
 LLSingletonBase::LLSingletonBase(tag<DERIVED_TYPE>):
     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 <typename DERIVED_TYPE>
 class LLSingleton : public LLSingletonBase
 {
 private:
-    // Allow LLParamSingleton subclass -- but NOT DERIVED_TYPE itself -- to
-    // access our private members.
-    friend class LLParamSingleton<DERIVED_TYPE>;
-
-    // Scoped lock on the mutex associated with this LLSingleton<T>
-    class Locker
+    // LLSingleton<DERIVED_TYPE> 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<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;
+        EInitState      mInitState{UNINITIALIZED};
+        DERIVED_TYPE*   mInstance{nullptr};
     };
+    typedef llthread::LockStatic<SingletonData> LockStatic;
+
+    // Allow LLParamSingleton subclass -- but NOT DERIVED_TYPE itself -- to
+    // access our private members.
+    friend class LLParamSingleton<DERIVED_TYPE>;
 
     // 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 <typename... Args>
-    static void constructSingleton(Args&&... args)
+    static void constructSingleton(LockStatic& lk, Args&&... args)
     {
         auto prev_size = LLSingleton_manage_master<DERIVED_TYPE>().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>(args)...);
-            // we have called constructor, have not yet called initSingleton()
+            lk->mInstance = new DERIVED_TYPE(std::forward<Args>(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<DERIVED_TYPE>().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<DERIVED_TYPE>().pop_initializing(sData.mInstance);
+        LLSingleton_manage_master<DERIVED_TYPE>().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<DERIVED_TYPE>().capture_dependency(
-            sData.mInstance, sData.mInitState);
+        LLSingleton_manage_master<DERIVED_TYPE>().capture_dependency(sb);
     }
 
     // We know of no way to instruct the compiler that every subclass
@@ -442,19 +427,6 @@ protected:
         LLSingleton_manage_master<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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<DERIVED_TYPE>().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 T>
-typename LLSingleton<T>::SingletonData LLSingleton<T>::sData;
-
 
 /**
  * LLParamSingleton<T> is like LLSingleton<T>, except in the following ways:
@@ -589,7 +643,7 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE>
 {
 private:
     typedef LLSingleton<DERIVED_TYPE> 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<DERIVED_TYPE>().c_str(),
@@ -613,7 +667,7 @@ public:
         }
         else
         {
-            super::constructSingleton(std::forward<Args>(args)...);
+            super::constructSingleton(lk, std::forward<Args>(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 ",
diff --git a/indra/llcommon/tests/llsingleton_test.cpp b/indra/llcommon/tests/llsingleton_test.cpp
index 75ddff9d7d..15ffe68e67 100644
--- a/indra/llcommon/tests/llsingleton_test.cpp
+++ b/indra/llcommon/tests/llsingleton_test.cpp
@@ -143,8 +143,6 @@ namespace tut
                                                                         \
         (void)CLS::instance();                                          \
         ensure_equals(sLog, #CLS "i" #CLS);                             \
-        LLSingletonBase::cleanupAll();                                  \
-        ensure_equals(sLog, #CLS "i" #CLS "x" #CLS);                    \
         LLSingletonBase::deleteAll();                                   \
         ensure_equals(sLog, #CLS "i" #CLS "x" #CLS "~" #CLS);           \
     }                                                                   \
@@ -159,10 +157,8 @@ namespace tut
                                                                         \
         (void)CLS::instance();                                          \
         ensure_equals(sLog, #CLS #OTHER "i" #OTHER "i" #CLS);           \
-        LLSingletonBase::cleanupAll();                                  \
-        ensure_equals(sLog, #CLS #OTHER "i" #OTHER "i" #CLS "x" #CLS "x" #OTHER); \
         LLSingletonBase::deleteAll();                                   \
-        ensure_equals(sLog, #CLS #OTHER "i" #OTHER "i" #CLS "x" #CLS "x" #OTHER "~" #CLS "~" #OTHER); \
+        ensure_equals(sLog, #CLS #OTHER "i" #OTHER "i" #CLS "x" #CLS "~" #CLS "x" #OTHER "~" #OTHER); \
     }                                                                   \
                                                                         \
     template<> template<>                                               \
@@ -175,10 +171,8 @@ namespace tut
                                                                         \
         (void)CLS::instance();                                          \
         ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER);           \
-        LLSingletonBase::cleanupAll();                                  \
-        ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "x" #OTHER); \
         LLSingletonBase::deleteAll();                                   \
-        ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "x" #OTHER "~" #CLS "~" #OTHER); \
+        ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "~" #CLS "x" #OTHER "~" #OTHER); \
     }                                                                   \
                                                                         \
     template<> template<>                                               \
@@ -191,10 +185,8 @@ namespace tut
                                                                         \
         (void)CLS::instance();                                          \
         ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER);           \
-        LLSingletonBase::cleanupAll();                                  \
-        ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "x" #OTHER); \
         LLSingletonBase::deleteAll();                                   \
-        ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "x" #OTHER "~" #CLS "~" #OTHER); \
+        ensure_equals(sLog, #CLS "i" #CLS #OTHER "i" #OTHER "x" #CLS "~" #CLS "x" #OTHER "~" #OTHER); \
     }
 
     TESTS(A, B, 4, 5, 6, 7)