From 2ee22ed26483381bb13bc840090bad1897b4fd27 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 18 Dec 2015 18:06:32 -0500
Subject: MAINT-5976: Fix bug in LLCoros::set_consuming() mechanism.

The original implementation of set_consuming() involved a bool* pointing to a
local bool in VoidListener::operator()()'s stack frame. postAndSuspend() would
set that bool (through the pointer) as soon as it returned from suspension.

The trouble with that is that LLEventMailDrop potentially calls its new
listener (fulfilling the future) immediately in the listen_impl() override --
in other words, way up at the top of postAndSuspend(), well before the code
that sets the relevant bool.

Instead, make the adapter formerly known as VoidListener bind the coroutine's
get_consuming() value at adapter construction time (before listening on the
LLEventPump), so that its operator()() has the coroutine's correct
get_consuming() value to return. Eliminating the bool* makes the code both
simpler AND more correct!

This change makes that adapter very specific to coroutine usage. Rename it
FutureListener and migrate it from lleventcoros.h into the .cpp file. Nobody
else was using it anyway.

Make corresponding changes to postAndSuspend2() and its WaitForEventOnHelper
class -- whose name no longer corresponds to the function as it used to.
Rename that one FutureListener2. The new FutureListener functionality, common
to both these adapters, makes it useful to derive FutureListener2 from
FutureListener.

Introduce llmake(), a generic function to deduce template type arguments from
function parameter types. This allows us to remove the voidlistener() and
wfeoh() helper functions.

Hiding VoidListener broke one of the lleventcoro_test.cpp tests. But that test
was sort of a lame recap of an earlier implementation of postAndSuspend(),
based on LLEventPump events. Recast that test to illustrate how to use a
coroutine future to suspend a coroutine for something other than an LLEventPump.

But that rubbed my nose in the fact that we MUST wrap future's context
switching with proper management of the current coroutine. Introduce
LLCoros::Future<T>, which wraps boost::dcoroutines::future<T>.

Use LLCoros::Future<T> in postAndSuspend() and postAndSuspend2().
---
 indra/llcommon/llcoros.h                  |  48 +++++++++++
 indra/llcommon/lleventcoro.cpp            | 135 ++++++++++++++++--------------
 indra/llcommon/lleventcoro.h              |  30 -------
 indra/llcommon/llmake.h                   |  63 ++++++++++++++
 indra/llcommon/tests/lleventcoro_test.cpp |  62 +++++++-------
 5 files changed, 219 insertions(+), 119 deletions(-)
 create mode 100644 indra/llcommon/llmake.h

(limited to 'indra/llcommon')

diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h
index 0b1f58f48e..f2fb7f1e70 100755
--- a/indra/llcommon/llcoros.h
+++ b/indra/llcommon/llcoros.h
@@ -30,6 +30,7 @@
 #define LL_LLCOROS_H
 
 #include <boost/dcoroutine/coroutine.hpp>
+#include <boost/dcoroutine/future.hpp>
 #include "llsingleton.h"
 #include <boost/ptr_container/ptr_map.hpp>
 #include <boost/function.hpp>
@@ -162,6 +163,15 @@ public:
     static void set_consuming(bool consuming);
     static bool get_consuming();
 
+    /**
+     * Please do NOT directly use boost::dcoroutines::future! It is essential
+     * to maintain the "current" coroutine at every context switch. This
+     * Future wraps the essential boost::dcoroutines::future functionality
+     * with that maintenance.
+     */
+    template <typename T>
+    class Future;
+
 private:
     LLCoros();
     friend class LLSingleton<LLCoros>;
@@ -233,4 +243,42 @@ private:
 
 } // namespace llcoro
 
+template <typename T>
+class LLCoros::Future
+{
+    typedef boost::dcoroutines::future<T> dfuture;
+
+public:
+    Future():
+        mFuture(get_self())
+    {}
+
+    typedef typename boost::dcoroutines::make_callback_result<dfuture>::type callback_t;
+
+    callback_t make_callback()
+    {
+        return boost::dcoroutines::make_callback(mFuture);
+    }
+
+    explicit operator bool() const
+    {
+        return bool(mFuture);
+    }
+
+    bool operator!() const
+    {
+        return ! mFuture;
+    }
+
+    T get()
+    {
+        // instantiate Suspending to manage the "current" coroutine
+        llcoro::Suspending suspended;
+        return *mFuture;
+    }
+
+private:
+    dfuture mFuture;
+};
+
 #endif /* ! defined(LL_LLCOROS_H) */
diff --git a/indra/llcommon/lleventcoro.cpp b/indra/llcommon/lleventcoro.cpp
index 9b7e8fb65f..578a2b62c8 100755
--- a/indra/llcommon/lleventcoro.cpp
+++ b/indra/llcommon/lleventcoro.cpp
@@ -34,12 +34,11 @@
 #include <map>
 // std headers
 // external library headers
-#include <boost/dcoroutine/coroutine.hpp>
-#include <boost/dcoroutine/future.hpp>
 // other Linden headers
 #include "llsdserialize.h"
 #include "llerror.h"
 #include "llcoros.h"
+#include "llmake.h"
 
 #include "lleventfilter.h"
 
@@ -145,6 +144,36 @@ void storeToLLSDPath(LLSD& dest, const LLSD& rawPath, const LLSD& value)
     *pdest = value;
 }
 
+/// For LLCoros::Future<LLSD>::make_callback(), the callback has a signature
+/// like void callback(LLSD), which isn't a valid LLEventPump listener: such
+/// listeners must return bool.
+template <typename LISTENER>
+class FutureListener
+{
+public:
+    // FutureListener is instantiated on the coroutine stack: the stack, in
+    // other words, that wants to suspend.
+    FutureListener(const LISTENER& listener):
+        mListener(listener),
+        // Capture the suspending coroutine's flag as a consuming or
+        // non-consuming listener.
+        mConsume(LLCoros::get_consuming())
+    {}
+
+    // operator()() is called on the main stack: the stack on which the
+    // expected event is fired.
+    bool operator()(const LLSD& event)
+    {
+        mListener(event);
+        // tell upstream LLEventPump whether listener consumed
+        return mConsume;
+    }
+
+protected:
+    LISTENER mListener;
+    bool mConsume;
+};
+
 } // anonymous
 
 void llcoro::suspend()
@@ -167,13 +196,13 @@ LLSD llcoro::postAndSuspend(const LLSD& event, const LLEventPumpOrPumpName& requ
                  const LLEventPumpOrPumpName& replyPump, const LLSD& replyPumpNamePath)
 {
     // declare the future
-    boost::dcoroutines::future<LLSD_consumed> future(LLCoros::get_self());
+    LLCoros::Future<LLSD> future;
     // make a callback that will assign a value to the future, and listen on
     // the specified LLEventPump with that callback
     std::string listenerName(listenerNameForCoro());
     LLTempBoundListener connection(
         replyPump.getPump().listen(listenerName,
-                                   voidlistener(boost::dcoroutines::make_callback(future))));
+                                   llmake<FutureListener>(future.make_callback())));
     // skip the "post" part if requestPump is default-constructed
     if (requestPump)
     {
@@ -192,67 +221,56 @@ LLSD llcoro::postAndSuspend(const LLSD& event, const LLEventPumpOrPumpName& requ
     LL_DEBUGS("lleventcoro") << "postAndSuspend(): coroutine " << listenerName
                              << " about to wait on LLEventPump " << replyPump.getPump().getName()
                              << LL_ENDL;
-    // trying to dereference ("resolve") the future makes us wait for it
-    LLSD_consumed value;
-    {
-        // instantiate Suspending to manage the "current" coroutine
-        llcoro::Suspending suspended;
-        value = *future;
-    } // destroy Suspending as soon as we're back
+    // calling get() on the future makes us wait for it
+    LLSD value(future.get());
     LL_DEBUGS("lleventcoro") << "postAndSuspend(): coroutine " << listenerName
-                             << " resuming with " << value.first << LL_ENDL;
-    // immediately set consumed according to consuming
-    *value.second = LLCoros::get_consuming();
+                             << " resuming with " << value << LL_ENDL;
     // returning should disconnect the connection
-    return value.first;
+    return value;
 }
 
 namespace
 {
 
-typedef std::pair<LLEventWithID, bool*> LLEventWithID_consumed;
-
 /**
- * This helper is specifically for the two-pump version of suspendUntilEventOn().
- * We use a single future object, but we want to listen on two pumps with it.
- * Since we must still adapt from (the callable constructed by)
- * boost::dcoroutines::make_callback() (void return) to provide an event
- * listener (bool return), we've adapted VoidListener for the purpose. The
- * basic idea is that we construct a distinct instance of WaitForEventOnHelper
- * -- binding different instance data -- for each of the pumps. Then, when a
- * pump delivers an LLSD value to either WaitForEventOnHelper, it can combine
- * that LLSD with its discriminator to feed the future object.
+ * This helper is specifically for postAndSuspend2(). We use a single future
+ * object, but we want to listen on two pumps with it. Since we must still
+ * adapt from the callable constructed by boost::dcoroutines::make_callback()
+ * (void return) to provide an event listener (bool return), we've adapted
+ * FutureListener for the purpose. The basic idea is that we construct a
+ * distinct instance of FutureListener2 -- binding different instance data --
+ * for each of the pumps. Then, when a pump delivers an LLSD value to either
+ * FutureListener2, it can combine that LLSD with its discriminator to feed
+ * the future object.
+ *
+ * DISCRIM is a template argument so we can use llmake() rather than
+ * having to write our own argument-deducing helper function.
  */
-template <typename LISTENER>
-class WaitForEventOnHelper
+template <typename LISTENER, typename DISCRIM>
+class FutureListener2: public FutureListener<LISTENER>
 {
+    typedef FutureListener<LISTENER> super;
+
 public:
-    WaitForEventOnHelper(const LISTENER& listener, int discriminator):
-        mListener(listener),
+    // instantiated on coroutine stack: the stack about to suspend
+    FutureListener2(const LISTENER& listener, DISCRIM discriminator):
+        super(listener),
         mDiscrim(discriminator)
     {}
 
-    // this signature is required for an LLEventPump listener
+    // called on main stack: the stack on which event is fired
     bool operator()(const LLSD& event)
     {
-        bool consumed = false;
-        // our future object is defined to accept LLEventWithID_consumed
-        mListener(LLEventWithID_consumed(LLEventWithID(event, mDiscrim), &consumed));
+        // our future object is defined to accept LLEventWithID
+        super::mListener(LLEventWithID(event, mDiscrim));
         // tell LLEventPump whether or not event was consumed
-        return consumed;
+        return super::mConsume;
     }
+
 private:
-    LISTENER mListener;
-    const int mDiscrim;
+    const DISCRIM mDiscrim;
 };
 
-/// WaitForEventOnHelper type-inference helper
-template <typename LISTENER>
-WaitForEventOnHelper<LISTENER> wfeoh(const LISTENER& listener, int discriminator)
-{
-    return WaitForEventOnHelper<LISTENER>(listener, discriminator);
-}
-
 } // anonymous
 
 namespace llcoro
@@ -266,16 +284,18 @@ LLEventWithID postAndSuspend2(const LLSD& event,
                            const LLSD& replyPump1NamePath)
 {
     // declare the future
-    boost::dcoroutines::future<LLEventWithID_consumed> future(LLCoros::get_self());
+    LLCoros::Future<LLEventWithID> future;
     // either callback will assign a value to this future; listen on
     // each specified LLEventPump with a callback
     std::string name(listenerNameForCoro());
     LLTempBoundListener connection0(
-        replyPump0.getPump().listen(name + "a",
-                                    wfeoh(boost::dcoroutines::make_callback(future), 0)));
+        replyPump0.getPump().listen(
+            name + "a",
+            llmake<FutureListener2>(future.make_callback(), 0)));
     LLTempBoundListener connection1(
-        replyPump1.getPump().listen(name + "b",
-                                    wfeoh(boost::dcoroutines::make_callback(future), 1)));
+        replyPump1.getPump().listen(
+            name + "b",
+            llmake<FutureListener2>(future.make_callback(), 1)));
     // skip the "post" part if requestPump is default-constructed
     if (requestPump)
     {
@@ -294,20 +314,13 @@ LLEventWithID postAndSuspend2(const LLSD& event,
     LL_DEBUGS("lleventcoro") << "postAndSuspend2(): coroutine " << name
                              << " about to wait on LLEventPumps " << replyPump0.getPump().getName()
                              << ", " << replyPump1.getPump().getName() << LL_ENDL;
-    // trying to dereference ("resolve") the future makes us wait for it
-    LLEventWithID_consumed value;
-    {
-        // instantiate Suspending to manage "current" coroutine
-        llcoro::Suspending suspended;
-        value = *future;
-    } // destroy Suspending as soon as we're back
+    // calling get() on the future makes us wait for it
+    LLEventWithID value(future.get());
     LL_DEBUGS("lleventcoro") << "postAndSuspend(): coroutine " << name
-                             << " resuming with (" << value.first.first
-                             << ", " << value.first.second << ")" << LL_ENDL;
-    // tell LLEventPump whether we're consuming
-    *value.second = LLCoros::get_consuming();
+                             << " resuming with (" << value.first << ", " << value.second << ")"
+                             << LL_ENDL;
     // returning should disconnect both connections
-    return value.first;
+    return value;
 }
 
 LLSD errorException(const LLEventWithID& result, const std::string& desc)
diff --git a/indra/llcommon/lleventcoro.h b/indra/llcommon/lleventcoro.h
index acf2ad24a4..2105faf861 100755
--- a/indra/llcommon/lleventcoro.h
+++ b/indra/llcommon/lleventcoro.h
@@ -76,36 +76,6 @@ private:
 namespace llcoro
 {
 
-typedef std::pair<LLSD, bool*> LLSD_consumed;
-
-/// This is an adapter for a signature like void LISTENER(const LLSD&), which
-/// isn't a valid LLEventPump listener: such listeners should return bool.
-template <typename LISTENER>
-class VoidListener
-{
-public:
-    VoidListener(const LISTENER& listener):
-        mListener(listener)
-    {}
-
-    bool operator()(const LLSD& event)
-    {
-        bool consumed = false;
-        mListener(LLSD_consumed(event, &consumed));
-        // tell upstream LLEventPump whether listener consumed
-        return consumed;
-    }
-private:
-    LISTENER mListener;
-};
-
-/// VoidListener helper function to infer the type of the LISTENER
-template <typename LISTENER>
-VoidListener<LISTENER> voidlistener(const LISTENER& listener)
-{
-    return VoidListener<LISTENER>(listener);
-}
-
 /**
  * Yield control from a coroutine for one "mainloop" tick. If your coroutine
  * runs without suspending for nontrivial time, sprinkle in calls to this
diff --git a/indra/llcommon/llmake.h b/indra/llcommon/llmake.h
new file mode 100644
index 0000000000..9a662a0640
--- /dev/null
+++ b/indra/llcommon/llmake.h
@@ -0,0 +1,63 @@
+/**
+ * @file   llmake.h
+ * @author Nat Goodspeed
+ * @date   2015-12-18
+ * @brief  Generic llmake<Template>(arg) function to instantiate
+ *         Template<decltype(arg)>(arg).
+ *
+ *         Many of our class templates have an accompanying helper function to
+ *         make an instance with arguments of arbitrary type. llmake()
+ *         eliminates the need to declare a new helper function for every such
+ *         class template.
+ * 
+ *         also relevant:
+ *
+ *         Template parameter deduction for constructors
+ *         http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0091r0.html
+ *
+ *         https://github.com/viboes/std-make
+ *
+ *         but obviously we're not there yet.
+ *
+ * $LicenseInfo:firstyear=2015&license=viewerlgpl$
+ * Copyright (c) 2015, Linden Research, Inc.
+ * $/LicenseInfo$
+ */
+
+#if ! defined(LL_LLMAKE_H)
+#define LL_LLMAKE_H
+
+/*==========================================================================*|
+// When we allow ourselves to compile with C++11 features enabled, this form
+// should generically handle an arbitrary number of arguments.
+
+template <template<typename...> class CLASS_TEMPLATE, typename... ARGS>
+CLASS_TEMPLATE<ARGS...> llmake(ARGS && ... args)
+{
+    return CLASS_TEMPLATE<ARGS...>(std::forward<ARGS>(args)...);
+}
+|*==========================================================================*/
+
+// As of 2015-12-18, this is what we'll use instead. Add explicit overloads
+// for different numbers of template parameters as use cases arise.
+
+/**
+ * Usage: llmake<SomeTemplate>(arg)
+ *
+ * Deduces the type T of 'arg' and returns an instance of SomeTemplate<T>
+ * initialized with 'arg'. Assumes a constructor accepting T (by value,
+ * reference or whatever).
+ */
+template <template<typename> class CLASS_TEMPLATE, typename ARG1>
+CLASS_TEMPLATE<ARG1> llmake(const ARG1& arg1)
+{
+    return CLASS_TEMPLATE<ARG1>(arg1);
+}
+
+template <template<typename, typename> class CLASS_TEMPLATE, typename ARG1, typename ARG2>
+CLASS_TEMPLATE<ARG1, ARG2> llmake(const ARG1& arg1, const ARG2& arg2)
+{
+    return CLASS_TEMPLATE<ARG1, ARG2>(arg1, arg2);
+}
+
+#endif /* ! defined(LL_LLMAKE_H) */
diff --git a/indra/llcommon/tests/lleventcoro_test.cpp b/indra/llcommon/tests/lleventcoro_test.cpp
index da1439418f..a459d17fb8 100755
--- a/indra/llcommon/tests/lleventcoro_test.cpp
+++ b/indra/llcommon/tests/lleventcoro_test.cpp
@@ -65,14 +65,11 @@
 // Boost.Coroutine #include is the *first* #include of the platform header.
 // That means that client code must generally #include Boost.Coroutine headers
 // before anything else.
-#define BOOST_RESULT_OF_USE_TR1 1
 #include <boost/dcoroutine/coroutine.hpp>
-// Normally, lleventcoro.h obviates future.hpp. We only include this because
-// we implement a "by hand" test of future functionality.
-#include <boost/dcoroutine/future.hpp>
 #include <boost/bind.hpp>
 #include <boost/range.hpp>
 #include <boost/utility.hpp>
+#include <boost/shared_ptr.hpp>
 
 #include "linden_common.h"
 
@@ -218,7 +215,7 @@ namespace tut
     // use static data so we can intersperse coroutine functions with the
     // tests that engage them
     ImmediateAPI immediateAPI;
-    std::string replyName, errorName, threw;
+    std::string replyName, errorName, threw, stringdata;
     LLSD result, errordata;
     int which;
 
@@ -228,28 +225,40 @@ namespace tut
         replyName.clear();
         errorName.clear();
         threw.clear();
+        stringdata.clear();
         result = LLSD();
         errordata = LLSD();
         which = 0;
     }
 
-    void explicit_wait(boost::dcoroutines::coroutine<void()>::self& self)
+    void explicit_wait(boost::shared_ptr<LLCoros::Future<std::string>::callback_t>& cbp)
     {
         BEGIN
         {
-            // ... do whatever preliminary stuff must happen ...
+            // The point of this test is to verify / illustrate suspending a
+            // coroutine for something other than an LLEventPump. In other
+            // words, this shows how to adapt to any async operation that
+            // provides a callback-style notification (and prove that it
+            // works).
+
+            LLCoros::Future<std::string> future;
+            // get the callback from that future
+            LLCoros::Future<std::string>::callback_t callback(future.make_callback());
+
+            // Perhaps we would send a request to a remote server and arrange
+            // for 'callback' to be called on response. Of course that might
+            // involve an adapter object from the actual callback signature to
+            // the signature of 'callback' -- in this case, void(std::string).
+            // For test purposes, instead of handing 'callback' (or the
+            // adapter) off to some I/O subsystem, we'll just pass it back to
+            // our caller.
+            cbp.reset(new LLCoros::Future<std::string>::callback_t(callback));
 
-            // declare the future
-            boost::dcoroutines::future<llcoro::LLSD_consumed> future(self);
-            // tell the future what to suspend for
-            LLTempBoundListener connection(
-                LLEventPumps::instance().obtain("source").listen("coro", voidlistener(boost::dcoroutines::make_callback(future))));
             ensure("Not yet", ! future);
-            // attempting to dereference ("resolve") the future causes the calling
-            // coroutine to suspend for it
+            // calling get() on the future causes us to suspend
             debug("about to suspend");
-            result = (*future).first;
-            ensure("Got it", future);
+            stringdata = future.get();
+            ensure("Got it", bool(future));
         }
         END
     }
@@ -262,19 +271,16 @@ namespace tut
         DEBUG;
 
         // Construct the coroutine instance that will run explicit_wait.
-        // Pass the ctor a callable that accepts the coroutine_type::self
-        // param passed by the library.
-        boost::dcoroutines::coroutine<void()> coro(explicit_wait);
-        // Start the coroutine
-        coro(std::nothrow);
-        // When the coroutine waits for the event pump, it returns here.
-        debug("about to send");
-        // Satisfy the suspend.
-        LLEventPumps::instance().obtain("source").post("received");
-        // Now suspend for the coroutine to complete.
-        ensure("coroutine complete", ! coro);
+        boost::shared_ptr<LLCoros::Future<std::string>::callback_t> respond;
+        LLCoros::instance().launch("test<2>",
+                                   boost::bind(explicit_wait, boost::ref(respond)));
+        // When the coroutine waits for the future, it returns here.
+        debug("about to respond");
+        // Now we're the I/O subsystem delivering a result. This immediately
+        // transfers control back to the coroutine.
+        (*respond)("received");
         // ensure the coroutine ran and woke up again with the intended result
-        ensure_equals(result.asString(), "received");
+        ensure_equals(stringdata, "received");
     }
 
     void waitForEventOn1()
-- 
cgit v1.2.3