MAINT-5351: Improve management of "current" coroutine information.

Our first cut at tracking the "current" coroutine simply reset the pointer to
NULL every time we context-switched away. But that strategy doesn't handle the
case of coroutine A launching coroutine B.
Introduce LLCoros::CoroData to track, among other things, the previous value
of the current-coroutine pointer each time we switch into a coroutine. Restore
THAT value when we switch back out.

3 files changed, 106 insertions, 66 deletions

diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp
index 957fe034e1..d76401d01b 100755
--- a/indra/llcommon/llcoros.cpp
+++ b/indra/llcommon/llcoros.cpp
@@ -34,63 +34,47 @@
 // std headers
 // external library headers
 #include <boost/bind.hpp>
-#include <boost/thread/tss.hpp>
 // other Linden headers
 #include "llevents.h"
 #include "llerror.h"
 #include "stringize.h"
 
-namespace {
-
 // do nothing, when we need nothing done
-void no_cleanup(LLCoros::coro::self*) {}
-
-// When the dcoroutine library calls a top-level callable, it implicitly
-// passes coro::self& as the first parameter. All our consumer code used to
-// explicitly pass coro::self& down through all levels of call stack, because
-// at the leaf level we need it for context-switching. But since coroutines
-// are based on cooperative switching, we can cause the top-level entry point
-// to stash a static pointer to the currently-running coroutine, and manage it
-// appropriately as we switch out and back in. That eliminates the need to
-// pass it as an explicit parameter down through every level, which is
-// unfortunately viral in nature. Finding it implicitly rather than explicitly
-// allows minor maintenance in which a leaf-level function adds a new async
-// I/O call that suspends the calling coroutine, WITHOUT having to propagate
-// coro::self& through every function signature down to that point -- and of
-// course through every other caller of every such function.
-// We use a boost::thread_specific_ptr because each thread potentially has its
-// own distinct pool of coroutines.
-// This thread_specific_ptr does NOT own the 'self' object! It merely
-// identifies it. For this reason we instantiate it with a no-op cleanup
-// function.
-static boost::thread_specific_ptr<LLCoros::coro::self>
-sCurrentSelf(no_cleanup);
-
-} // anonymous
+void LLCoros::no_cleanup(CoroData*) {}
+
+// CoroData for the currently-running coroutine. Use a thread_specific_ptr
+// because each thread potentially has its own distinct pool of coroutines.
+// This thread_specific_ptr does NOT own the CoroData object! That's owned by
+// LLCoros::mCoros. It merely identifies it. For this reason we instantiate
+// it with a no-op cleanup function.
+boost::thread_specific_ptr<LLCoros::CoroData>
+LLCoros::sCurrentCoro(LLCoros::no_cleanup);
 
 //static
-LLCoros::coro::self& llcoro::get_self()
+LLCoros::coro::self& LLCoros::get_self()
 {
-    LLCoros::coro::self* current_self = sCurrentSelf.get();
-    if (! current_self)
+    CoroData* current = sCurrentCoro.get();
+    if (! current)
     {
         LL_ERRS("LLCoros") << "Calling get_self() from non-coroutine context!" << LL_ENDL;
     }
-    return *current_self;
+    return *current->mSelf;
 }
 
 llcoro::Suspending::Suspending():
-    mSuspended(sCurrentSelf.get())
+    mSuspended(LLCoros::sCurrentCoro.get())
 {
-    // For the duration of our time away from this coroutine, sCurrentSelf
-    // must NOT refer to this coroutine.
-    sCurrentSelf.reset();
+    // Revert mCurrentCoro to the value it had at the moment we last switched
+    // into this coroutine.
+    LLCoros::sCurrentCoro.reset(mSuspended->mPrev);
 }
 
 llcoro::Suspending::~Suspending()
 {
-    // Okay, we're back, reinstate previous value of sCurrentSelf.
-    sCurrentSelf.reset(mSuspended);
+    // Okay, we're back, update our mPrev
+    mSuspended->mPrev = LLCoros::sCurrentCoro.get();
+    // and reinstate our sCurrentCoro.
+    LLCoros::sCurrentCoro.reset(mSuspended);
 }
 
 LLCoros::LLCoros():
@@ -112,7 +96,7 @@ bool LLCoros::cleanup(const LLSD&)
     {
         // Has this coroutine exited (normal return, exception, exit() call)
         // since last tick?
-        if (mi->second->exited())
+        if (mi->second->mCoro.exited())
         {
             LL_INFOS("LLCoros") << "LLCoros: cleaning up coroutine " << mi->first << LL_ENDL;
             // The erase() call will invalidate its passed iterator value --
@@ -170,18 +154,13 @@ bool LLCoros::kill(const std::string& name)
 
 std::string LLCoros::getName() const
 {
-    // Walk the existing coroutines, looking for the current one.
-    void* self_id = llcoro::get_self().get_id();
-    for (CoroMap::const_iterator mi(mCoros.begin()), mend(mCoros.end()); mi != mend; ++mi)
+    CoroData* current = sCurrentCoro.get();
+    if (! current)
     {
-        namespace coro_private = boost::dcoroutines::detail;
-        if (static_cast<void*>(coro_private::coroutine_accessor::get_impl(const_cast<coro&>(*mi->second)).get())
-            == self_id)
-        {
-            return mi->first;
-        }
+        // not in a coroutine
+        return "";
     }
-    return "";
+    return current->mName;
 }
 
 void LLCoros::setStackSize(S32 stacksize)
@@ -190,20 +169,20 @@ void LLCoros::setStackSize(S32 stacksize)
     mStackSize = stacksize;
 }
 
-namespace {
-
 // Top-level wrapper around caller's coroutine callable. This function accepts
 // the coroutine library's implicit coro::self& parameter and sets sCurrentSelf
 // but does not pass it down to the caller's callable.
-void toplevel(LLCoros::coro::self& self, const LLCoros::callable_t& callable)
+void LLCoros::toplevel(coro::self& self, CoroData* data, const callable_t& callable)
 {
-    sCurrentSelf.reset(&self);
+    // capture the 'self' param in CoroData
+    data->mSelf = &self;
+    // run the code the caller actually wants in the coroutine
     callable();
-    sCurrentSelf.reset();
+    // This cleanup isn't perfectly symmetrical with the way we initially set
+    // data->mPrev, but this is our last chance to reset mCurrentCoro.
+    sCurrentCoro.reset(data->mPrev);
 }
 
-} // anonymous
-
 /*****************************************************************************
 *   MUST BE LAST
 *****************************************************************************/
@@ -215,19 +194,33 @@ void toplevel(LLCoros::coro::self& self, const LLCoros::callable_t& callable)
 #if LL_MSVC
 // work around broken optimizations
 #pragma warning(disable: 4748)
+#pragma warning(disable: 4355) // 'this' used in initializer list: yes, intentionally
 #pragma optimize("", off)
 #endif // LL_MSVC
 
+LLCoros::CoroData::CoroData(CoroData* prev, const std::string& name,
+                            const callable_t& callable, S32 stacksize):
+    mPrev(prev),
+    mName(name),
+    // Wrap the caller's callable in our toplevel() function so we can manage
+    // sCurrentCoro appropriately at startup and shutdown of each coroutine.
+    mCoro(boost::bind(toplevel, _1, this, callable), stacksize),
+    mSelf(0)
+{
+}
+
 std::string LLCoros::launch(const std::string& prefix, const callable_t& callable)
 {
     std::string name(generateDistinctName(prefix));
-    // Wrap the caller's callable in our toplevel() function so we can manage
-    // sCurrentSelf appropriately at startup and shutdown of each coroutine.
-    coro* newCoro = new coro(boost::bind(toplevel, _1, callable), mStackSize);
+    // pass the current value of sCurrentCoro as previous context
+    CoroData* newCoro = new CoroData(sCurrentCoro.get(), name,
+                                     callable, mStackSize);
     // Store it in our pointer map
     mCoros.insert(name, newCoro);
+    // also set it as current
+    sCurrentCoro.reset(newCoro);
     /* Run the coroutine until its first wait, then return here */
-    (*newCoro)(std::nothrow);
+    (newCoro->mCoro)(std::nothrow);
     return name;
 }
 
diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h
index e478600f00..56eed8cafe 100755
--- a/indra/llcommon/llcoros.h
+++ b/indra/llcommon/llcoros.h
@@ -33,9 +33,16 @@
 #include "llsingleton.h"
 #include <boost/ptr_container/ptr_map.hpp>
 #include <boost/function.hpp>
+#include <boost/thread/tss.hpp>
 #include <string>
 #include <stdexcept>
 
+// forward-declare helper class
+namespace llcoro
+{
+class Suspending;
+}
+
 /**
  * Registry of named Boost.Coroutine instances
  *
@@ -140,23 +147,63 @@ public:
     /// for delayed initialization
     void setStackSize(S32 stacksize);
 
+    /// get the current coro::self& for those who really really care
+    static coro::self& get_self();
+
 private:
     LLCoros();
     friend class LLSingleton<LLCoros>;
+    friend class llcoro::Suspending;
     std::string generateDistinctName(const std::string& prefix) const;
     bool cleanup(const LLSD&);
+    struct CoroData;
+    static void no_cleanup(CoroData*);
+    static void toplevel(coro::self& self, CoroData* data, const callable_t& callable);
 
     S32 mStackSize;
-    typedef boost::ptr_map<std::string, coro> CoroMap;
+
+    // coroutine-local storage, as it were: one per coro we track
+    struct CoroData
+    {
+        CoroData(CoroData* prev, const std::string& name,
+                 const callable_t& callable, S32 stacksize);
+
+        // The boost::dcoroutines library supports asymmetric coroutines. Every
+        // time we context switch out of a coroutine, we pass control to the
+        // previously-active one (or to the non-coroutine stack owned by the
+        // thread). So our management of the "current" coroutine must be able to
+        // restore the previous value when we're about to switch away.
+        CoroData* mPrev;
+        // tweaked name of the current coroutine
+        const std::string mName;
+        // the actual coroutine instance
+        LLCoros::coro mCoro;
+        // When the dcoroutine library calls a top-level callable, it implicitly
+        // passes coro::self& as the first parameter. All our consumer code used
+        // to explicitly pass coro::self& down through all levels of call stack,
+        // because at the leaf level we need it for context-switching. But since
+        // coroutines are based on cooperative switching, we can cause the
+        // top-level entry point to stash a pointer to the currently-running
+        // coroutine, and manage it appropriately as we switch out and back in.
+        // That eliminates the need to pass it as an explicit parameter down
+        // through every level, which is unfortunately viral in nature. Finding it
+        // implicitly rather than explicitly allows minor maintenance in which a
+        // leaf-level function adds a new async I/O call that suspends the calling
+        // coroutine, WITHOUT having to propagate coro::self& through every
+        // function signature down to that point -- and of course through every
+        // other caller of every such function.
+        LLCoros::coro::self* mSelf;
+    };
+    typedef boost::ptr_map<std::string, CoroData> CoroMap;
     CoroMap mCoros;
+
+    // identify the current coroutine's CoroData
+    static boost::thread_specific_ptr<LLCoros::CoroData> sCurrentCoro;
 };
 
 namespace llcoro
 {
 
-/// get the current coro::self& for those who really really care
-LLCoros::coro::self& get_self();
-
 /// Instantiate one of these in a block surrounding any leaf point when
 /// control literally switches away from this coroutine.
 class Suspending
@@ -166,7 +213,7 @@ public:
     ~Suspending();
 
 private:
-    LLCoros::coro::self* mSuspended;
+    LLCoros::CoroData* mSuspended;
 };
 
 } // namespace llcoro
diff --git a/indra/llcommon/lleventcoro.cpp b/indra/llcommon/lleventcoro.cpp
index ad02e139b8..85af0d9b9f 100755
--- a/indra/llcommon/lleventcoro.cpp
+++ b/indra/llcommon/lleventcoro.cpp
@@ -157,7 +157,7 @@ LLSD llcoro::postAndWait(const LLSD& event, const LLEventPumpOrPumpName& request
                          const LLEventPumpOrPumpName& replyPump, const LLSD& replyPumpNamePath)
 {
     // declare the future
-    boost::dcoroutines::future<LLSD> future(llcoro::get_self());
+    boost::dcoroutines::future<LLSD> future(LLCoros::get_self());
     // make a callback that will assign a value to the future, and listen on
     // the specified LLEventPump with that callback
     std::string listenerName(listenerNameForCoro());
@@ -250,7 +250,7 @@ LLEventWithID postAndWait2(const LLSD& event,
                            const LLSD& replyPump1NamePath)
 {
     // declare the future
-    boost::dcoroutines::future<LLEventWithID> future(llcoro::get_self());
+    boost::dcoroutines::future<LLEventWithID> future(LLCoros::get_self());
     // either callback will assign a value to this future; listen on
     // each specified LLEventPump with a callback
     std::string name(listenerNameForCoro());