From 310db14beefc29ee72e0d13f0cb63cb2958ebf68 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 19 Aug 2019 11:44:56 -0400 Subject: DRTVWR-493: Improve exception safety of LLSingleton initialization. Add try/catch clauses to constructSingleton() (to catch exceptions in the subclass constructor) and finishInitializing() (to catch exceptions in the subclass initSingleton() method). Each of these catch clauses rethrows the exception -- they're for cleanup, not for ultimate handling. Introduce LLSingletonBase::reset_initializing(list_t::size_t). The idea is that since we can't know whether the exception happened before or after the push_initializing() call in LLSingletonBase's constructor, we can't just pop the stack. Instead, constructSingleton() captures the stack size before attempting to construct the new LLSingleton subclass. On exception, it calls reset_initializing() to restore the stack to that size. Naturally that requires a corresponding LLSingleton_manage_master method, whose MasterList specialization is a no-op. finishInitializing()'s exception handling is a bit simpler because it has a constructed LLSingleton subclass instance in hand, therefore push_initializing() has definitely been called, therefore it can call pop_initializing(). Break out new static capture_dependency() method from finishInitializing() because, in the previous LLSingleton::getInstance() implementation, the logic now wrapped in capture_dependency() was reached even in the INITIALIZED case. TODO: Add a new EInitState to differentiate "have been constructed, now calling initSingleton()" from "fully initialized, normal case" -- in the latter control path we should not be calling capture_dependency(). The LLSingleton_manage_master specialization's get_initializing() function (which called get_initializing_from()) was potentially dangerous. get_initializing() is called by push_initializing(), which (in the general case) is called by LLSingletonBase's constructor. If somehow the MasterList's LLSingletonBase constructor ended up calling get_initializing(), it would have called get_initializing_from(), passing an LLSingletonBase which had not yet been constructed into the MasterList. In particular, its mInitializing map would not yet have been initialized at all. Since the MasterList must not, by design, depend on any other LLSingletons, LLSingleton_manage_master::get_initializing() need not return a list from the official mInitializing map anyway. It can, and should, and now does, return a static dummy list. That obviates get_initializing_from(), which is removed. That in turn means we no longer need to pass get_initializing() an LLSingletonBase*. Remove that parameter. --- indra/llcommon/llsingleton.cpp | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) (limited to 'indra/llcommon/llsingleton.cpp') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 9fbd78a000..adf72bf700 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -134,12 +134,6 @@ LLSingletonBase::list_t& LLSingletonBase::get_initializing() return LLSingletonBase::MasterList::instance().get_initializing_(); } -//static -LLSingletonBase::list_t& LLSingletonBase::get_initializing_from(MasterList* master) -{ - return master->get_initializing_(); -} - LLSingletonBase::~LLSingletonBase() {} void LLSingletonBase::push_initializing(const char* name) @@ -186,6 +180,31 @@ void LLSingletonBase::pop_initializing() log_initializing("Popping", typeid(*back).name()); } +void LLSingletonBase::reset_initializing(list_t::size_type size) +{ + // called for cleanup in case the LLSingleton subclass constructor throws + // an exception + + // The tricky thing about this, the reason we have a separate method + // instead of just calling pop_initializing(), is (hopefully remote) + // possibility that the exception happened *before* the + // push_initializing() call in LLSingletonBase's constructor. So only + // remove the stack top if in fact we've pushed something more than the + // previous size. + list_t& list(get_initializing()); + + while (list.size() > size) + { + list.pop_back(); + } + + // as in pop_initializing() + if (list.empty()) + { + MasterList::instance().cleanup_initializing_(); + } +} + //static void LLSingletonBase::log_initializing(const char* verb, const char* name) { -- cgit v1.2.3 From 54b98cb8c1302a1da5779d8968e54e5be641e644 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2019 12:36:06 -0400 Subject: 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. --- indra/llcommon/llsingleton.cpp | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) (limited to 'indra/llcommon/llsingleton.cpp') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index adf72bf700..88527a3d31 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -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) -- cgit v1.2.3 From 5b5eb55f0c999a55cd0df1dd983629967d447b6c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Aug 2019 12:48:21 -0400 Subject: DRTVWR-493: Clarify capturing LLError::getFatalFunction() in a var. VS 2013 thought we were storing an initialization-list. --- indra/llcommon/llsingleton.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llcommon/llsingleton.cpp') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 88527a3d31..c45c144570 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -461,7 +461,7 @@ void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, co // https://www.youtube.com/watch?v=OMG7paGJqhQ (emphasis on OMG) std::ostringstream out; out << p1 << p2 << p3 << p4; - auto crash{ LLError::getFatalFunction() }; + auto crash = LLError::getFatalFunction(); if (crash) { crash(out.str()); -- cgit v1.2.3