Merge branch 'master' into DRTVWR-483

1 files changed, 159 insertions, 108 deletions

diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp
index c45c144570..d3d25201b2 100644
--- a/indra/llcommon/llsingleton.cpp
+++ b/indra/llcommon/llsingleton.cpp
@@ -30,9 +30,9 @@
 #include "llerror.h"
 #include "llerrorcontrol.h"         // LLError::is_available()
 #include "lldependencies.h"
-#include "llcoro_get_id.h"
+#include "llexception.h"
+#include "llcoros.h"
 #include <boost/foreach.hpp>
-#include <boost/unordered_map.hpp>
 #include <algorithm>
 #include <iostream>                 // std::cerr in dire emergency
 #include <sstream>
@@ -42,8 +42,6 @@ namespace {
 void log(LLError::ELevel level,
          const char* p1, const char* p2, const char* p3, const char* p4);
 
-void logdebugs(const char* p1="", const char* p2="", const char* p3="", const char* p4="");
-
 bool oktolog();
 } // anonymous namespace
 
@@ -57,63 +55,131 @@ bool oktolog();
 class LLSingletonBase::MasterList:
     public LLSingleton<LLSingletonBase::MasterList>
 {
+private:
     LLSINGLETON_EMPTY_CTOR(MasterList);
 
-public:
-    // No need to make this private with accessors; nobody outside this source
-    // file can see it.
+    // Independently of the LLSingleton locks governing construction,
+    // destruction and other state changes of the MasterList instance itself,
+    // we must also defend each of the data structures owned by the
+    // MasterList.
+    // This must be a recursive_mutex because, while the lock is held for
+    // manipulating some data in the master list, we must also check whether
+    // it's safe to log -- which involves querying a different LLSingleton --
+    // which requires accessing the master list.
+    typedef std::recursive_mutex mutex_t;
+    typedef std::unique_lock<mutex_t> lock_t;
 
+    mutex_t mMutex;
+
+public:
+    // Instantiate this to both obtain a reference to MasterList::instance()
+    // and lock its mutex for the lifespan of this Lock instance.
+    class Lock
+    {
+    public:
+        Lock():
+            mMasterList(MasterList::instance()),
+            mLock(mMasterList.mMutex)
+        {}
+        Lock(const Lock&) = delete;
+        Lock& operator=(const Lock&) = delete;
+        MasterList& get() const { return mMasterList; }
+        operator MasterList&() const { return get(); }
+
+    protected:
+        MasterList& mMasterList;
+        MasterList::lock_t mLock;
+    };
+
+private:
     // This is the master list of all instantiated LLSingletons (save the
     // MasterList itself) in arbitrary order. You MUST call dep_sort() before
     // traversing this list.
-    LLSingletonBase::list_t mMaster;
+    list_t mMaster;
+
+public:
+    // Instantiate this to obtain a reference to MasterList::mMaster and to
+    // hold the MasterList lock for the lifespan of this LockedMaster
+    // instance.
+    struct LockedMaster: public Lock
+    {
+        list_t& get() const { return mMasterList.mMaster; }
+        operator list_t&() const { return get(); }
+    };
 
+private:
     // We need to maintain a stack of LLSingletons currently being
     // initialized, either in the constructor or in initSingleton(). However,
     // managing that as a stack depends on having a DISTINCT 'initializing'
     // stack for every C++ stack in the process! And we have a distinct C++
-    // stack for every running coroutine. It would be interesting and cool to
-    // implement a generic coroutine-local-storage mechanism and use that
-    // here. The trouble is that LLCoros is itself an LLSingleton, so
-    // depending on LLCoros functionality could dig us into infinite
-    // recursion. (Moreover, when we reimplement LLCoros on top of
-    // Boost.Fiber, that library already provides fiber_specific_ptr -- so
-    // it's not worth a great deal of time and energy implementing a generic
-    // equivalent on top of boost::dcoroutine, which is on its way out.)
-    // Instead, use a map of llcoro::id to select the appropriate
-    // coro-specific 'initializing' stack. llcoro::get_id() is carefully
-    // implemented to avoid requiring LLCoros.
-    typedef boost::unordered_map<llcoro::id, LLSingletonBase::list_t> InitializingMap;
-    InitializingMap mInitializing;
-
-    // non-static method, cf. LLSingletonBase::get_initializing()
+    // stack for every running coroutine. Therefore this stack must be based
+    // on a coroutine-local pointer.
+    // This local_ptr isn't static because it's a member of an LLSingleton.
+    LLCoros::local_ptr<list_t> mInitializing;
+
+public:
+    // Instantiate this to obtain a reference to the coroutine-specific
+    // initializing list and to hold the MasterList lock for the lifespan of
+    // this LockedInitializing instance.
+    struct LockedInitializing: public Lock
+    {
+    public:
+        LockedInitializing():
+            // only do the lookup once, cache the result
+            // note that the lock is already locked during this lookup
+            mList(&mMasterList.get_initializing_())
+        {}
+        list_t& get() const
+        {
+            if (! mList)
+            {
+                LLTHROW(LLException("Trying to use LockedInitializing "
+                                    "after cleanup_initializing()"));
+            }
+            return *mList;
+        }
+        operator list_t&() const { return get(); }
+        void log(const char* verb, const char* name);
+        void cleanup_initializing()
+        {
+            mMasterList.cleanup_initializing_();
+            mList = nullptr;
+        }
+
+    private:
+        // Store pointer since cleanup_initializing() must clear it.
+        list_t* mList;
+    };
+
+private:
     list_t& get_initializing_()
     {
-        // map::operator[] has find-or-create semantics, exactly what we need
-        // here. It returns a reference to the selected mapped_type instance.
-        return mInitializing[llcoro::get_id()];
+        LLSingletonBase::list_t* current = mInitializing.get();
+        if (! current)
+        {
+            // If the running coroutine doesn't already have an initializing
+            // stack, allocate a new one and save it for future reference.
+            current = new LLSingletonBase::list_t();
+            mInitializing.reset(current);
+        }
+        return *current;
     }
 
+    // By the time mInitializing is destroyed, its value for every coroutine
+    // except the running one must have been reset() to nullptr. So every time
+    // we pop the list to empty, reset() the running coroutine's local_ptr.
     void cleanup_initializing_()
     {
-        InitializingMap::iterator found = mInitializing.find(llcoro::get_id());
-        if (found != mInitializing.end())
-        {
-            mInitializing.erase(found);
-        }
+        mInitializing.reset(nullptr);
     }
 };
 
-//static
-LLSingletonBase::list_t& LLSingletonBase::get_master()
-{
-    return LLSingletonBase::MasterList::instance().mMaster;
-}
-
 void LLSingletonBase::add_master()
 {
     // As each new LLSingleton is constructed, add to the master list.
-    get_master().push_back(this);
+    // This temporary LockedMaster should suffice to hold the MasterList lock
+    // during the push_back() call.
+    MasterList::LockedMaster().get().push_back(this);
 }
 
 void LLSingletonBase::remove_master()
@@ -125,27 +191,32 @@ void LLSingletonBase::remove_master()
     // master list, and remove this item IF FOUND. We have few enough
     // LLSingletons, and they are so rarely destroyed (once per run), that the
     // cost of a linear search should not be an issue.
-    get_master().remove(this);
+    // This temporary LockedMaster should suffice to hold the MasterList lock
+    // during the remove() call.
+    MasterList::LockedMaster().get().remove(this);
 }
 
 //static
-LLSingletonBase::list_t& LLSingletonBase::get_initializing()
+LLSingletonBase::list_t::size_type LLSingletonBase::get_initializing_size()
 {
-    return LLSingletonBase::MasterList::instance().get_initializing_();
+    return MasterList::LockedInitializing().get().size();
 }
 
 LLSingletonBase::~LLSingletonBase() {}
 
 void LLSingletonBase::push_initializing(const char* name)
 {
+    MasterList::LockedInitializing locked_list;
     // log BEFORE pushing so logging singletons don't cry circularity
-    log_initializing("Pushing", name);
-    get_initializing().push_back(this);
+    locked_list.log("Pushing", name);
+    locked_list.get().push_back(this);
 }
 
 void LLSingletonBase::pop_initializing()
 {
-    list_t& list(get_initializing());
+    // Lock the MasterList for the duration of this call
+    MasterList::LockedInitializing locked_list;
+    list_t& list(locked_list.get());
 
     if (list.empty())
     {
@@ -165,7 +236,7 @@ void LLSingletonBase::pop_initializing()
     // entirely.
     if (list.empty())
     {
-        MasterList::instance().cleanup_initializing_();
+        locked_list.cleanup_initializing();
     }
 
     // Now validate the newly-popped LLSingleton.
@@ -177,7 +248,7 @@ void LLSingletonBase::pop_initializing()
     }
 
     // log AFTER popping so logging singletons don't cry circularity
-    log_initializing("Popping", typeid(*back).name());
+    locked_list.log("Popping", typeid(*back).name());
 }
 
 void LLSingletonBase::reset_initializing(list_t::size_type size)
@@ -191,7 +262,8 @@ void LLSingletonBase::reset_initializing(list_t::size_type size)
     // 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());
+    MasterList::LockedInitializing locked_list;
+    list_t& list(locked_list.get());
 
     while (list.size() > size)
     {
@@ -201,29 +273,32 @@ void LLSingletonBase::reset_initializing(list_t::size_type size)
     // as in pop_initializing()
     if (list.empty())
     {
-        MasterList::instance().cleanup_initializing_();
+        locked_list.cleanup_initializing();
     }
 }
 
-//static
-void LLSingletonBase::log_initializing(const char* verb, const char* name)
+void LLSingletonBase::MasterList::LockedInitializing::log(const char* verb, const char* name)
 {
     if (oktolog())
     {
         LL_DEBUGS("LLSingleton") << verb << ' ' << demangle(name) << ';';
-        list_t& list(get_initializing());
-        for (list_t::const_reverse_iterator ri(list.rbegin()), rend(list.rend());
-             ri != rend; ++ri)
+        if (mList)
         {
-            LLSingletonBase* sb(*ri);
-            LL_CONT << ' ' << classname(sb);
+            for (list_t::const_reverse_iterator ri(mList->rbegin()), rend(mList->rend());
+                 ri != rend; ++ri)
+            {
+                LLSingletonBase* sb(*ri);
+                LL_CONT << ' ' << classname(sb);
+            }
         }
         LL_ENDL;
     }
 }
 
-void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initState)
+void LLSingletonBase::capture_dependency()
 {
+    MasterList::LockedInitializing locked_list;
+    list_t& initializing(locked_list.get());
     // Did this getInstance() call come from another LLSingleton, or from
     // vanilla application code? Note that although this is a nontrivial
     // method, the vast majority of its calls arrive here with initializing
@@ -252,21 +327,8 @@ void LLSingletonBase::capture_dependency(list_t& initializing, EInitState initSt
                 LLSingletonBase* foundp(*found);
                 out << classname(foundp) << " -> ";
             }
-            // We promise to capture dependencies from both the constructor
-            // and the initSingleton() method, so an LLSingleton's instance
-            // pointer is on the initializing list during both. Now that we've
-            // detected circularity, though, we must distinguish the two. If
-            // the recursive call is from the constructor, we CAN'T honor it:
-            // otherwise we'd be returning a pointer to a partially-
-            // constructed object! But from initSingleton() is okay: that
-            // method exists specifically to support circularity.
             // Decide which log helper to call.
-            if (initState == CONSTRUCTING)
-            {
-                logerrs("LLSingleton circularity in Constructor: ", out.str().c_str(),
-                    classname(this).c_str(), "");
-            }
-            else if (it_next == initializing.end())
+            if (it_next == initializing.end())
             {
                 // Points to self after construction, but during initialization.
                 // Singletons can initialize other classes that depend onto them,
@@ -309,12 +371,12 @@ LLSingletonBase::vec_t LLSingletonBase::dep_sort()
     // SingletonDeps through the life of the program, dynamically adding and
     // removing LLSingletons as they are created and destroyed, in practice
     // it's less messy to construct it on demand. The overhead of doing so
-    // should happen basically twice: once for cleanupAll(), once for
-    // deleteAll().
+    // should happen basically once: for deleteAll().
     typedef LLDependencies<LLSingletonBase*> SingletonDeps;
     SingletonDeps sdeps;
-    list_t& master(get_master());
-    BOOST_FOREACH(LLSingletonBase* sp, master)
+    // Lock while traversing the master list 
+    MasterList::LockedMaster master;
+    for (LLSingletonBase* sp : master.get())
     {
         // Build the SingletonDeps structure by adding, for each
         // LLSingletonBase* sp in the master list, sp itself. It has no
@@ -326,51 +388,32 @@ LLSingletonBase::vec_t LLSingletonBase::dep_sort()
                   SingletonDeps::KeyList(sp->mDepends.begin(), sp->mDepends.end()));
     }
     vec_t ret;
-    ret.reserve(master.size());
+    ret.reserve(master.get().size());
     // We should be able to effect this with a transform_iterator that
     // extracts just the first (key) element from each sorted_iterator, then
     // uses vec_t's range constructor... but frankly this is more
     // straightforward, as long as we remember the above reserve() call!
-    BOOST_FOREACH(SingletonDeps::sorted_iterator::value_type pair, sdeps.sort())
+    for (const SingletonDeps::sorted_iterator::value_type& pair : sdeps.sort())
     {
         ret.push_back(pair.first);
     }
     // The master list is not itself pushed onto the master list. Add it as
     // the very last entry -- it is the LLSingleton on which ALL others
     // depend! -- so our caller will process it.
-    ret.push_back(MasterList::getInstance());
+    ret.push_back(&master.Lock::get());
     return ret;
 }
 
-//static
-void LLSingletonBase::cleanupAll()
+void LLSingletonBase::cleanup_()
 {
-    // It's essential to traverse these in dependency order.
-    BOOST_FOREACH(LLSingletonBase* sp, dep_sort())
+    logdebugs("calling ", classname(this).c_str(), "::cleanupSingleton()");
+    try
     {
-        // Call cleanupSingleton() only if we haven't already done so for this
-        // instance.
-        if (! sp->mCleaned)
-        {
-            sp->mCleaned = true;
-
-            logdebugs("calling ",
-                      classname(sp).c_str(), "::cleanupSingleton()");
-            try
-            {
-                sp->cleanupSingleton();
-            }
-            catch (const std::exception& e)
-            {
-                logwarns("Exception in ", classname(sp).c_str(),
-                         "::cleanupSingleton(): ", e.what());
-            }
-            catch (...)
-            {
-                logwarns("Unknown exception in ", classname(sp).c_str(),
-                         "::cleanupSingleton()");
-            }
-        }
+        cleanupSingleton();
+    }
+    catch (...)
+    {
+        LOG_UNHANDLED_EXCEPTION(classname(this) + "::cleanupSingleton()");
     }
 }
 
@@ -441,10 +484,6 @@ void log(LLError::ELevel level,
     }
 }
 
-void logdebugs(const char* p1, const char* p2, const char* p3, const char* p4)
-{
-    log(LLError::LEVEL_DEBUG, p1, p2, p3, p4);
-}
 } // anonymous namespace        
 
 //static
@@ -454,6 +493,18 @@ void LLSingletonBase::logwarns(const char* p1, const char* p2, const char* p3, c
 }
 
 //static
+void LLSingletonBase::loginfos(const char* p1, const char* p2, const char* p3, const char* p4)
+{
+    log(LLError::LEVEL_INFO, p1, p2, p3, p4);
+}
+
+//static
+void LLSingletonBase::logdebugs(const char* p1, const char* p2, const char* p3, const char* p4)
+{
+    log(LLError::LEVEL_DEBUG, p1, p2, p3, p4);
+}
+
+//static
 void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, const char* p4)
 {
     log(LLError::LEVEL_ERROR, p1, p2, p3, p4);