From 0c32c98f25f3f7d99a244a249dc08b03cd36a5b0 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 29 Dec 2018 10:15:28 -0500 Subject: SL-793: Add LLEventPumps::clear() method to disconnect all listeners. This is like the existing reset() method, except that reset() is specifically intended for shutdown: it disables every existing LLEventPump in such a way that it cannot be subsequently reused. (The original idea was to disconnect listeners in DLLs unloaded at shutdown.) clear() forcibly disconnects all existing listeners, but leaves LLEventPumps ready for reuse. This is useful (e.g.) for test programs to reset the state of LLEventPumps between individual test functions. --- indra/llcommon/llevents.cpp | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) (limited to 'indra/llcommon/llevents.cpp') diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp index eedd8c92b5..99abb333bb 100644 --- a/indra/llcommon/llevents.cpp +++ b/indra/llcommon/llevents.cpp @@ -45,6 +45,7 @@ #include // external library headers #include +#include #if LL_WINDOWS #pragma warning (push) #pragma warning (disable : 4701) // compiler thinks might use uninitialized var, but no @@ -154,13 +155,23 @@ void LLEventPumps::flush() } } +void LLEventPumps::clear() +{ + // Clear every known LLEventPump instance. Leave it up to each instance to + // decide what to do with the clear() call. + for (PumpMap::value_type& pair : mPumpMap) + { + pair.second->clear(); + } +} + void LLEventPumps::reset() { // Reset every known LLEventPump instance. Leave it up to each instance to // decide what to do with the reset() call. - for (PumpMap::iterator pmi = mPumpMap.begin(), pmend = mPumpMap.end(); pmi != pmend; ++pmi) + for (PumpMap::value_type& pair : mPumpMap) { - pmi->second->reset(); + pair.second->reset(); } } @@ -283,7 +294,7 @@ LLEventPump::LLEventPump(const std::string& name, bool tweak): // Register every new instance with LLEventPumps mRegistry(LLEventPumps::instance().getHandle()), mName(mRegistry.get()->registerNew(*this, name, tweak)), - mSignal(new LLStandardSignal()), + mSignal(boost::make_shared()), mEnabled(true) {} @@ -311,6 +322,14 @@ std::string LLEventPump::inventName(const std::string& pfx) return STRINGIZE(pfx << suffix++); } +void LLEventPump::clear() +{ + // Destroy the original LLStandardSignal instance, replacing it with a + // whole new one. + mSignal = boost::make_shared(); + mConnections.clear(); +} + void LLEventPump::reset() { mSignal.reset(); @@ -553,7 +572,7 @@ bool LLEventMailDrop::post(const LLSD& event) // be posted to any future listeners when they attach. mEventHistory.push_back(event); } - + return posted; } -- cgit v1.3 From 6805e82acdcde9a3f18681427a33a5bc7be7a0a6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 17 Oct 2019 12:04:02 -0400 Subject: DRTVWR-476: Introduce LLEventMailDrop::discard() (instead of flush()). Overriding virtual LLEventPump::flush() for the semantic of discarding LLEventMailDrop's queued events turns out not to be such a great idea, because LLEventPumps::flush(), which calls every registered LLEventPump's flush() method, is called every mainloop tick. The first time we hit a use case in which we expected LLEventMailDrop to hold queued events across a mainloop tick, we were baffled that they were never delivered. Moving that logic to a separate method specific to LLEventMailDrop resolves that problem. Naming it discard() clarifies its intended functionality. --- indra/llcommon/llevents.cpp | 9 +++++++-- indra/llcommon/llevents.h | 3 ++- indra/newview/llvoicevivox.cpp | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) (limited to 'indra/llcommon/llevents.cpp') diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp index 99abb333bb..186e710c43 100644 --- a/indra/llcommon/llevents.cpp +++ b/indra/llcommon/llevents.cpp @@ -602,6 +602,11 @@ LLBoundListener LLEventMailDrop::listen_impl(const std::string& name, return LLEventStream::listen_impl(name, listener, after, before); } +void LLEventMailDrop::discard() +{ + mEventHistory.clear(); + LLEventStream::flush(); +} /***************************************************************************** * LLEventQueue @@ -621,8 +626,8 @@ bool LLEventQueue::post(const LLSD& event) void LLEventQueue::flush() { - if(!mSignal) return; - + if(!mSignal) return; + // Consider the case when a given listener on this LLEventQueue posts yet // another event on the same queue. If we loop over mEventQueue directly, // we'll end up processing all those events during the same flush() call diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h index 3c388bf176..ce2aa2f3c9 100644 --- a/indra/llcommon/llevents.h +++ b/indra/llcommon/llevents.h @@ -610,7 +610,8 @@ public: virtual bool post(const LLSD& event) override; /// Remove any history stored in the mail drop. - virtual void flush() override { mEventHistory.clear(); LLEventStream::flush(); }; + void discard(); + protected: virtual LLBoundListener listen_impl(const std::string& name, const LLEventListener&, const NameList& after, diff --git a/indra/newview/llvoicevivox.cpp b/indra/newview/llvoicevivox.cpp index 7d9085a214..1141b29163 100644 --- a/indra/newview/llvoicevivox.cpp +++ b/indra/newview/llvoicevivox.cpp @@ -1488,7 +1488,7 @@ bool LLVivoxVoiceClient::addAndJoinSession(const sessionStatePtr_t &nextSession) // We are about to start a whole new session. Anything that MIGHT still be in our // maildrop is going to be stale and cause us much wailing and gnashing of teeth. // Just flush it all out and start new. - mVivoxPump.flush(); + mVivoxPump.discard(); // It appears that I need to wait for BOTH the SessionGroup.AddSession response and the SessionStateChangeEvent with state 4 // before continuing from this state. They can happen in either order, and if I don't wait for both, things can get stuck. -- cgit v1.3 From 79a3e391d3c992925230ff01551747e7edccb0ca Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 17 Oct 2019 13:26:51 -0400 Subject: DRTVWR-476: Kill LLEventQueue, per-frame LLEventPump::flush() calls. No one uses LLEventQueue to defer posted events until the next mainloop tick -- and with LLCoros moving to Boost.Fiber, cross-coroutine event posting works that way anyway, making LLEventQueue pretty unnecessary. The static RegisterFlush instance in llevents.cpp was used to call LLEventPumps::flush() once per mainloop tick, which in turn called flush() on every registered LLEventPump. But the only reason for that mechanism was to support LLEventQueue. In fact, when LLEventMailDrop overrode its flush() method for something quite different, it was startling to find that the new flush() override was being called once per frame -- which caused at least one fairly mysterious bug. Remove RegisterFlush. Both LLEventPumps::flush() and LLEventPump::flush() remain for now, though intended usage is unclear. Eliminating LLEventQueue means we must at least repurpose LLEventPumps::mQueueNames, a map intended to make LLEventPumps::obtain() instantiate an LLEventQueue rather than the default LLEventPump. Replace it with mFactories, a map from desired instance name to a callable returning LLEventPump*. New map initialization syntax plus lambda support allows us to populate that map at compile time with little lambdas returning the correct subclass instance. Similarly, LLLeapListener::newpump() used to check the ["type"] entry in the LLSD request specifically for "LLEventQueue". Introduce another such map in llleaplistener.cpp for potential future extensibility. Eliminate the LLEventQueue-specific test. --- indra/llcommon/llevents.cpp | 102 +++++--------------------------------- indra/llcommon/llevents.h | 39 +++------------ indra/llcommon/llleaplistener.cpp | 21 ++++++-- indra/test/llevents_tut.cpp | 57 ++++----------------- 4 files changed, 46 insertions(+), 173 deletions(-) (limited to 'indra/llcommon/llevents.cpp') diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp index 186e710c43..d31f5f2d32 100644 --- a/indra/llcommon/llevents.cpp +++ b/indra/llcommon/llevents.cpp @@ -64,51 +64,16 @@ #endif /***************************************************************************** -* queue_names: specify LLEventPump names that should be instantiated as -* LLEventQueue -*****************************************************************************/ -/** - * At present, we recognize particular requested LLEventPump names as needing - * LLEventQueues. Later on we'll migrate this information to an external - * configuration file. - */ -const char* queue_names[] = -{ - "placeholder - replace with first real name string" -}; - -/***************************************************************************** -* If there's a "mainloop" pump, listen on that to flush all LLEventQueues +* LLEventPumps *****************************************************************************/ -struct RegisterFlush : public LLEventTrackable +LLEventPumps::PumpFactories LLEventPumps::mFactories { - RegisterFlush(): - pumps(LLEventPumps::instance()) - { - pumps.obtain("mainloop").listen("flushLLEventQueues", boost::bind(&RegisterFlush::flush, this, _1)); - } - bool flush(const LLSD&) - { - pumps.flush(); - return false; - } - ~RegisterFlush() - { - // LLEventTrackable handles stopListening for us. - } - LLEventPumps& pumps; + // LLEventStream is the default for obtain(), so even if somebody DOES + // call obtain("placeholder"), this sample entry won't break anything. + { "placeholder", [](const std::string& name) { return new LLEventStream(name); } } }; -static RegisterFlush registerFlush; -/***************************************************************************** -* LLEventPumps -*****************************************************************************/ -LLEventPumps::LLEventPumps(): - // Until we migrate this information to an external config file, - // initialize mQueueNames from the static queue_names array. - mQueueNames(boost::begin(queue_names), boost::end(queue_names)) -{ -} +LLEventPumps::LLEventPumps() {} LLEventPump& LLEventPumps::obtain(const std::string& name) { @@ -121,10 +86,10 @@ LLEventPump& LLEventPumps::obtain(const std::string& name) } // Here we must instantiate an LLEventPump subclass. LLEventPump* newInstance; - // Should this name be an LLEventQueue? - PumpNames::const_iterator nfound = mQueueNames.find(name); - if (nfound != mQueueNames.end()) - newInstance = new LLEventQueue(name); + // Do we have a predefined factory for this instance name? + PumpFactories::const_iterator nfound = mFactories.find(name); + if (nfound != mFactories.end()) + newInstance = (nfound->second)(name); else newInstance = new LLEventStream(name); // LLEventPump's constructor implicitly registers each new instance in @@ -144,14 +109,13 @@ bool LLEventPumps::post(const std::string&name, const LLSD&message) return (*found).second->post(message); } - void LLEventPumps::flush() { // Flush every known LLEventPump instance. Leave it up to each instance to // decide what to do with the flush() call. - for (PumpMap::iterator pmi = mPumpMap.begin(), pmend = mPumpMap.end(); pmi != pmend; ++pmi) + for (PumpMap::value_type& pair : mPumpMap) { - pmi->second->flush(); + pair.second->flush(); } } @@ -605,48 +569,6 @@ LLBoundListener LLEventMailDrop::listen_impl(const std::string& name, void LLEventMailDrop::discard() { mEventHistory.clear(); - LLEventStream::flush(); -} - -/***************************************************************************** -* LLEventQueue -*****************************************************************************/ -bool LLEventQueue::post(const LLSD& event) -{ - if (mEnabled) - { - // Defer sending this event by queueing it until flush() - mEventQueue.push_back(event); - } - // Unconditionally return false. We won't know until flush() whether a - // listener claims to have handled the event -- meanwhile, don't block - // other listeners. - return false; -} - -void LLEventQueue::flush() -{ - if(!mSignal) return; - - // Consider the case when a given listener on this LLEventQueue posts yet - // another event on the same queue. If we loop over mEventQueue directly, - // we'll end up processing all those events during the same flush() call - // -- rather like an EventStream. Instead, copy mEventQueue and clear it, - // so that any new events posted to this LLEventQueue during flush() will - // be processed in the *next* flush() call. - EventQueue queue(mEventQueue); - mEventQueue.clear(); - // NOTE NOTE NOTE: Any new access to member data beyond this point should - // cause us to move our LLStandardSignal object to a pimpl class along - // with said member data. Then the local shared_ptr will preserve both. - - // DEV-43463: capture a local copy of mSignal. See LLEventStream::post() - // for detailed comments. - boost::shared_ptr signal(mSignal); - for ( ; ! queue.empty(); queue.pop_front()) - { - (*signal)(queue.front()); - } } /***************************************************************************** diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h index ce2aa2f3c9..c55351919e 100644 --- a/indra/llcommon/llevents.h +++ b/indra/llcommon/llevents.h @@ -37,6 +37,7 @@ #include #include #include +#include #if LL_WINDOWS #pragma warning (push) #pragma warning (disable : 4263) // boost::signals2::expired_slot::what() has const mismatch @@ -55,7 +56,6 @@ #include #include // reference_wrapper #include -#include #include #include "llsd.h" #include "llsingleton.h" @@ -303,10 +303,10 @@ testable: // destroyed. typedef std::set PumpSet; PumpSet mOurPumps; - // LLEventPump names that should be instantiated as LLEventQueue rather - // than as LLEventStream - typedef std::set PumpNames; - PumpNames mQueueNames; + // LLEventPump subclasses that should be instantiated for particular + // instance names + typedef std::map> PumpFactories; + static PumpFactories mFactories; }; /***************************************************************************** @@ -351,7 +351,7 @@ typedef boost::signals2::trackable LLEventTrackable; *****************************************************************************/ /** * LLEventPump is the base class interface through which we access the - * concrete subclasses LLEventStream and LLEventQueue. + * concrete subclasses such as LLEventStream. * * @NOTE * LLEventPump derives from LLEventTrackable so that when you "chain" @@ -594,11 +594,10 @@ public: * event *must* eventually reach a listener that will consume it, else the * queue will grow to arbitrary length. * - * @NOTE: When using an LLEventMailDrop (or LLEventQueue) with a LLEventTimeout or + * @NOTE: When using an LLEventMailDrop with an LLEventTimeout or * LLEventFilter attaching the filter downstream, using Timeout's constructor will * cause the MailDrop to discharge any of its stored events. The timeout should * instead be connected upstream using its listen() method. - * See llcoro::suspendUntilEventOnWithTimeout() for an example. */ class LL_COMMON_API LLEventMailDrop : public LLEventStream { @@ -622,30 +621,6 @@ private: EventList mEventHistory; }; -/***************************************************************************** -* LLEventQueue -*****************************************************************************/ -/** - * LLEventQueue is a LLEventPump whose post() method defers calling registered - * listeners until flush() is called. - */ -class LL_COMMON_API LLEventQueue: public LLEventPump -{ -public: - LLEventQueue(const std::string& name, bool tweak=false): LLEventPump(name, tweak) {} - virtual ~LLEventQueue() {} - - /// Post an event to all listeners - virtual bool post(const LLSD& event); - - /// flush queued events - virtual void flush(); - -private: - typedef std::deque EventQueue; - EventQueue mEventQueue; -}; - /***************************************************************************** * LLReqID *****************************************************************************/ diff --git a/indra/llcommon/llleaplistener.cpp b/indra/llcommon/llleaplistener.cpp index f50bacb1e8..0d18e5fff9 100644 --- a/indra/llcommon/llleaplistener.cpp +++ b/indra/llcommon/llleaplistener.cpp @@ -14,6 +14,8 @@ // associated header #include "llleaplistener.h" // STL headers +#include +#include // std headers // external library headers #include @@ -119,6 +121,18 @@ LLLeapListener::~LLLeapListener() } } +namespace +{ + +static std::map> factories +{ + // tweak name for uniqueness + { "LLEventStream", [](const std::string& name){ return new LLEventStream(name, true); } }, + { "LLEventMailDrop", [](const std::string& name){ return new LLEventMailDrop(name, true); } } +}; + +} // anonymous namespace + void LLLeapListener::newpump(const LLSD& request) { Response reply(LLSD(), request); @@ -127,13 +141,14 @@ void LLLeapListener::newpump(const LLSD& request) LLSD const & type = request["type"]; LLEventPump * new_pump = NULL; - if (type.asString() == "LLEventQueue") + auto found = factories.find(type.asString()); + if (found != factories.end()) { - new_pump = new LLEventQueue(name, true); // tweak name for uniqueness + new_pump = (found->second)(name); } else { - if (! (type.isUndefined() || type.asString() == "LLEventStream")) + if (! type.isUndefined()) { reply.warn(STRINGIZE("unknown 'type' " << type << ", using LLEventStream")); } diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp index a8a3188249..17f64a4953 100644 --- a/indra/test/llevents_tut.cpp +++ b/indra/test/llevents_tut.cpp @@ -91,9 +91,7 @@ template<> template<> void events_object::test<1>() { set_test_name("basic operations"); - // Now there's a static constructor in llevents.cpp that registers on - // the "mainloop" pump to call LLEventPumps::flush(). - // Actually -- having to modify this to track the statically- + // Having to modify this to track the statically- // constructed pumps in other TUT modules in this giant monolithic test // executable isn't such a hot idea. // ensure_equals("initial pump", pumps.mPumpMap.size(), 1); @@ -209,43 +207,6 @@ bool chainEvents(Listener& someListener, const LLSD& event) template<> template<> void events_object::test<3>() -{ - set_test_name("LLEventQueue delayed action"); - // This access is NOT legal usage: we can do it only because we're - // hacking private for test purposes. Normally we'd either compile in - // a particular name, or (later) edit a config file. - pumps.mQueueNames.insert("login"); - LLEventPump& login(pumps.obtain("login")); - // The "mainloop" pump is special: posting on that implicitly calls - // LLEventPumps::flush(), which in turn should flush our "login" - // LLEventQueue. - LLEventPump& mainloop(pumps.obtain("mainloop")); - ensure("LLEventQueue leaf class", dynamic_cast (&login)); - listener0.listenTo(login); - listener0.reset(0); - login.post(1); - check_listener("waiting for queued event", listener0, 0); - mainloop.post(LLSD()); - check_listener("got queued event", listener0, 1); - login.stopListening(listener0.getName()); - // Verify that when an event handler posts a new event on the same - // LLEventQueue, it doesn't get processed in the same flush() call -- - // it waits until the next flush() call. - listener0.reset(17); - login.listen("chainEvents", boost::bind(chainEvents, boost::ref(listener0), _1)); - login.post(1); - check_listener("chainEvents(1) not yet called", listener0, 17); - mainloop.post(LLSD()); - check_listener("chainEvents(1) called", listener0, 1); - mainloop.post(LLSD()); - check_listener("chainEvents(0) called", listener0, 0); - mainloop.post(LLSD()); - check_listener("chainEvents(-1) not called", listener0, 0); - login.stopListening("chainEvents"); -} - -template<> template<> -void events_object::test<4>() { set_test_name("explicitly-instantiated LLEventStream"); // Explicitly instantiate an LLEventStream, and verify that it @@ -270,7 +231,7 @@ void events_object::test<4>() } template<> template<> -void events_object::test<5>() +void events_object::test<4>() { set_test_name("stopListening()"); LLEventPump& login(pumps.obtain("login")); @@ -284,7 +245,7 @@ void events_object::test<5>() } template<> template<> -void events_object::test<6>() +void events_object::test<5>() { set_test_name("chaining LLEventPump instances"); LLEventPump& upstream(pumps.obtain("upstream")); @@ -309,7 +270,7 @@ void events_object::test<6>() } template<> template<> -void events_object::test<7>() +void events_object::test<6>() { set_test_name("listener dependency order"); typedef LLEventPump::NameList NameList; @@ -391,7 +352,7 @@ void events_object::test<7>() } template<> template<> -void events_object::test<8>() +void events_object::test<7>() { set_test_name("tweaked and untweaked LLEventPump instance names"); { // nested scope @@ -423,7 +384,7 @@ void eventSource(const LLListenerOrPumpName& listener) } template<> template<> -void events_object::test<9>() +void events_object::test<8>() { set_test_name("LLListenerOrPumpName"); // Passing a boost::bind() expression to LLListenerOrPumpName @@ -464,7 +425,7 @@ private: }; template<> template<> -void events_object::test<10>() +void events_object::test<9>() { set_test_name("listen(boost::bind(...TempListener...))"); // listen() can't do anything about a plain TempListener instance: @@ -501,7 +462,7 @@ public: }; template<> template<> -void events_object::test<11>() +void events_object::test<10>() { set_test_name("listen(boost::bind(...TempTrackableListener ref...))"); bool live = false; @@ -524,7 +485,7 @@ void events_object::test<11>() } template<> template<> -void events_object::test<12>() +void events_object::test<11>() { set_test_name("listen(boost::bind(...TempTrackableListener pointer...))"); bool live = false; -- cgit v1.3 From 95a218fcc0d22cdf02a7c81c555b9c909e24bd40 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 18 Oct 2019 14:46:05 -0400 Subject: DRTVWR-476: Eliminate static LLEventPump factory maps. Having a map from std::string to a factory function returning LLEventPump* is a cool idea, especially since you could statically populate such a map with string literals and little lambdas. Unfortunately, static initialization of any data is a bad idea when control can reach consuming code before that module's static data are constructed. Since LLEventPumps is already an LLSingleton, it's simple enough to make its map non-static and initialize it in the constructor. But another recent static factory-function map was introduced in llleaplistener.cpp to support the LLLeapListener::newpump() operation. That involves no LLSingletons. Introduce LLEventPumps::make(name, tweak, type) to instantiate an LLEventPump subclass of the specified type with specified (name, tweak) parameters. Instances returned by make() are owned by LLEventPumps, as with obtain(). Introduce LLEventPumps::BadType exception for when the type string isn't recognized. LLEventPumps::obtain() can then simply call make() when the specified instance name doesn't already exist. The configuration data used internally by obtain() becomes { string instance name, string subclass name }. Although this too is currently initialized in the LLEventPumps constructor, migrating it to a configuration file would now be far more straightforward than before. LLLeapListener::newpump(), too, can call LLEventPumps::make() with the caller-specified type string. This eliminates that static factory map. newpump() must catch BadType and report the error back to its invoker. Given that the LLEventPump subclass instances returned by make() are owned by LLEventPumps rather than LLLeapListener, there is no further need for the LLLeapListener::mEventPumps ptr_map, which was used only to manage lifetime. Also remove LLLeapListener's "killpump" operation since LLEventPumps provides no corresponding functionality. --- indra/llcommon/llevents.cpp | 56 +++++++++++++++++++++++++---------- indra/llcommon/llevents.h | 55 ++++++++++++++++++++++++++--------- indra/llcommon/llleaplistener.cpp | 61 +++++++++------------------------------ indra/llcommon/llleaplistener.h | 5 ---- 4 files changed, 95 insertions(+), 82 deletions(-) (limited to 'indra/llcommon/llevents.cpp') diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp index d31f5f2d32..281a1121bd 100644 --- a/indra/llcommon/llevents.cpp +++ b/indra/llcommon/llevents.cpp @@ -66,14 +66,21 @@ /***************************************************************************** * LLEventPumps *****************************************************************************/ -LLEventPumps::PumpFactories LLEventPumps::mFactories -{ - // LLEventStream is the default for obtain(), so even if somebody DOES - // call obtain("placeholder"), this sample entry won't break anything. - { "placeholder", [](const std::string& name) { return new LLEventStream(name); } } -}; - -LLEventPumps::LLEventPumps() {} +LLEventPumps::LLEventPumps(): + mFactories + { + { "LLEventStream", [](const std::string& name, bool tweak) + { return new LLEventStream(name, tweak); } }, + { "LLEventMailDrop", [](const std::string& name, bool tweak) + { return new LLEventMailDrop(name, tweak); } } + }, + mTypes + { + // LLEventStream is the default for obtain(), so even if somebody DOES + // call obtain("placeholder"), this sample entry won't break anything. + { "placeholder", "LLEventStream" } + } +{} LLEventPump& LLEventPumps::obtain(const std::string& name) { @@ -84,14 +91,31 @@ LLEventPump& LLEventPumps::obtain(const std::string& name) // name. return *found->second; } - // Here we must instantiate an LLEventPump subclass. - LLEventPump* newInstance; - // Do we have a predefined factory for this instance name? - PumpFactories::const_iterator nfound = mFactories.find(name); - if (nfound != mFactories.end()) - newInstance = (nfound->second)(name); - else - newInstance = new LLEventStream(name); + + // Here we must instantiate an LLEventPump subclass. Is there a + // preregistered class name override for this specific instance name? + auto nfound = mTypes.find(name); + std::string type; + if (nfound != mTypes.end()) + { + type = nfound->second; + } + // pass tweak=false: we already know there's no existing instance with + // this name + return make(name, false, type); +} + +LLEventPump& LLEventPumps::make(const std::string& name, bool tweak, + const std::string& type) +{ + // find the relevant factory for this (or default) type + auto found = mFactories.find(type.empty()? "LLEventStream" : type); + if (found == mFactories.end()) + { + // Passing an unrecognized type name is a no-no + LLTHROW(BadType(type)); + } + auto newInstance = (found->second)(name, tweak); // LLEventPump's constructor implicitly registers each new instance in // mPumpMap. But remember that we instantiated it (in mOurPumps) so we'll // delete it later. diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h index c55351919e..e380c108f4 100644 --- a/indra/llcommon/llevents.h +++ b/indra/llcommon/llevents.h @@ -211,8 +211,7 @@ public: /// exception if you try to call when empty struct Empty: public LLException { - Empty(const std::string& what): - LLException(std::string("LLListenerOrPumpName::Empty: ") + what) {} + Empty(const std::string& what): LLException("LLListenerOrPumpName::Empty: " + what) {} }; private: @@ -247,6 +246,30 @@ public: */ LLEventPump& obtain(const std::string& name); + /// exception potentially thrown by make() + struct BadType: public LLException + { + BadType(const std::string& what): LLException("BadType: " + what) {} + }; + + /** + * Create an LLEventPump with suggested name (optionally of specified + * LLEventPump subclass type). As with obtain(), LLEventPumps owns the new + * instance. + * + * As with LLEventPump's constructor, make() could throw + * LLEventPump::DupPumpName unless you pass tweak=true. + * + * As with a hand-constructed LLEventPump subclass, if you pass + * tweak=true, the tweaked name can be obtained by LLEventPump::getName(). + * + * Pass empty type to get the default LLEventStream. + * + * If you pass an unrecognized type string, make() throws BadType. + */ + LLEventPump& make(const std::string& name, bool tweak=false, + const std::string& type=std::string()); + /** * Find the named LLEventPump instance. If it exists post the message to it. * If the pump does not exist, do nothing. @@ -303,10 +326,19 @@ testable: // destroyed. typedef std::set PumpSet; PumpSet mOurPumps; - // LLEventPump subclasses that should be instantiated for particular - // instance names - typedef std::map> PumpFactories; - static PumpFactories mFactories; + // for make(), map string type name to LLEventPump subclass factory function + typedef std::map> PumpFactories; + // Data used by make(). + // One might think mFactories and mTypes could reasonably be static. So + // they could -- if not for the fact that make() or obtain() might be + // called before this module's static variables have been initialized. + // This is why we use singletons in the first place. + PumpFactories mFactories; + + // for obtain(), map desired string instance name to string type when + // obtain() must create the instance + typedef std::map InstanceTypes; + InstanceTypes mTypes; }; /***************************************************************************** @@ -372,8 +404,7 @@ public: */ struct DupPumpName: public LLException { - DupPumpName(const std::string& what): - LLException(std::string("DupPumpName: ") + what) {} + DupPumpName(const std::string& what): LLException("DupPumpName: " + what) {} }; /** @@ -409,9 +440,7 @@ public: */ struct DupListenerName: public ListenError { - DupListenerName(const std::string& what): - ListenError(std::string("DupListenerName: ") + what) - {} + DupListenerName(const std::string& what): ListenError("DupListenerName: " + what) {} }; /** * exception thrown by listen(). The order dependencies specified for your @@ -423,7 +452,7 @@ public: */ struct Cycle: public ListenError { - Cycle(const std::string& what): ListenError(std::string("Cycle: ") + what) {} + Cycle(const std::string& what): ListenError("Cycle: " + what) {} }; /** * exception thrown by listen(). This one means that your new listener @@ -444,7 +473,7 @@ public: */ struct OrderChange: public ListenError { - OrderChange(const std::string& what): ListenError(std::string("OrderChange: ") + what) {} + OrderChange(const std::string& what): ListenError("OrderChange: " + what) {} }; /// used by listen() diff --git a/indra/llcommon/llleaplistener.cpp b/indra/llcommon/llleaplistener.cpp index 0d18e5fff9..3e6ce9092c 100644 --- a/indra/llcommon/llleaplistener.cpp +++ b/indra/llcommon/llleaplistener.cpp @@ -62,16 +62,11 @@ LLLeapListener::LLLeapListener(const ConnectFunc& connect): LLSD need_name(LLSDMap("name", LLSD())); add("newpump", "Instantiate a new LLEventPump named like [\"name\"] and listen to it.\n" - "If [\"type\"] == \"LLEventQueue\", make LLEventQueue, else LLEventStream.\n" + "[\"type\"] == \"LLEventStream\", \"LLEventMailDrop\" et al.\n" "Events sent through new LLEventPump will be decorated with [\"pump\"]=name.\n" "Returns actual name in [\"name\"] (may be different if collision).", &LLLeapListener::newpump, need_name); - add("killpump", - "Delete LLEventPump [\"name\"] created by \"newpump\".\n" - "Returns [\"status\"] boolean indicating whether such a pump existed.", - &LLLeapListener::killpump, - need_name); LLSD need_source_listener(LLSDMap("source", LLSD())("listener", LLSD())); add("listen", "Listen to an existing LLEventPump named [\"source\"], with listener name\n" @@ -121,58 +116,28 @@ LLLeapListener::~LLLeapListener() } } -namespace -{ - -static std::map> factories -{ - // tweak name for uniqueness - { "LLEventStream", [](const std::string& name){ return new LLEventStream(name, true); } }, - { "LLEventMailDrop", [](const std::string& name){ return new LLEventMailDrop(name, true); } } -}; - -} // anonymous namespace - void LLLeapListener::newpump(const LLSD& request) { Response reply(LLSD(), request); std::string name = request["name"]; - LLSD const & type = request["type"]; + std::string type = request["type"]; - LLEventPump * new_pump = NULL; - auto found = factories.find(type.asString()); - if (found != factories.end()) + try { - new_pump = (found->second)(name); + // tweak name for uniqueness + LLEventPump& new_pump(LLEventPumps::instance().make(name, true, type)); + name = new_pump.getName(); + reply["name"] = name; + + // Now listen on this new pump with our plugin listener + std::string myname("llleap"); + saveListener(name, myname, mConnect(new_pump, myname)); } - else + catch (const LLEventPumps::BadType& error) { - if (! type.isUndefined()) - { - reply.warn(STRINGIZE("unknown 'type' " << type << ", using LLEventStream")); - } - new_pump = new LLEventStream(name, true); // tweak name for uniqueness + reply.error(error.what()); } - - name = new_pump->getName(); - - mEventPumps.insert(name, new_pump); - - // Now listen on this new pump with our plugin listener - std::string myname("llleap"); - saveListener(name, myname, mConnect(*new_pump, myname)); - - reply["name"] = name; -} - -void LLLeapListener::killpump(const LLSD& request) -{ - Response reply(LLSD(), request); - - std::string name = request["name"]; - // success == (nonzero number of entries were erased) - reply["status"] = bool(mEventPumps.erase(name)); } void LLLeapListener::listen(const LLSD& request) diff --git a/indra/llcommon/llleaplistener.h b/indra/llcommon/llleaplistener.h index 2193d81b9e..0ca5893657 100644 --- a/indra/llcommon/llleaplistener.h +++ b/indra/llcommon/llleaplistener.h @@ -40,7 +40,6 @@ public: private: void newpump(const LLSD&); - void killpump(const LLSD&); void listen(const LLSD&); void stoplistening(const LLSD&); void ping(const LLSD&) const; @@ -64,10 +63,6 @@ private: // and listener name. typedef std::map, LLBoundListener> ListenersMap; ListenersMap mListeners; - // Similar lifespan reasoning applies to LLEventPumps instantiated by - // newpump() operations. - typedef boost::ptr_map EventPumpsMap; - EventPumpsMap mEventPumps; }; #endif /* ! defined(LL_LLLEAPLISTENER_H) */ -- cgit v1.3 From aea1e469820d276d0bc077fb8b3a77a5e6a35faf Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 22 Oct 2019 16:27:59 -0400 Subject: DRTVWR-476: Make ~LLEventPumps() call reset() on its way out. ~LLEventPumps() deletes every LLEventPump instance it created itself. However, many classes themselves contain LLEventPump subclass instances. These are registered with LLEventPumps without it managing their lifespan. But LLEventPump::reset() frees the LLStandardSignal aka boost::signals2::signal instance owned by the LLEventPump, perforce disconnecting all current listeners and disabling the LLEventPump. Even though the instance still exists, if someone subsequently calls post(), nothing will happen -- which is better than control trying to reach a method of a deleted object. --- indra/llcommon/llevents.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'indra/llcommon/llevents.cpp') diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp index 281a1121bd..64fb985951 100644 --- a/indra/llcommon/llevents.cpp +++ b/indra/llcommon/llevents.cpp @@ -266,6 +266,9 @@ LLEventPumps::~LLEventPumps() { delete *mOurPumps.begin(); } + // Reset every remaining registered LLEventPump subclass instance: those + // we DIDN'T instantiate using either make() or obtain(). + reset(); } /***************************************************************************** -- cgit v1.3