From 04c3f2b0f0e6cc2a2a64007ae179072ed45102b5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Mar 2017 13:58:59 -0400 Subject: DRTVWR-418: Tweak shutdown sequence to avoid resurrecting singletons. The LLSingletonBase::deleteAll() call late in LLAppViewer::cleanup() deletes the LLSingleton(s) used by the logging machinery, among other things. Attempting further logging after that call (such as our cheery "Goodbye!") has the unfortunate effect of attempting to resurrect the deleted LLSingleton(s). Move "Goodbye!" to just *before* the call. Also, given that call, the manual references to a couple specific LLSingletons in ~LLAppViewer() are (a) unnecessary and (b) cause attempted resurrection. Eliminate both. --- indra/newview/llappviewer.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index b58af54985..616e084119 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -738,10 +738,7 @@ LLAppViewer::LLAppViewer() LLAppViewer::~LLAppViewer() { delete mSettingsLocationList; - LLViewerEventRecorder::deleteSingleton(); - LLLoginInstance::instance().setUpdaterService(0); - destroyMainloopTimeout(); // If we got to this destructor somehow, the app didn't hang. @@ -2110,6 +2107,10 @@ bool LLAppViewer::cleanup() // realtime, or might throw an exception. LLSingletonBase::cleanupAll(); + // The logging subsystem depends on an LLSingleton. Any logging after + // LLSingletonBase::deleteAll() won't be recorded. + LL_INFOS() << "Goodbye!" << LL_ENDL; + // This calls every remaining LLSingleton's deleteSingleton() method. // No class destructor should perform any cleanup that might take // significant realtime, or throw an exception. @@ -2122,8 +2123,6 @@ bool LLAppViewer::cleanup() // probably useful to be able to log that order. LLSingletonBase::deleteAll(); - LL_INFOS() << "Goodbye!" << LL_ENDL; - removeDumpDir(); // return 0; -- cgit v1.2.3 From 1a8c8df6862620de64f621363b025b0ffbef72fa Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Mar 2017 14:09:14 -0400 Subject: DRTVWR-418: Ignore logging that requires resurrecting singletons. The logging subsystem depends on two different LLSingletons for some reason. It turns out to be very difficult to completely avoid executing any logging calls after the LLSingletonBase::deleteAll(), but we really don't want to resurrect those LLSingletons so late in the run for a couple stragglers. Introduce LLSingleton::wasDeleted() query method, and use it in logging subsystem to simply bypass last-millisecond logging requests. --- indra/llcommon/llerror.cpp | 97 ++++++++++++++++++++++++++++---------------- indra/llcommon/llsingleton.h | 8 ++++ 2 files changed, 70 insertions(+), 35 deletions(-) diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp index e6407ecf22..2ddb3edbdd 100644 --- a/indra/llcommon/llerror.cpp +++ b/indra/llcommon/llerror.cpp @@ -1067,7 +1067,15 @@ namespace LLError { return false; } - + + // If we hit a logging request very late during shutdown processing, + // when either of the relevant LLSingletons has already been deleted, + // DO NOT resurrect them. + if (Settings::wasDeleted() || Globals::wasDeleted()) + { + return false; + } + SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); s->mShouldLogCallCounter++; @@ -1106,7 +1114,10 @@ namespace LLError std::ostringstream* Log::out() { LogLock lock; - if (lock.ok()) + // If we hit a logging request very late during shutdown processing, + // when either of the relevant LLSingletons has already been deleted, + // DO NOT resurrect them. + if (lock.ok() && ! (Settings::wasDeleted() || Globals::wasDeleted())) { Globals* g = Globals::getInstance(); @@ -1116,41 +1127,49 @@ namespace LLError return &g->messageStream; } } - + return new std::ostringstream; } - + void Log::flush(std::ostringstream* out, char* message) - { - LogLock lock; - if (!lock.ok()) - { - return; - } - - if(strlen(out->str().c_str()) < 128) - { - strcpy(message, out->str().c_str()); - } - else - { - strncpy(message, out->str().c_str(), 127); - message[127] = '\0' ; - } - - Globals* g = Globals::getInstance(); - if (out == &g->messageStream) - { - g->messageStream.clear(); - g->messageStream.str(""); - g->messageStreamInUse = false; - } - else - { - delete out; - } - return ; - } + { + LogLock lock; + if (!lock.ok()) + { + return; + } + + // If we hit a logging request very late during shutdown processing, + // when either of the relevant LLSingletons has already been deleted, + // DO NOT resurrect them. + if (Settings::wasDeleted() || Globals::wasDeleted()) + { + return; + } + + if(strlen(out->str().c_str()) < 128) + { + strcpy(message, out->str().c_str()); + } + else + { + strncpy(message, out->str().c_str(), 127); + message[127] = '\0' ; + } + + Globals* g = Globals::getInstance(); + if (out == &g->messageStream) + { + g->messageStream.clear(); + g->messageStream.str(""); + g->messageStreamInUse = false; + } + else + { + delete out; + } + return ; + } void Log::flush(std::ostringstream* out, const CallSite& site) { @@ -1159,7 +1178,15 @@ namespace LLError { return; } - + + // If we hit a logging request very late during shutdown processing, + // when either of the relevant LLSingletons has already been deleted, + // DO NOT resurrect them. + if (Settings::wasDeleted() || Globals::wasDeleted()) + { + return; + } + Globals* g = Globals::getInstance(); SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 1b915dfd6e..0d4a1f34f8 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -452,6 +452,14 @@ public: return sData.mInitState == INITIALIZED; } + // Has this singleton been deleted? This can be useful during shutdown + // processing to avoid "resurrecting" a singleton we thought we'd already + // cleaned up. + static bool wasDeleted() + { + return sData.mInitState == DELETED; + } + private: struct SingletonData { -- cgit v1.2.3 From a33c5930cbfe9b2a29359d906d6b869e983a782a Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Mar 2017 14:19:50 -0400 Subject: DRTVWR-418: Use LLTempBoundListener to manage "mainloop" listener. LLUpdaterServiceImpl binds its onMainLoop() listener method to the "mainloop" event so it can wake up periodically to recheck for updates. (Suggests maybe a smarter conventional callback-on-timer facility with a central queue, instead of every interested party intercepting *every* frame...) ~LLUpdaterServiceImpl() was calling LLEventPumps::instance() only to disconnect that listener, which was resurrecting the deleted LLEventPumps instance. Instead store an LLTempBoundListener in LLUpdaterServiceImpl, the conventional way to implicitly disconnect on destroy. Use its disconnect() method when explicit disconnection is desired. --- indra/viewer_components/updater/llupdaterservice.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/indra/viewer_components/updater/llupdaterservice.cpp b/indra/viewer_components/updater/llupdaterservice.cpp index 1665e41e70..df021948c3 100644 --- a/indra/viewer_components/updater/llupdaterservice.cpp +++ b/indra/viewer_components/updater/llupdaterservice.cpp @@ -158,7 +158,8 @@ public: private: std::string mNewChannel; std::string mNewVersion; - + LLTempBoundListener mMainLoopConnection; + void restartTimer(unsigned int seconds); void setState(LLUpdaterService::eUpdaterState state); void stopTimer(); @@ -179,7 +180,8 @@ LLUpdaterServiceImpl::LLUpdaterServiceImpl() : LLUpdaterServiceImpl::~LLUpdaterServiceImpl() { LL_INFOS("UpdaterService") << "shutting down updater service" << LL_ENDL; - LLEventPumps::instance().obtain("mainloop").stopListening(sListenerName); + // Destroying an LLTempBoundListener implicitly disconnects. That's its + // whole purpose. } void LLUpdaterServiceImpl::initialize(const std::string& channel, @@ -560,7 +562,7 @@ void LLUpdaterServiceImpl::restartTimer(unsigned int seconds) seconds << " seconds" << LL_ENDL; mTimer.start(); mTimer.setTimerExpirySec((F32)seconds); - LLEventPumps::instance().obtain("mainloop").listen( + mMainLoopConnection = LLEventPumps::instance().obtain("mainloop").listen( sListenerName, boost::bind(&LLUpdaterServiceImpl::onMainLoop, this, _1)); } @@ -589,7 +591,7 @@ void LLUpdaterServiceImpl::setState(LLUpdaterService::eUpdaterState state) void LLUpdaterServiceImpl::stopTimer() { mTimer.stop(); - LLEventPumps::instance().obtain("mainloop").stopListening(sListenerName); + mMainLoopConnection.disconnect(); } bool LLUpdaterServiceImpl::onMainLoop(LLSD const & event) -- cgit v1.2.3 From e6fc3528fdfd2a251571ef86f321e321865d4592 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Mar 2017 14:22:19 -0400 Subject: DRTVWR-418: #include "llrefcount.h" : LLTombStone uses LLRefCount. Apparently we've been getting away so far without this essential #include only by "leakage" from other #includes in existing consumers. --- indra/llcommon/llhandle.h | 1 + 1 file changed, 1 insertion(+) diff --git a/indra/llcommon/llhandle.h b/indra/llcommon/llhandle.h index feb5f41848..570cd330b8 100644 --- a/indra/llcommon/llhandle.h +++ b/indra/llcommon/llhandle.h @@ -28,6 +28,7 @@ #define LLHANDLE_H #include "llpointer.h" +#include "llrefcount.h" #include "llexception.h" #include #include -- cgit v1.2.3 From c1458713dea2ac8cec100628c0ca5238fcca93ba Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Mar 2017 14:31:38 -0400 Subject: DRTVWR-418: Make LLEventPumps an LLHandleProvider for LLEventPump. LLEventPump's destructor was using LLEventPumps::instance() to unregister the LLEventPump instance from LLEventPumps. Evidently, though, there are lingering LLEventPump instances that persist even after the LLSingletonBase::deleteAll() call destroys the LLEventPumps LLSingleton instance. These were resurrecting LLEventPumps -- pointlessly, since a newly-resurrected LLEventPumps instance can have no knowledge of the LLEventPump instance! Unregistering is unnecessary! What we want is a reference we can bind into each LLEventPump instance that allows us to safely test whether the LLEventPumps instance still exists. LLHandle is exactly that. Make LLEventPumps an LLHandleProvider and bind its LLHandle in each LLEventPump's constructor; then the destructor can unregister only when LLEventPumps still exists. --- indra/llcommon/llevents.cpp | 12 +++++++++--- indra/llcommon/llevents.h | 18 +++++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp index 97270e4931..a3856e4fc4 100644 --- a/indra/llcommon/llevents.cpp +++ b/indra/llcommon/llevents.cpp @@ -281,7 +281,8 @@ const std::string LLEventPump::ANONYMOUS = std::string(); LLEventPump::LLEventPump(const std::string& name, bool tweak): // Register every new instance with LLEventPumps - mName(LLEventPumps::instance().registerNew(*this, name, tweak)), + mRegistry(LLEventPumps::instance().getHandle()), + mName(mRegistry.get()->registerNew(*this, name, tweak)), mSignal(new LLStandardSignal()), mEnabled(true) {} @@ -292,8 +293,13 @@ LLEventPump::LLEventPump(const std::string& name, bool tweak): LLEventPump::~LLEventPump() { - // Unregister this doomed instance from LLEventPumps - LLEventPumps::instance().unregister(*this); + // Unregister this doomed instance from LLEventPumps -- but only if + // LLEventPumps is still around! + LLEventPumps* registry = mRegistry.get(); + if (registry) + { + registry->unregister(*this); + } } // static data member diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h index 7cff7dfd45..1d51c660ed 100644 --- a/indra/llcommon/llevents.h +++ b/indra/llcommon/llevents.h @@ -62,6 +62,7 @@ #include "lldependencies.h" #include "llstl.h" #include "llexception.h" +#include "llhandle.h" /*==========================================================================*| // override this to allow binding free functions with more parameters @@ -227,7 +228,15 @@ class LLEventPump; * LLEventPumps is a Singleton manager through which one typically accesses * this subsystem. */ -class LL_COMMON_API LLEventPumps: public LLSingleton +// LLEventPumps isa LLHandleProvider only for (hopefully rare) long-lived +// class objects that must refer to this class late in their lifespan, say in +// the destructor. Specifically, the case that matters is a possible reference +// after LLEventPumps::deleteSingleton(). (Lingering LLEventPump instances are +// capable of this.) In that case, instead of calling LLEventPumps::instance() +// again -- resurrecting the deleted LLSingleton -- store an +// LLHandle and test it before use. +class LL_COMMON_API LLEventPumps: public LLSingleton, + public LLHandleProvider { LLSINGLETON(LLEventPumps); public: @@ -590,6 +599,9 @@ private: return this->listen_impl(name, listener, after, before); } + // must precede mName; see LLEventPump::LLEventPump() + LLHandle mRegistry; + std::string mName; protected: @@ -817,14 +829,14 @@ public: mConnection(new LLBoundListener) { } - + /// Copy constructor. Copy shared_ptrs to original instance data. LLListenerWrapperBase(const LLListenerWrapperBase& that): mName(that.mName), mConnection(that.mConnection) { } - virtual ~LLListenerWrapperBase() {} + virtual ~LLListenerWrapperBase() {} /// Ask LLEventPump::listen() for the listener name virtual void accept_name(const std::string& name) const -- cgit v1.2.3