livie
/
phoenix-firestorm
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

DRTVWR-493: Defend LL[Param]Singleton against ctor/init exceptions. 

An exception in the LLSingleton subclass constructor, or in its
initSingleton() method, could leave the LLSingleton machinery in a bad state:
the failing instance would remain in the MasterList, also on the stack of
initializing LLSingletons. Catch exceptions in either and perform relevant
cleanup.

This problem is highlighted by test programs, in which LL_ERRS throws an
exception rather than crashing the whole process.

In the relevant catch clauses, clean up the initializing stack BEFORE logging.
Otherwise we get tangled up recording bogus dependencies.

Move capture_dependency() out of finishInitializing(): it must be called by
every valid getInstance() call, both from LLSingleton and LLParamSingleton.

Introduce new CONSTRUCTED EInitState value to distinguish "have called the
constructor but not yet the initSingleton() method" from "currently within
initSingleton() method." This is transient, but we execute the 'switch' on
state within that moment. One could argue that the previous enum used
INITIALIZING for current CONSTRUCTED, and INITIALIZED meant INITIALIZING too,
but this is clearer.

Introduce template LLSingletonBase::classname() helper methods to clarify
verbose demangle(typeid(stuff).name()) calls.

Similarly, introduce LLSingleton::pop_initializing() shorthand method.
meow-7.2.2
Nat Goodspeed 2019-08-20 12:36:06 -04:00
parent 310db14bee
commit 54b98cb8c1
2 changed files with 99 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
indra/llcommon/llsingleton.cpp
View File

@ -150,7 +150,7 @@ void LLSingletonBase::pop_initializing()
if (list.empty())
{
logerrs("Underflow in stack of currently-initializing LLSingletons at ",
demangle(typeid(*this).name()).c_str(), "::getInstance()");
classname(this).c_str(), "::getInstance()");
}
// Now we know list.back() exists: capture it
@ -172,8 +172,8 @@ void LLSingletonBase::pop_initializing()
if (back != this)
{
logerrs("Push/pop mismatch in stack of currently-initializing LLSingletons: ",
demangle(typeid(*this).name()).c_str(), "::getInstance() trying to pop ",
demangle(typeid(*back).name()).c_str());
classname(this).c_str(), "::getInstance() trying to pop ",
classname(back).c_str());
}
// log AFTER popping so logging singletons don't cry circularity
@ -216,7 +216,7 @@ void LLSingletonBase::log_initializing(const char* verb, const char* name)
ri != rend; ++ri)
{
LLSingletonBase* sb(*ri);
LL_CONT << ' ' << demangle(typeid(*sb).name());
LL_CONT << ' ' << classname(sb);
}
LL_ENDL;
}
@ -250,7 +250,7 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt
// 'found' is an iterator; *found is an LLSingletonBase*; **found
// is the actual LLSingletonBase instance.
LLSingletonBase* foundp(*found);
out << demangle(typeid(*foundp).name()) << " -> ";
out << classname(foundp) << " -> ";
}
// We promise to capture dependencies from both the constructor
// and the initSingleton() method, so an LLSingleton's instance
@ -264,7 +264,7 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt
if (initState == CONSTRUCTING)
{
logerrs("LLSingleton circularity in Constructor: ", out.str().c_str(),
demangle(typeid(*this).name()).c_str(), "");
classname(this).c_str(), "");
}
else if (it_next == initializing.end())
{
@ -275,14 +275,14 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt
// Example: LLNotifications singleton initializes default channels.
// Channels register themselves with singleton once done.
logdebugs("LLSingleton circularity: ", out.str().c_str(),
demangle(typeid(*this).name()).c_str(), "");
classname(this).c_str(), "");
}
else
{
// Actual circularity with other singleton (or single singleton is used extensively).
// Dependency can be unclear.
logwarns("LLSingleton circularity: ", out.str().c_str(),
demangle(typeid(*this).name()).c_str(), "");
classname(this).c_str(), "");
}
}
else
@ -295,8 +295,8 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt
if (current->mDepends.insert(this).second)
{
// only log the FIRST time we hit this dependency!
logdebugs(demangle(typeid(*current).name()).c_str(),
" depends on ", demangle(typeid(*this).name()).c_str());
logdebugs(classname(current).c_str(),
" depends on ", classname(this).c_str());
}
}
}
@ -355,19 +355,19 @@ void LLSingletonBase::cleanupAll()
sp->mCleaned = true;
logdebugs("calling ",
demangle(typeid(*sp).name()).c_str(), "::cleanupSingleton()");
classname(sp).c_str(), "::cleanupSingleton()");
try
{
sp->cleanupSingleton();
}
catch (const std::exception& e)
{
logwarns("Exception in ", demangle(typeid(*sp).name()).c_str(),
logwarns("Exception in ", classname(sp).c_str(),
"::cleanupSingleton(): ", e.what());
}
catch (...)
{
logwarns("Unknown exception in ", demangle(typeid(*sp).name()).c_str(),
logwarns("Unknown exception in ", classname(sp).c_str(),
"::cleanupSingleton()");
}
}
@ -382,7 +382,7 @@ void LLSingletonBase::deleteAll()
{
// Capture the class name first: in case of exception, don't count on
// being able to extract it later.
const std::string name = demangle(typeid(*sp).name());
const std::string name = classname(sp);
try
{
// Call static method through instance function pointer.
@ -459,7 +459,17 @@ void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, co
log(LLError::LEVEL_ERROR, p1, p2, p3, p4);
// The other important side effect of LL_ERRS() is
// https://www.youtube.com/watch?v=OMG7paGJqhQ (emphasis on OMG)
LLError::crashAndLoop(std::string());
std::ostringstream out;
out << p1 << p2 << p3 << p4;
auto crash{ LLError::getFatalFunction() };
if (crash)
{
crash(out.str());
}
else
{
LLError::crashAndLoop(out.str());
}
}
std::string LLSingletonBase::demangle(const char* mangled)

111
indra/llcommon/llsingleton.h
View File

@ -68,10 +68,11 @@ protected:
typedef enum e_init_state
{
UNINITIALIZED = 0, // must be default-initialized state
CONSTRUCTING,
INITIALIZING,
INITIALIZED,
DELETED
CONSTRUCTING, // within DERIVED_TYPE constructor
CONSTRUCTED, // finished DERIVED_TYPE constructor
INITIALIZING, // within DERIVED_TYPE::initSingleton()
INITIALIZED, // normal case
DELETED // deleteSingleton() or deleteAll() called
} EInitState;
// Define tag<T> to pass to our template constructor. You can't explicitly
@ -129,6 +130,10 @@ protected:
static void logwarns(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()); }
template <typename T>
static std::string classname(T* ptr) { return demangle(typeid(*ptr).name()); }
// Default methods in case subclass doesn't declare them.
virtual void initSingleton() {}
@ -317,25 +322,28 @@ private:
template <typename... Args>
static void constructSingleton(Args&&... args)
{
sData.mInitState = CONSTRUCTING;
auto prev_size = LLSingleton_manage_master<DERIVED_TYPE>().get_initializing().size();
// getInstance() calls are from within constructor
sData.mInitState = CONSTRUCTING;
try
{
sData.mInstance = new DERIVED_TYPE(std::forward<Args>(args)...);
sData.mInitState = INITIALIZING;
// we have called constructor, have not yet called initSingleton()
sData.mInitState = CONSTRUCTED;
}
catch (const std::exception& err)
{
logwarns("Error constructing ", demangle(typeid(DERIVED_TYPE).name()).c_str(),
// LLSingletonBase might -- or might not -- have pushed the new
// instance onto the init stack before the exception. Reset the
// init stack to its previous size BEFORE logging so log-machinery
// LLSingletons don't record a dependency on DERIVED_TYPE!
LLSingleton_manage_master<DERIVED_TYPE>().reset_initializing(prev_size);
logwarns("Error constructing ", classname<DERIVED_TYPE>().c_str(),
": ", err.what());
// 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;
// LLSingletonBase might -- or might not -- have pushed the new
// instance onto the init stack before the exception. Reset the
// init stack to its previous size.
LLSingleton_manage_master<DERIVED_TYPE>().reset_initializing(prev_size);
// propagate the exception
throw;
}
@ -343,28 +351,27 @@ private:
static void finishInitializing()
{
// go ahead and flag ourselves as initialized so we can be
// reentrant during initialization
sData.mInitState = INITIALIZED;
// initialize singleton after constructing it so that it can
// reference other singletons which in turn depend on it, thus
// breaking cyclic dependencies
// getInstance() calls are from within initSingleton()
sData.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;
// pop this off stack of initializing singletons
LLSingleton_manage_master<DERIVED_TYPE>().pop_initializing(sData.mInstance);
// record the dependency, if any
capture_dependency();
pop_initializing();
}
catch (const std::exception& err)
{
logwarns("Error in ", demangle(typeid(DERIVED_TYPE).name()).c_str(),
// 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();
logwarns("Error in ", classname<DERIVED_TYPE>().c_str(),
"::initSingleton(): ", err.what());
// pop this off stack of initializing singletons here, too
LLSingleton_manage_master<DERIVED_TYPE>().pop_initializing(sData.mInstance);
// and get rid of the instance entirely
deleteSingleton();
// propagate the exception
@ -372,6 +379,13 @@ private:
}
}
static void pop_initializing()
{
// route through LLSingleton_manage_master so we Do The Right Thing
// (namely, nothing) for MasterList
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;
@ -458,8 +472,7 @@ public:
static void deleteSingleton()
{
delete sData.mInstance;
sData.mInstance = NULL;
sData.mInitState = DELETED;
// SingletonData state handled by destructor, above
}
static DERIVED_TYPE* getInstance()
@ -472,38 +485,46 @@ public:
case UNINITIALIZED:
// should never be uninitialized at this point
logerrs("Uninitialized singleton ",
demangle(typeid(DERIVED_TYPE).name()).c_str());
classname<DERIVED_TYPE>().c_str());
return NULL;
case CONSTRUCTING:
// here if DERIVED_TYPE's constructor (directly or indirectly)
// calls DERIVED_TYPE::getInstance()
logerrs("Tried to access singleton ",
demangle(typeid(DERIVED_TYPE).name()).c_str(),
classname<DERIVED_TYPE>().c_str(),
" from singleton constructor!");
return NULL;
case INITIALIZING:
// first time through: set to INITIALIZING by
// constructSingleton(), called by sInitializer's constructor
case CONSTRUCTED:
// first time through: set to CONSTRUCTED by
// constructSingleton(), called by sInitializer's constructor;
// still have to call initSingleton()
finishInitializing();
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
capture_dependency();
break;
case DELETED:
// called after deleteSingleton()
logwarns("Trying to access deleted singleton ",
demangle(typeid(DERIVED_TYPE).name()).c_str(),
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;
}
// record the dependency, if any: check if we got here from another
// LLSingleton's constructor or initSingleton() method
capture_dependency();
return sData.mInstance;
}
@ -589,7 +610,7 @@ public:
if (super::sData.mInitState != super::UNINITIALIZED)
{
super::logerrs("Tried to initialize singleton ",
super::demangle(typeid(DERIVED_TYPE).name()).c_str(),
super::template classname<DERIVED_TYPE>().c_str(),
" twice!");
}
else
@ -609,22 +630,38 @@ public:
{
case super::UNINITIALIZED:
super::logerrs("Uninitialized param singleton ",
super::demangle(typeid(DERIVED_TYPE).name()).c_str());
super::template classname<DERIVED_TYPE>().c_str());
break;
case super::CONSTRUCTING:
super::logerrs("Tried to access param singleton ",
super::demangle(typeid(DERIVED_TYPE).name()).c_str(),
super::template classname<DERIVED_TYPE>().c_str(),
" from singleton constructor!");
break;
case super::CONSTRUCTED:
// Should never happen!? The CONSTRUCTED state is specifically to
// navigate through LLSingleton::SingletonInitializer getting
// constructed (once) before LLSingleton::getInstance()'s switch
// on mInitState. But our initParamSingleton() method calls
// constructSingleton() and then calls finishInitializing(), which
// immediately sets INITIALIZING. Why are we here?
super::logerrs("Param singleton ",
super::template classname<DERIVED_TYPE>().c_str(),
"::initSingleton() not yet called");
break;
case super::INITIALIZING:
// As with LLSingleton, explicitly permit circular calls from
// within initSingleton()
case super::INITIALIZED:
// for any valid call, capture dependencies
super::capture_dependency();
return super::sData.mInstance;
case super::DELETED:
super::logerrs("Trying to access deleted param singleton ",
super::demangle(typeid(DERIVED_TYPE).name()).c_str());
super::template classname<DERIVED_TYPE>().c_str());
break;
}