Merge pull request #2068 from secondlife/lua-pump-notrack

Allow `LLEventPump` listener access to its own `LLBoundListener`. 

10 files changed, 101 insertions, 152 deletions

diff --git a/indra/llcommon/lleventfilter.cpp b/indra/llcommon/lleventfilter.cpp
index da19946e3b..2b5401e9f7 100644
--- a/indra/llcommon/lleventfilter.cpp
+++ b/indra/llcommon/lleventfilter.cpp
@@ -365,21 +365,22 @@ bool LLEventLogProxy::post(const LLSD& event) /* override */
 }
 
 LLBoundListener LLEventLogProxy::listen_impl(const std::string& name,
-                                             const LLEventListener& target,
+                                             const LLAwareListener& target,
                                              const NameList& after,
                                              const NameList& before)
 {
     LL_DEBUGS("LogProxy") << "LLEventLogProxy('" << getName() << "').listen('"
                           << name << "')" << LL_ENDL;
     return mPump.listen(name,
-                        [this, name, target](const LLSD& event)->bool
-                        { return listener(name, target, event); },
+                        [this, name, target](const LLBoundListener& conn, const LLSD& event)
+                        { return listener(conn, name, target, event); },
                         after,
                         before);
 }
 
-bool LLEventLogProxy::listener(const std::string& name,
-                               const LLEventListener& target,
+bool LLEventLogProxy::listener(const LLBoundListener& conn,
+                               const std::string& name,
+                               const LLAwareListener& target,
                                const LLSD& event) const
 {
     auto eventminus = event;
@@ -391,7 +392,7 @@ bool LLEventLogProxy::listener(const std::string& name,
     }
     std::string hdr{STRINGIZE(getName() << " to " << name << " " << counter)};
     LL_INFOS("LogProxy") << hdr << ": " << eventminus << LL_ENDL;
-    bool result = target(eventminus);
+    bool result = target(conn, eventminus);
     LL_INFOS("LogProxy") << hdr << " => " << result << LL_ENDL;
     return result;
 }
diff --git a/indra/llcommon/lleventfilter.h b/indra/llcommon/lleventfilter.h
index 9988459aae..de706d72eb 100644
--- a/indra/llcommon/lleventfilter.h
+++ b/indra/llcommon/lleventfilter.h
@@ -459,7 +459,7 @@ public:
     LLEventLogProxy(LLEventPump& source, const std::string& name, bool tweak=false);
 
     /// register a new listener
-    LLBoundListener listen_impl(const std::string& name, const LLEventListener& target,
+    LLBoundListener listen_impl(const std::string& name, const LLAwareListener& target,
                                 const NameList& after, const NameList& before);
 
     /// Post an event to all listeners
@@ -469,8 +469,9 @@ private:
     /// This method intercepts each call to any target listener. We pass it
     /// the listener name and the caller's intended target listener plus the
     /// posted LLSD event.
-    bool listener(const std::string& name,
-                  const LLEventListener& target,
+    bool listener(const LLBoundListener& conn,
+                  const std::string& name,
+                  const LLAwareListener& target,
                   const LLSD& event) const;
 
     LLEventPump& mPump;
diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp
index 5a6e13cb7d..98bd990f31 100644
--- a/indra/llcommon/llevents.cpp
+++ b/indra/llcommon/llevents.cpp
@@ -415,7 +415,7 @@ void LLEventPump::reset()
     //mDeps.clear();
 }
 
-LLBoundListener LLEventPump::listen_impl(const std::string& name, const LLEventListener& listener,
+LLBoundListener LLEventPump::listen_impl(const std::string& name, const LLAwareListener& listener,
                                          const NameList& after,
                                          const NameList& before)
 {
@@ -575,7 +575,7 @@ LLBoundListener LLEventPump::listen_impl(const std::string& name, const LLEventL
     }
     // Now that newNode has a value that places it appropriately in mSignal,
     // connect it.
-    LLBoundListener bound = mSignal->connect(nodePosition, listener);
+    LLBoundListener bound = mSignal->connect_extended(nodePosition, listener);
     
     if (!name.empty())
     {   // note that we are not tracking anonymous listeners here either.
@@ -659,7 +659,7 @@ bool LLEventMailDrop::post(const LLSD& event)
 }
 
 LLBoundListener LLEventMailDrop::listen_impl(const std::string& name,
-                                    const LLEventListener& listener,
+                                    const LLAwareListener& listener,
                                     const NameList& after,
                                     const NameList& before)
 {
@@ -668,7 +668,10 @@ LLBoundListener LLEventMailDrop::listen_impl(const std::string& name,
     // Remove any that this listener consumes -- Effective STL, Item 9.
     for (auto hi(mEventHistory.begin()), hend(mEventHistory.end()); hi != hend; )
     {
-        if (listener(*hi))
+        // We don't actually have an LLBoundListener in hand, and we won't
+        // until the base-class listen_impl() call below. Pass an empty
+        // instance.
+        if (listener({}, *hi))
         {
             // new listener consumed this event, erase it
             hi = mEventHistory.erase(hi);
diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h
index d0686bd8b5..abc25ba400 100644
--- a/indra/llcommon/llevents.h
+++ b/indra/llcommon/llevents.h
@@ -32,12 +32,13 @@
 #if ! defined(LL_LLEVENTS_H)
 #define LL_LLEVENTS_H
 
-#include <string>
+#include <deque>
+#include <functional>
 #include <map>
 #include <set>
+#include <string>
+#include <type_traits>
 #include <vector>
-#include <deque>
-#include <functional>
 #if LL_WINDOWS
     #pragma warning (push)
     #pragma warning (disable : 4263) // boost::signals2::expired_slot::what() has const mismatch
@@ -137,6 +138,10 @@ typedef boost::signals2::signal<bool(const LLSD&), LLStopWhenHandled, float>  LL
 /// Methods that forward listeners (e.g. constructed with
 /// <tt>boost::bind()</tt>) should accept (const LLEventListener&)
 typedef LLStandardSignal::slot_type LLEventListener;
+/// Support a listener accepting (const LLBoundListener&, const LLSD&).
+/// Note that LLBoundListener::disconnect() is a const method: this feature is
+/// specifically intended to allow a listener to disconnect itself when done.
+typedef LLStandardSignal::extended_slot_type LLAwareListener;
 /// Accept a void listener too
 typedef std::function<void(const LLSD&)> LLVoidListener;
 /// Result of registering a listener, supports <tt>connected()</tt>,
@@ -359,56 +364,13 @@ testable:
 };
 
 /*****************************************************************************
-*   LLEventTrackable
-*****************************************************************************/
-/**
- * LLEventTrackable wraps boost::signals2::trackable, which resembles
- * boost::trackable. Derive your listener class from LLEventTrackable instead,
- * and use something like
- * <tt>LLEventPump::listen(boost::bind(&YourTrackableSubclass::method,
- * instance, _1))</tt>. This will implicitly disconnect when the object
- * referenced by @c instance is destroyed.
- *
- * @note
- * LLEventTrackable doesn't address a couple of cases:
- * * Object destroyed during call
- *   - You enter a slot call in thread A.
- *   - Thread B destroys the object, which of course disconnects it from any
- *     future slot calls.
- *   - Thread A's call uses 'this', which now refers to a defunct object.
- *     Undefined behavior results.
- * * Call during destruction
- *   - @c MySubclass is derived from LLEventTrackable.
- *   - @c MySubclass registers one of its own methods using
- *     <tt>LLEventPump::listen()</tt>.
- *   - The @c MySubclass object begins destruction. <tt>~MySubclass()</tt>
- *     runs, destroying state specific to the subclass. (For instance, a
- *     <tt>Foo*</tt> data member is <tt>delete</tt>d but not zeroed.)
- *   - The listening method will not be disconnected until
- *     <tt>~LLEventTrackable()</tt> runs.
- *   - Before we get there, another thread posts data to the @c LLEventPump
- *     instance, calling the @c MySubclass method.
- *   - The method in question relies on valid @c MySubclass state. (For
- *     instance, it attempts to dereference the <tt>Foo*</tt> pointer that was
- *     <tt>delete</tt>d but not zeroed.)
- *   - Undefined behavior results.
- */
-typedef boost::signals2::trackable LLEventTrackable;
-
-/*****************************************************************************
 *   LLEventPump
 *****************************************************************************/
 /**
  * LLEventPump is the base class interface through which we access the
  * concrete subclasses such as LLEventStream.
- *
- * @NOTE
- * LLEventPump derives from LLEventTrackable so that when you "chain"
- * LLEventPump instances together, they will automatically disconnect on
- * destruction. Please see LLEventTrackable documentation for situations in
- * which this may be perilous across threads.
  */
-class LL_COMMON_API LLEventPump: public LLEventTrackable
+class LL_COMMON_API LLEventPump
 {
 public:
     static const std::string ANONYMOUS; // constant for anonymous listeners.
@@ -522,18 +484,37 @@ public:
      * call, allows us to optimize away the second and subsequent dependency
      * sorts.
      *
-     * If name is set to LLEventPump::ANONYMOUS listen will bypass the entire
+     * If name is set to LLEventPump::ANONYMOUS, listen() will bypass the entire
      * dependency and ordering calculation. In this case, it is critical that
      * the result be assigned to a LLTempBoundListener or the listener is
-     * manually disconnected when no longer needed since there will be no
+     * manually disconnected when no longer needed, since there will be no
      * way to later find and disconnect this listener manually.
      */
+    template <typename LISTENER>
     LLBoundListener listen(const std::string& name,
-                           const LLEventListener& listener,
+                           LISTENER&& listener,
                            const NameList& after=NameList(),
                            const NameList& before=NameList())
     {
-        return listen_impl(name, listener, after, before);
+        if constexpr (std::is_invocable_v<LISTENER, const LLSD&>)
+        {
+            // wrap classic LLEventListener in LLAwareListener lambda
+            return listenb(
+                name,
+                [listener=std::move(listener)]
+                (const LLBoundListener&, const LLSD& event)
+                {
+                    return listener(event);
+                },
+                after,
+                before);
+        }
+        else
+        {
+            static_assert(std::is_invocable_v<LISTENER, LLBoundListener, const LLSD&>,
+                          "LLEventPump::listen() listener has bad parameter signature");
+            return listenb(name, std::forward<LISTENER>(listener), after, before);
+        }
     }
 
     /// Get the LLBoundListener associated with the passed name (dummy
@@ -579,7 +560,35 @@ private:
     LLMutex mConnectionListMutex;
 
 protected:
-    virtual LLBoundListener listen_impl(const std::string& name, const LLEventListener&,
+    template <typename LISTENER>
+    LLBoundListener listenb(const std::string& name,
+                            LISTENER&& listener,
+                            const NameList& after=NameList(),
+                            const NameList& before=NameList())
+    {
+        using result_t = std::decay_t<decltype(listener(LLBoundListener(), LLSD()))>;
+        if constexpr (std::is_same_v<bool, result_t>)
+        {
+            return listen_impl(name, std::forward<LISTENER>(listener), after, before);
+        }
+        else
+        {
+            static_assert(std::is_same_v<void, result_t>,
+                          "LLEventPump::listen() listener has bad return type");
+            // wrap void listener in one that returns bool
+            return listen_impl(
+                name,
+                [listener=std::move(listener)]
+                (const LLBoundListener& conn, const LLSD& event)
+                {
+                    listener(conn, event);
+                    return false;
+                },
+                after,
+                before);
+        }
+    }
+    virtual LLBoundListener listen_impl(const std::string& name, const LLAwareListener&,
                                         const NameList& after,
                                         const NameList& before);
 
@@ -654,7 +663,7 @@ public:
     void discard();
 
 protected:
-    virtual LLBoundListener listen_impl(const std::string& name, const LLEventListener&,
+    virtual LLBoundListener listen_impl(const std::string& name, const LLAwareListener&,
                                         const NameList& after,
                                         const NameList& before) override;
 
diff --git a/indra/llui/llnotifications.h b/indra/llui/llnotifications.h
index ab4f009a80..206c521592 100644
--- a/indra/llui/llnotifications.h
+++ b/indra/llui/llnotifications.h
@@ -734,7 +734,7 @@ typedef std::multimap<std::string, LLNotificationPtr> LLNotificationMap;
 // all of the built-in tests should attach to the "Visible" channel
 //
 class LLNotificationChannelBase :
-    public LLEventTrackable,
+    public boost::signals2::trackable,
     public LLRefCount
 {
     LOG_CLASS(LLNotificationChannelBase);
diff --git a/indra/llui/llnotificationslistener.cpp b/indra/llui/llnotificationslistener.cpp
index ace9e37e25..2ad1689a45 100644
--- a/indra/llui/llnotificationslistener.cpp
+++ b/indra/llui/llnotificationslistener.cpp
@@ -204,7 +204,7 @@ void LLNotificationsListener::ignore(const LLSD& params) const
     }
 }
 
-class LLNotificationsListener::Forwarder: public LLEventTrackable
+class LLNotificationsListener::Forwarder: public boost::signals2::trackable
 {
     LOG_CLASS(LLNotificationsListener::Forwarder);
 public:
@@ -213,8 +213,10 @@ public:
         mRespond(false)
     {
         // Connect to the specified channel on construction. Because
-        // LLEventTrackable is a base, we should automatically disconnect when
-        // destroyed.
+        // boost::signals2::trackable is a base, because we use boost::bind()
+        // below, and because connectPassedFilter() directly calls
+        // boost::signals2::signal::connect(), we should automatically
+        // disconnect when destroyed.
         LLNotificationChannelPtr channelptr(llnotifications.getChannel(channel));
         if (channelptr)
         {
@@ -252,10 +254,10 @@ void LLNotificationsListener::forward(const LLSD& params)
     if (! forward)
     {
         // This is a request to stop forwarding notifications on the specified
-        // channel. The rest of the params don't matter.
-        // Because mForwarders contains scoped_ptrs, erasing the map entry
-        // DOES delete the heap Forwarder object. Because Forwarder derives
-        // from LLEventTrackable, destroying it disconnects it from the
+        // channel. The rest of the params don't matter. Because mForwarders
+        // contains scoped_ptrs, erasing the map entry DOES delete the heap
+        // Forwarder object. Because Forwarder derives from
+        // boost::signals2::trackable, destroying it disconnects it from the
         // channel.
         mForwarders.erase(channel);
         return;
diff --git a/indra/newview/llspeakers.h b/indra/newview/llspeakers.h
index 234de42953..d3304dba59 100644
--- a/indra/newview/llspeakers.h
+++ b/indra/newview/llspeakers.h
@@ -37,7 +37,7 @@ class LLSpeakerMgr;
 class LLAvatarName;
 
 // data for a given participant in a voice channel
-class LLSpeaker : public LLRefCount, public LLOldEvents::LLObservable, public LLHandleProvider<LLSpeaker>, public boost::signals2::trackable
+class LLSpeaker : public LLRefCount, public LLOldEvents::LLObservable, public LLHandleProvider<LLSpeaker>
 {
 public:
     typedef enum e_speaker_type
diff --git a/indra/newview/tests/llluamanager_test.cpp b/indra/newview/tests/llluamanager_test.cpp
index 26a4ac95e3..3209d93d39 100644
--- a/indra/newview/tests/llluamanager_test.cpp
+++ b/indra/newview/tests/llluamanager_test.cpp
@@ -42,17 +42,6 @@ public:
 
 LLControlGroup gSavedSettings("Global");
 
-template <typename CALLABLE>
-auto listener(CALLABLE&& callable)
-{
-    return [callable=std::forward<CALLABLE>(callable)]
-    (const LLSD& data)
-    {
-        callable(data);
-        return false;
-    };
-}
-
 /*****************************************************************************
 *   TUT
 *****************************************************************************/
@@ -138,7 +127,7 @@ namespace tut
     {
         LLSD fromlua;
         LLStreamListener pump("testpump",
-                              listener([&fromlua](const LLSD& data){ fromlua = data; }));
+                              [&fromlua](const LLSD& data){ fromlua = data; });
         const std::string lua(stringize(
             "data = ", construct, "\n"
             "LL.post_on('testpump', data)\n"
@@ -167,8 +156,8 @@ namespace tut
         set_test_name("test post_on(), get_event_pumps(), get_event_next()");
         StringVec posts;
         LLStreamListener pump("testpump",
-                              listener([&posts](const LLSD& data)
-                              { posts.push_back(data.asString()); }));
+                              [&posts](const LLSD& data)
+                              { posts.push_back(data.asString()); });
         const std::string lua(
             "-- test post_on,get_event_pumps,get_event_next\n"
             "LL.post_on('testpump', 'entry')\n"
@@ -346,11 +335,11 @@ namespace tut
 
         LLStreamListener pump(
             "echo",
-            listener([](const LLSD& data)
+            [](const LLSD& data)
             {
                 LL_DEBUGS("Lua") << "echo pump got: " << data << LL_ENDL;
                 sendReply(data, data);
-            }));
+            });
 
         auto [count, result] = LLLUAmanager::waitScriptLine(lua);
         ensure_equals("Lua script didn't return item", count, 1);
@@ -424,11 +413,11 @@ namespace tut
         LLSD requests;
         LLStreamListener pump(
             "testpump",
-            listener([&requests](const LLSD& data)
+            [&requests](const LLSD& data)
             {
                 LL_DEBUGS("Lua") << "testpump got: " << data << LL_ENDL;
                 requests.append(data);
-            }));
+            });
 
         auto future = LLLUAmanager::startScriptLine(lua);
         // LuaState::expr() periodically interrupts a running chunk to ensure
diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp
index 875ca9ad89..f9cc99203b 100644
--- a/indra/test/llevents_tut.cpp
+++ b/indra/test/llevents_tut.cpp
@@ -429,7 +429,7 @@ void events_object::test<9>()
 {
     set_test_name("listen(boost::bind(...TempListener...))");
     // listen() can't do anything about a plain TempListener instance:
-    // it's not managed with shared_ptr, nor is it an LLEventTrackable subclass
+    // it's not managed with shared_ptr
     bool live = false;
     LLEventPump& heaptest(pumps.obtain("heaptest"));
     LLBoundListener connection;
@@ -453,60 +453,4 @@ void events_object::test<9>()
     heaptest.stopListening("temp");
 }
 
-class TempTrackableListener: public TempListener, public LLEventTrackable
-{
-public:
-    TempTrackableListener(const std::string& name, bool& liveFlag):
-        TempListener(name, liveFlag)
-    {}
-};
-
-template<> template<>
-void events_object::test<10>()
-{
-    set_test_name("listen(boost::bind(...TempTrackableListener ref...))");
-    bool live = false;
-    LLEventPump& heaptest(pumps.obtain("heaptest"));
-    LLBoundListener connection;
-    {
-        TempTrackableListener tempListener("temp", live);
-        ensure("TempTrackableListener constructed", live);
-        connection = heaptest.listen(tempListener.getName(),
-                                     boost::bind(&TempTrackableListener::call,
-                                                 boost::ref(tempListener), _1));
-        heaptest.post(1);
-        check_listener("received", tempListener, 1);
-    } // presumably this will make tempListener go away?
-    // verify that
-    ensure("TempTrackableListener destroyed", ! live);
-    ensure("implicit disconnect", ! connection.connected());
-    // now just make sure we don't blow up trying to access a freed object!
-    heaptest.post(2);
-}
-
-template<> template<>
-void events_object::test<11>()
-{
-    set_test_name("listen(boost::bind(...TempTrackableListener pointer...))");
-    bool live = false;
-    LLEventPump& heaptest(pumps.obtain("heaptest"));
-    LLBoundListener connection;
-    {
-        TempTrackableListener* newListener(new TempTrackableListener("temp", live));
-        ensure("TempTrackableListener constructed", live);
-        connection = heaptest.listen(newListener->getName(),
-                                     boost::bind(&TempTrackableListener::call,
-                                                 newListener, _1));
-        heaptest.post(1);
-        check_listener("received", *newListener, 1);
-        // explicitly destroy newListener
-        delete newListener;
-    }
-    // verify that
-    ensure("TempTrackableListener destroyed", ! live);
-    ensure("implicit disconnect", ! connection.connected());
-    // now just make sure we don't blow up trying to access a freed object!
-    heaptest.post(2);
-}
-
 } // namespace tut
diff --git a/indra/viewer_components/login/tests/lllogin_test.cpp b/indra/viewer_components/login/tests/lllogin_test.cpp
index 8aea3b37aa..f051f8c67f 100644
--- a/indra/viewer_components/login/tests/lllogin_test.cpp
+++ b/indra/viewer_components/login/tests/lllogin_test.cpp
@@ -66,7 +66,7 @@
 *   Helper classes
 *****************************************************************************/
 // This is a listener to receive results from lllogin.
-class LoginListener: public LLEventTrackable
+class LoginListener
 {
     std::string mName;
     LLSD mLastEvent;
@@ -137,7 +137,7 @@ public:
     }
 };
 
-class LLXMLRPCListener: public LLEventTrackable
+class LLXMLRPCListener
 {
     std::string mName;
     LLSD mEvent;