DRTVWR-493: Permit LLParamSingleton::initSingleton() circularity.

This was forbidden, but AndreyK points out cases in which LLParamSingleton::
initSingleton() should in fact be allowed to circle back to its own instance()
method. Use a recursive_mutex instead of plain mutex to permit that; remove
LL_ERRS preventing it.

Add LLParamSingleton::instance() method that calls
LLParamSingleton::getInstance(). Inheriting LLSingleton::instance() called
LLSingleton::getInstance() -- not at all what we want.

Add LLParamSingleton unit tests.

2 files changed, 144 insertions, 15 deletions

diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h
index 38d5af5309..03b7d5a349 100644
--- a/indra/llcommon/llsingleton.h
+++ b/indra/llcommon/llsingleton.h
@@ -491,9 +491,6 @@ typename LLSingleton<T>::SingletonData LLSingleton<T>::sData;
  * * However, distinct initParamSingleton() calls can be used to engage
  *   different constructors, as long as only one such call is executed at
  *   runtime.
- * * Circularity is not permitted. No LLSingleton referenced by an
- *   LLParamSingleton's constructor or initSingleton() method may call this
- *   LLParamSingleton's instance() or getInstance() methods.
  * * Unlike LLSingleton, an LLParamSingleton cannot be "revived" by an
  *   instance() or getInstance() call after deleteSingleton().
  *
@@ -508,7 +505,6 @@ private:
 
 public:
     using super::deleteSingleton;
-    using super::instance;
     using super::instanceExists;
     using super::wasDeleted;
 
@@ -519,7 +515,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<std::mutex> lk(mMutex);
+        std::unique_lock<decltype(mMutex)> lk(mMutex);
         // For organizational purposes this function shouldn't be called twice
         if (super::sData.mInitState != super::UNINITIALIZED)
         {
@@ -538,7 +534,7 @@ public:
     {
         // In case racing threads call getInstance() at the same moment as
         // initParamSingleton(), serialize the calls.
-        std::unique_lock<std::mutex> lk(mMutex);
+        std::unique_lock<decltype(mMutex)> lk(mMutex);
 
         switch (super::sData.mInitState)
         {
@@ -550,15 +546,10 @@ public:
         case super::CONSTRUCTING:
             super::logerrs("Tried to access param singleton ",
                            super::demangle(typeid(DERIVED_TYPE).name()).c_str(),
-                " from singleton constructor!");
+                           " from singleton constructor!");
             break;
 
         case super::INITIALIZING:
-            super::logerrs("Tried to access param singleton ",
-                           super::demangle(typeid(DERIVED_TYPE).name()).c_str(),
-                           " from initSingleton() method!");
-            break;
-
         case super::INITIALIZED:
             return super::sData.mInstance;
 
@@ -573,12 +564,23 @@ public:
         return nullptr;
     }
 
+    // instance() is replicated here so it calls
+    // LLParamSingleton::getInstance() rather than LLSingleton::getInstance()
+    // -- avoid making getInstance() virtual
+    static DERIVED_TYPE& instance()
+    {
+        return *getInstance();
+    }
+
 private:
-    static std::mutex mMutex;
+    // Use a recursive_mutex in case of constructor circularity. With a
+    // non-recursive mutex, that would result in deadlock rather than the
+    // logerrs() call coded above.
+    static std::recursive_mutex mMutex;
 };
 
 template<typename T>
-typename std::mutex LLParamSingleton<T>::mMutex;
+typename std::recursive_mutex LLParamSingleton<T>::mMutex;
 
 /**
  * Initialization locked singleton, only derived class can decide when to initialize.
diff --git a/indra/llcommon/tests/llsingleton_test.cpp b/indra/llcommon/tests/llsingleton_test.cpp
index 56886bc73f..da7bc6355c 100644
--- a/indra/llcommon/tests/llsingleton_test.cpp
+++ b/indra/llcommon/tests/llsingleton_test.cpp
@@ -29,7 +29,8 @@
 
 #include "llsingleton.h"
 #include "../test/lltut.h"
-
+#include "wrapllerrs.h"
+#include "llsd.h"
 
 // Capture execution sequence by appending to log string.
 std::string sLog;
@@ -198,4 +199,130 @@ namespace tut
 
     TESTS(A, B, 4, 5, 6, 7)
     TESTS(B, A, 8, 9, 10, 11)
+
+#define PARAMSINGLETON(cls)                                             \
+    class cls: public LLParamSingleton<cls>                             \
+    {                                                                   \
+        LLSINGLETON(cls, const LLSD::String& str): mDesc(str) {}        \
+        cls(LLSD::Integer i): mDesc(i) {}                               \
+                                                                        \
+    public:                                                             \
+        std::string desc() const { return mDesc.asString(); }           \
+                                                                        \
+    private:                                                            \
+        LLSD mDesc;                                                     \
+    }
+
+    // Declare two otherwise-identical LLParamSingleton classes so we can
+    // validly initialize each using two different constructors. If we tried
+    // to test that with a single LLParamSingleton class within the same test
+    // program, we'd get 'trying to use deleted LLParamSingleton' errors.
+    PARAMSINGLETON(PSing1);
+    PARAMSINGLETON(PSing2);
+
+    template<> template<>
+    void singleton_object_t::test<12>()
+    {
+        set_test_name("LLParamSingleton");
+
+        WrapLLErrs catcherr;
+        // query methods
+        ensure("false positive on instanceExists()", ! PSing1::instanceExists());
+        ensure("false positive on wasDeleted()", ! PSing1::wasDeleted());
+        // try to reference before initializing
+        std::string threw = catcherr.catch_llerrs([](){
+                (void)PSing1::instance();
+            });
+        ensure_contains("too-early instance() didn't throw", threw, "Uninitialized");
+        // getInstance() behaves the same as instance()
+        threw = catcherr.catch_llerrs([](){
+                (void)PSing1::getInstance();
+            });
+        ensure_contains("too-early getInstance() didn't throw", threw, "Uninitialized");
+        // initialize using LLSD::String constructor
+        PSing1::initParamSingleton("string");
+        ensure_equals(PSing1::instance().desc(), "string");
+        ensure("false negative on instanceExists()", PSing1::instanceExists());
+        // try to initialize again
+        threw = catcherr.catch_llerrs([](){
+                PSing1::initParamSingleton("again");
+            });
+        ensure_contains("second ctor(string) didn't throw", threw, "twice");
+        // try to initialize using the other constructor -- should be
+        // well-formed, but illegal at runtime
+        threw = catcherr.catch_llerrs([](){
+                PSing1::initParamSingleton(17);
+            });
+        ensure_contains("other ctor(int) didn't throw", threw, "twice");
+        PSing1::deleteSingleton();
+        ensure("false negative on wasDeleted()", PSing1::wasDeleted());
+        threw = catcherr.catch_llerrs([](){
+                (void)PSing1::instance();
+            });
+        ensure_contains("accessed deleted LLParamSingleton", threw, "deleted");
+    }
+
+    template<> template<>
+    void singleton_object_t::test<13>()
+    {
+        set_test_name("LLParamSingleton alternate ctor");
+
+        WrapLLErrs catcherr;
+        // We don't have to restate all the tests for PSing1. Only test validly
+        // using the other constructor.
+        PSing2::initParamSingleton(17);
+        ensure_equals(PSing2::instance().desc(), "17");
+        // can't do it twice
+        std::string threw = catcherr.catch_llerrs([](){
+                PSing2::initParamSingleton(34);
+            });
+        ensure_contains("second ctor(int) didn't throw", threw, "twice");
+        // can't use the other constructor either
+        threw = catcherr.catch_llerrs([](){
+                PSing2::initParamSingleton("string");
+            });
+        ensure_contains("other ctor(string) didn't throw", threw, "twice");
+    }
+
+    class CircularPCtor: public LLParamSingleton<CircularPCtor>
+    {
+        LLSINGLETON(CircularPCtor)
+        {
+            // never mind indirection, just go straight for the circularity
+            (void)instance();
+        }
+    };
+
+    template<> template<>
+    void singleton_object_t::test<14>()
+    {
+        set_test_name("Circular LLParamSingleton constructor");
+        WrapLLErrs catcherr;
+        std::string threw = catcherr.catch_llerrs([](){
+                CircularPCtor::initParamSingleton();
+            });
+        ensure_contains("constructor circularity didn't throw", threw, "constructor");
+    }
+
+    class CircularPInit: public LLParamSingleton<CircularPInit>
+    {
+        LLSINGLETON_EMPTY_CTOR(CircularPInit);
+    public:
+        virtual void initSingleton()
+        {
+            // never mind indirection, just go straight for the circularity
+            (void)instance();
+        }
+    };
+
+    template<> template<>
+    void singleton_object_t::test<15>()
+    {
+        set_test_name("Circular LLParamSingleton initSingleton()");
+        WrapLLErrs catcherr;
+        std::string threw = catcherr.catch_llerrs([](){
+                CircularPInit::initParamSingleton();
+            });
+        ensure("initSingleton() circularity threw", threw.empty());
+    }
 }