From 4fce6dc4353dbf9ccd3c9c3aced89df72a4f3abd Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Mon, 12 Aug 2019 17:35:45 -0400
Subject: [PATCH] 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.
---
indra/llcommon/llsingleton.h | 30 ++---
indra/llcommon/tests/llsingleton_test.cpp | 129 +++++++++++++++++++++-
2 files changed, 144 insertions(+), 15 deletions(-)
diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h
index 38d5af5309e..03b7d5a349c 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 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE>
public:
using super::deleteSingleton;
- using super::instance;
using super::instanceExists;
using super::wasDeleted;
@@ -519,7 +515,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<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 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE>
{
// 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 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE>
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 @@ class LLParamSingleton : public LLSingleton<DERIVED_TYPE>
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 56886bc73fd..da7bc6355c9 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());
+ }
}
--
GitLab