From b26e516d2b93a442d09f5c3b1b4d8d60139c42f5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 6 Apr 2022 17:34:28 -0400 Subject: DRTVWR-558: Change LLEventDispatcher error action (also LLEventAPI). Originally the LLEventAPI mechanism was primarily used for VITA testing. In that case it was okay for the viewer to crash with LL_ERRS if the test script passed a bad request. With puppetry, hopefully new LEAP scripts will be written to engage LLEventAPIs in all sorts of interesting ways. Change error handling from LL_ERRS to LL_WARNS. Furthermore, if the incoming request contains a "reply" key, send back an error response to the requester. Update lleventdispatcher_test.cpp accordingly. (cherry picked from commit de0539fcbe815ceec2041ecc9981e3adf59f2806) --- indra/llcommon/lleventdispatcher.cpp | 127 ++++++++++++++++++++++++----------- 1 file changed, 89 insertions(+), 38 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 5b6d4efbe9..742d6cf51f 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -40,15 +40,24 @@ // other Linden headers #include "llevents.h" #include "llerror.h" +#include "llexception.h" #include "llsdutil.h" #include "stringize.h" #include // std::auto_ptr +/***************************************************************************** +* DispatchError +*****************************************************************************/ +struct DispatchError: public LLException +{ + DispatchError(const std::string& what): LLException(what) {} +}; + /***************************************************************************** * LLSDArgsSource *****************************************************************************/ /** - * Store an LLSD array, producing its elements one at a time. Die with LL_ERRS + * Store an LLSD array, producing its elements one at a time. It is an error * if the consumer requests more elements than the array contains. */ class LL_COMMON_API LLSDArgsSource @@ -74,8 +83,7 @@ LLSDArgsSource::LLSDArgsSource(const std::string function, const LLSD& args): { if (! (_args.isUndefined() || _args.isArray())) { - LL_ERRS("LLSDArgsSource") << _function << " needs an args array instead of " - << _args << LL_ENDL; + LLTHROW(DispatchError(stringize(_function, " needs an args array instead of ", _args))); } } @@ -88,8 +96,8 @@ LLSD LLSDArgsSource::next() { if (_index >= _args.size()) { - LL_ERRS("LLSDArgsSource") << _function << " requires more arguments than the " - << _args.size() << " provided: " << _args << LL_ENDL; + LLTHROW(DispatchError(stringize(_function, " requires more arguments than the ", + _args.size(), " provided: ", _args))); } return _args[_index++]; } @@ -163,7 +171,8 @@ public: /// default values LLSDArgsMapper(const std::string& function, const LLSD& names, const LLSD& defaults); - /// Given arguments map, return LLSD::Array of parameter values, or LL_ERRS. + /// Given arguments map, return LLSD::Array of parameter values, or + /// trigger error. LLSD map(const LLSD& argsmap) const; private: @@ -195,7 +204,7 @@ LLSDArgsMapper::LLSDArgsMapper(const std::string& function, { if (! (_names.isUndefined() || _names.isArray())) { - LL_ERRS("LLSDArgsMapper") << function << " names must be an array, not " << names << LL_ENDL; + LLTHROW(DispatchError(stringize(function, " names must be an array, not ", names))); } LLSD::Integer nparams(_names.size()); // From _names generate _indexes. @@ -218,8 +227,8 @@ LLSDArgsMapper::LLSDArgsMapper(const std::string& function, // defaults is a (possibly empty) array. Right-align it with names. if (ndefaults > nparams) { - LL_ERRS("LLSDArgsMapper") << function << " names array " << names - << " shorter than defaults array " << defaults << LL_ENDL; + LLTHROW(DispatchError(stringize(function, " names array ", names, + " shorter than defaults array ", defaults))); } // Offset by which we slide defaults array right to right-align with @@ -256,14 +265,14 @@ LLSDArgsMapper::LLSDArgsMapper(const std::string& function, } if (bogus.size()) { - LL_ERRS("LLSDArgsMapper") << function << " defaults specified for nonexistent params " - << formatlist(bogus) << LL_ENDL; + LLTHROW(DispatchError(stringize(function, " defaults specified for nonexistent params ", + formatlist(bogus)))); } } else { - LL_ERRS("LLSDArgsMapper") << function << " defaults must be a map or an array, not " - << defaults << LL_ENDL; + LLTHROW(DispatchError(stringize(function, " defaults must be a map or an array, not ", + defaults))); } } @@ -271,8 +280,8 @@ LLSD LLSDArgsMapper::map(const LLSD& argsmap) const { if (! (argsmap.isUndefined() || argsmap.isMap() || argsmap.isArray())) { - LL_ERRS("LLSDArgsMapper") << _function << " map() needs a map or array, not " - << argsmap << LL_ENDL; + LLTHROW(DispatchError(stringize(_function, " map() needs a map or array, not ", + argsmap))); } // Initialize the args array. Indexing a non-const LLSD array grows it // to appropriate size, but we don't want to resize this one on each @@ -369,8 +378,8 @@ LLSD LLSDArgsMapper::map(const LLSD& argsmap) const // by argsmap, that's a problem. if (unfilled.size()) { - LL_ERRS("LLSDArgsMapper") << _function << " missing required arguments " - << formatlist(unfilled) << " from " << argsmap << LL_ENDL; + LLTHROW(DispatchError(stringize(_function, " missing required arguments ", + formatlist(unfilled), " from ", argsmap))); } // done @@ -420,7 +429,7 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE std::string mismatch(llsd_matches(mRequired, event)); if (! mismatch.empty()) { - LL_ERRS("LLEventDispatcher") << desc << ": bad request: " << mismatch << LL_ENDL; + LLTHROW(DispatchError(stringize(desc, ": bad request: ", mismatch))); } // Event syntax looks good, go for it! mFunc(event); @@ -596,48 +605,90 @@ bool LLEventDispatcher::remove(const std::string& name) return true; } -/// Call a registered callable with an explicitly-specified name. If no -/// such callable exists, die with LL_ERRS. +/// Call a registered callable with an explicitly-specified name. It is an +/// error if no such callable exists. void LLEventDispatcher::operator()(const std::string& name, const LLSD& event) const { - if (! try_call(name, event)) + std::string error{ try_call_log(std::string(), name, event) }; + if (! error.empty()) { - LL_ERRS("LLEventDispatcher") << "LLEventDispatcher(" << mDesc << "): '" << name - << "' not found" << LL_ENDL; + callFail(event, error); } } -/// Extract the @a key value from the incoming @a event, and call the -/// callable whose name is specified by that map @a key. If no such -/// callable exists, die with LL_ERRS. +/// Extract the @a key value from the incoming @a event, and call the callable +/// whose name is specified by that map @a key. It is an error if no such +/// callable exists. void LLEventDispatcher::operator()(const LLSD& event) const { - // This could/should be implemented in terms of the two-arg overload. - // However -- we can produce a more informative error message. - std::string name(event[mKey]); - if (! try_call(name, event)) + std::string error{ try_call_log(mKey, event[mKey], event) }; + if (! error.empty()) + { + callFail(event, error); + } +} + +void LLEventDispatcher::callFail(const LLSD& event, const std::string& msg) const +{ + static LLSD::String key{ "reply" }; + if (event.has(key)) { - LL_ERRS("LLEventDispatcher") << "LLEventDispatcher(" << mDesc << "): bad " << mKey - << " value '" << name << "'" << LL_ENDL; + // Oh good, the incoming event specifies a reply pump -- pass back a + // response that includes an "error" key with the message. + sendReply(llsd::map("error", msg), event, key); } } bool LLEventDispatcher::try_call(const LLSD& event) const { - return try_call(event[mKey], event); + return try_call_log(mKey, event[mKey], event).empty(); } bool LLEventDispatcher::try_call(const std::string& name, const LLSD& event) const +{ + return try_call_log(std::string(), name, event).empty(); +} + +std::string LLEventDispatcher::try_call_log(const std::string& key, const std::string& name, + const LLSD& event) const +{ + std::string error{ try_call(key, name, event) }; + if (! error.empty()) + { + LL_WARNS("LLEventDispatcher") << error << LL_ENDL; + } + return error; +} + +// This internal method returns empty string if the call succeeded, else +// non-empty error message. +std::string LLEventDispatcher::try_call(const std::string& key, const std::string& name, + const LLSD& event) const { DispatchMap::const_iterator found = mDispatch.find(name); if (found == mDispatch.end()) { - return false; + if (key.empty()) + { + return stringize("LLEventDispatcher(", mDesc, "): '", name, "' not found"); + } + else + { + return stringize("LLEventDispatcher(", mDesc, "): bad ", key, " value '", name, "'"); + } + } + + try + { + // Found the name, so it's plausible to even attempt the call. + found->second->call(stringize("LLEventDispatcher(", mDesc, ") calling '", name, "'"), + event); + } + catch (const DispatchError& err) + { + return err.what(); } - // Found the name, so it's plausible to even attempt the call. - found->second->call(STRINGIZE("LLEventDispatcher(" << mDesc << ") calling '" << name << "'"), - event); - return true; // tell caller we were able to call + return {}; // tell caller we were able to call } LLSD LLEventDispatcher::getMetadata(const std::string& name) const -- cgit v1.2.3 From af4fbc1f8a99a3c5370cb6db45435e67f9ce92d2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 18 Jun 2022 11:57:10 -0400 Subject: DRTVWR-564: WIP: Add LazyEventAPI and tests. Tests don't yet pass. LazyEventAPI is a registrar that implicitly instantiates some particular LLEventAPI subclass on demand: that is, when LLEventPumps::obtain() tries to find an LLEventPump by the registered name. This leverages the new LLEventPumps::registerPumpFactory() machinery. Fix registerPumpFactory() to adapt the passed PumpFactory to accept TypeFactory parameters (two of which it ignores). Supplement it with unregisterPumpFactory() to support LazyEventAPI instances with lifespans shorter than the process -- which may be mostly test programs, but still a hole worth closing. Similarly, add unregisterTypeFactory(). A LazyEventAPI subclass takes over responsibility for specifying the LLEventAPI's name, desc, field, plus whatever add() calls will be needed to register the LLEventAPI's operations. This is so we can (later) enhance LLLeapListener to consult LazyEventAPI instances for not-yet-instantiated LLEventAPI metadata, as well as enumerating existing LLEventAPI instances. The trickiest part of this is capturing calls to the various LLEventDispatcher::add() overloads in such a way that, when the LLEventAPI subclass is eventually instantiated, we can replay them in the new instance. LLEventAPI acquires a new protected constructor specifically for use by a subclass registered by a companion LazyEventAPI. It accepts a const reference to LazyEventAPIParams, intended to be opaque to the LLEventAPI subclass; the subclass must declare a constructor that accepts and forwards the parameter block to the new LLEventAPI constructor. The implementation delegates to the existing LLEventAPI constructor, plus it runs deferred add() calls. LLDispatchListener now derives from LLEventStream instead of containing it as a data member. The reason is that if LLEventPumps::obtain() implicitly instantiates it, LLEventPumps's destructor will try to destroy it by deleting the LLEventPump*. If the LLEventPump returned by the factory function is a data member of an outer class, that won't work so well. But if LLDispatchListener (and by implication, LLEventAPI and any subclass) is derived from LLEventPump, then the virtual destructor will Do The Right Thing. Change LLDispatchListener to *not* allow tweaking the LLEventPump name. Since the overwhelming use case for LLDispatchListener is LLEventAPI, accepting but silently renaming an LLEventAPI subclass would ensure nobody could reach it. Change LLEventDispatcher's use of std::enable_if to control the set of add() overloads available for the intended use cases. Apparently this formulation is just as functional at the method declaration point, while avoiding the need to restate the whole enable_if expression at the method definition point. Add lazyeventapi_test.cpp to exercise. --- indra/llcommon/lleventdispatcher.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 742d6cf51f..bc53ec3da0 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -706,8 +706,17 @@ LLSD LLEventDispatcher::getMetadata(const std::string& name) const LLDispatchListener::LLDispatchListener(const std::string& pumpname, const std::string& key): LLEventDispatcher(pumpname, key), - mPump(pumpname, true), // allow tweaking for uniqueness - mBoundListener(mPump.listen("self", boost::bind(&LLDispatchListener::process, this, _1))) + // Do NOT tweak the passed pumpname. In practice, when someone + // instantiates a subclass of our LLEventAPI subclass, they intend to + // claim that LLEventPump name in the global LLEventPumps namespace. It + // would be mysterious and distressing if we allowed name tweaking, and + // someone else claimed pumpname first for a completely unrelated + // LLEventPump. Posted events would never reach our subclass listener + // because we would have silently changed its name; meanwhile listeners + // (if any) on that other LLEventPump would be confused by the events + // intended for our subclass. + LLEventStream(pumpname, false), + mBoundListener(listen("self", [this](const LLSD& event){ return process(event); })) { } -- cgit v1.2.3 From f9d810ac2a02ef96c843e214c7479146dd4f4157 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 21 Jun 2022 17:24:03 -0400 Subject: DRTVWR-564: Per NickyD, need not test static_cast result for nullptr. --- indra/llcommon/lleventdispatcher.cpp | 7 ------- 1 file changed, 7 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index bc53ec3da0..7ba8c5ada7 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -586,13 +586,6 @@ void LLEventDispatcher::add(const std::string& name, const std::string& desc, new LLSDDispatchEntry(desc, callable, required)))); } -void LLEventDispatcher::addFail(const std::string& name, const std::string& classname) const -{ - LL_ERRS("LLEventDispatcher") << "LLEventDispatcher(" << mDesc << ")::add(" << name - << "): " << classname << " is not a subclass " - << "of LLEventDispatcher" << LL_ENDL; -} - /// Unregister a callable bool LLEventDispatcher::remove(const std::string& name) { -- cgit v1.2.3 From cc3d21cceac7d6c1b2fc9297330fa819855f6e5b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 20 Dec 2022 09:53:58 -0500 Subject: DRTVWR-558: Tweak LLEventDispatcher. Instead of std::map, use std::unique_ptr as the mapped_type, using emplace() to store new entries. This more correctly captures the desired semantics: we have no intention of passing around the pointers in the map, we just want the map to delete them on destruction. Use std::function instead of boost::function. (cherry picked from commit 7ba53ef82db5683756e296225f0c8b838420a26e) --- indra/llcommon/lleventdispatcher.cpp | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 0c3bb35cfe..3e45601429 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -561,9 +561,7 @@ void LLEventDispatcher::addArrayParamsDispatchEntry(const std::string& name, const invoker_function& invoker, LLSD::Integer arity) { - mDispatch.insert( - DispatchMap::value_type(name, DispatchMap::mapped_type( - new ArrayParamsDispatchEntry(desc, invoker, arity)))); + mDispatch.emplace(name, new ArrayParamsDispatchEntry(desc, invoker, arity)); } void LLEventDispatcher::addMapParamsDispatchEntry(const std::string& name, @@ -572,18 +570,14 @@ void LLEventDispatcher::addMapParamsDispatchEntry(const std::string& name, const LLSD& params, const LLSD& defaults) { - mDispatch.insert( - DispatchMap::value_type(name, DispatchMap::mapped_type( - new MapParamsDispatchEntry(name, desc, invoker, params, defaults)))); + mDispatch.emplace(name, new MapParamsDispatchEntry(name, desc, invoker, params, defaults)); } /// Register a callable by name void LLEventDispatcher::add(const std::string& name, const std::string& desc, const Callable& callable, const LLSD& required) { - mDispatch.insert( - DispatchMap::value_type(name, DispatchMap::mapped_type( - new LLSDDispatchEntry(desc, callable, required)))); + mDispatch.emplace(name, new LLSDDispatchEntry(desc, callable, required)); } /// Unregister a callable -- cgit v1.2.3 From 07c5645f5f9130a7fc338df0bc2bb791d43bd702 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 22 Dec 2022 14:53:29 -0500 Subject: DRTVWR-558: LLEventDispatcher uses LL::apply(), not boost::fusion. While calling a C++ function with arguments taken from a runtime-variable data structure necessarily involves a bit of hocus-pocus, the best you can say for the boost::fusion based implementation is that it worked. Sadly, template recursion limited its applicability to a handful of function arguments. Now that we have LL::apply(), use that instead. This implementation is much more straightforward. In particular, the LLSDArgsSource class, whose job was to dole out elements of an LLSD array one at a time for the template recursion, goes away entirely. Make virtual LLEventDispatcher::DispatchEntry::call() return LLSD instead of void. All LLEventDispatcher target functions so far have been void; any function that wants to respond to its invoker must do so explicitly by calling sendReply() or constructing an LLEventAPI::Response instance. Supporting non- void functions permits LLEventDispatcher to respond implicitly with the returned value. Of course this requires a wrapper for void target functions that returns LLSD::isUndefined(). Break out LLEventDispatcher::reply() from callFail(), so we can reply with success as well as failure. Make LLEventDispatcher::try_call_log() prepend the actual leaf class name and description to any error returned by three-arg try_call(). That try_call() overload reported "LLEventDispatcher(desc): " for a couple specific errors, but no others. Hoist to try_call_log() to apply uniformly. Introduce new try_call_one() method to diagnose name-not-found errors and catch internal DispatchError and LL::apply_error exceptions. try_call_one() returns a std::pair, containing either an error message or an LLSD value. Make try_call_log() and three-arg try_call() accept LLSD 'name' instead of plain std::string, allowing for the possibility of an array or map. That lets us extend three-arg try_call() to break out new cases for the function selector LLSD: isUndefined(), isArray(), isMap() and (current case) scalar String. If try_call_one() reports an error, log it and try to send reply, as now. If it returns LLSD::isUndefined(), e.g. from a void target function wrapper, do nothing. But if it returns an LLSD map, try to send that back to the invoker. And if it returns an LLSD scalar or array, wrap it in a map with key "data" to respond to the invoker. Allowing a target function to return its result rather than explicitly sending it opens the possibility of batched requests (aggregate 'name') returning batched responses. Almost every place that constructs LLEventDispatcher's internal DispatchError exception called stringize() to format the what() string. Simplify calls by making DispatchError accept variadic arguments and forward to stringize(). Add LL::invoke() to apply.h. Like LL::apply(), this is a (limited) C++14 foreshadowing of std::invoke(), with preprocessor conditionals to switch to std::invoke() when that's available. Introduce LL::invoke() to handle a callable that's actually a pointer to method. Now our C++14 apply() implementation can accept pointer to method, using invoke() to generalize the actual function call. Also anticipate std::bind_front() with LL::bind_front(). For apply(func, std::array) and our extensions apply(func, std::vector) and apply(func, LLSD), we can't pass a pointer to method as the func unless the second argument happens to be an array or vector of pointers (or references) to instances of exactly the right class -- and of course LLSD can't store such at all. It's tempting to pass std::bind(std::mem_fn(ptr_to_method), instance), but that won't work: std::bind() requires a value or placeholder for each argument to pass to the bound function. The bind() expression above would only work for a nullary method. std::bind_front() would work, but that doesn't arrive until C++20. Again, once we get there we'll defer to the std:: implementation. Instead of the generic __cplusplus, check the appropriate feature-test macro for availability of each of std::invoke(), std::apply() and std::bind_front(). Change apply() error handling from assert() to new LL::apply_error exception. LLEventDispatcher must be able to intercept apply() errors. Move validation and synthesis of the relevant error message to new apply.cpp source file. Add to llptrto.h new LL::get_ref() and LL::get_ptr() template functions to unify the cases of a calling template accepting either a pointer or a reference. Wrapping the parameter in either get_ref() or get_ptr() allows dereferencing the parameter as desired. Move LL::apply(function, LLSD) argument validation/manipulation to a non- template function in llsdutil.cpp: no need to replicate that logic in the template for every CALLABLE specialization. The trouble with passing bind_front(std::mem_fn(ptr_to_method), instance) to apply() is that since bind_front() accepts and forwards variadic additional arguments, apply() can't infer the arity of the bound ptr_to_method. Address that by introducing apply_n(function, LLSD), permitting a caller to infer the arity of ptr_to_method and explicitly pass it to apply_n(). Polish up lleventdispatcher_test.cpp accordingly. Wrong LLSD type and wrong number of arguments now produce different (somewhat more informative) error messages. Moreover, passing too many entries in an LLSD array used to work: the extra arguments used to be ignored. Now we require that the size of the array match the arity of the target function. Change the too-many-arguments tests from success testing to error testing. Replace 'foreach' aka BOOST_FOREACH macro invocations with range 'for'. Replace STRINGIZE(item0 << item1 << ...) with stringize(item0, item1, ...). (cherry picked from commit 9c049563b5480bb7e8ed87d9313822595b479c3b) --- indra/llcommon/lleventdispatcher.cpp | 224 ++++++++++++++++++++--------------- 1 file changed, 130 insertions(+), 94 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 3e45601429..e7e73125a7 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -50,68 +50,13 @@ *****************************************************************************/ struct DispatchError: public LLException { - DispatchError(const std::string& what): LLException(what) {} + // template constructor involving strings passes all arguments to + // stringize() to construct LLException's what() string + template + DispatchError(const std::string& arg0, ARGS&&... args): + LLException(stringize(arg0, std::forward(args)...)) {} }; -/***************************************************************************** -* LLSDArgsSource -*****************************************************************************/ -/** - * Store an LLSD array, producing its elements one at a time. It is an error - * if the consumer requests more elements than the array contains. - */ -class LL_COMMON_API LLSDArgsSource -{ -public: - LLSDArgsSource(const std::string function, const LLSD& args); - ~LLSDArgsSource(); - - LLSD next(); - - void done() const; - -private: - std::string _function; - LLSD _args; - LLSD::Integer _index; -}; - -LLSDArgsSource::LLSDArgsSource(const std::string function, const LLSD& args): - _function(function), - _args(args), - _index(0) -{ - if (! (_args.isUndefined() || _args.isArray())) - { - LLTHROW(DispatchError(stringize(_function, " needs an args array instead of ", _args))); - } -} - -LLSDArgsSource::~LLSDArgsSource() -{ - done(); -} - -LLSD LLSDArgsSource::next() -{ - if (_index >= _args.size()) - { - LLTHROW(DispatchError(stringize(_function, " requires more arguments than the ", - _args.size(), " provided: ", _args))); - } - return _args[_index++]; -} - -void LLSDArgsSource::done() const -{ - if (_index < _args.size()) - { - LL_WARNS("LLSDArgsSource") << _function << " only consumed " << _index - << " of the " << _args.size() << " arguments provided: " - << _args << LL_ENDL; - } -} - /***************************************************************************** * LLSDArgsMapper *****************************************************************************/ @@ -204,7 +149,7 @@ LLSDArgsMapper::LLSDArgsMapper(const std::string& function, { if (! (_names.isUndefined() || _names.isArray())) { - LLTHROW(DispatchError(stringize(function, " names must be an array, not ", names))); + LLTHROW(DispatchError(function, " names must be an array, not ", names)); } auto nparams(_names.size()); // From _names generate _indexes. @@ -227,8 +172,8 @@ LLSDArgsMapper::LLSDArgsMapper(const std::string& function, // defaults is a (possibly empty) array. Right-align it with names. if (ndefaults > nparams) { - LLTHROW(DispatchError(stringize(function, " names array ", names, - " shorter than defaults array ", defaults))); + LLTHROW(DispatchError(function, " names array ", names, + " shorter than defaults array ", defaults)); } // Offset by which we slide defaults array right to right-align with @@ -265,14 +210,14 @@ LLSDArgsMapper::LLSDArgsMapper(const std::string& function, } if (bogus.size()) { - LLTHROW(DispatchError(stringize(function, " defaults specified for nonexistent params ", - formatlist(bogus)))); + LLTHROW(DispatchError(function, " defaults specified for nonexistent params ", + formatlist(bogus))); } } else { - LLTHROW(DispatchError(stringize(function, " defaults must be a map or an array, not ", - defaults))); + LLTHROW(DispatchError(function, " defaults must be a map or an array, not ", + defaults)); } } @@ -280,8 +225,8 @@ LLSD LLSDArgsMapper::map(const LLSD& argsmap) const { if (! (argsmap.isUndefined() || argsmap.isMap() || argsmap.isArray())) { - LLTHROW(DispatchError(stringize(_function, " map() needs a map or array, not ", - argsmap))); + LLTHROW(DispatchError(_function, " map() needs a map or array, not ", + argsmap)); } // Initialize the args array. Indexing a non-const LLSD array grows it // to appropriate size, but we don't want to resize this one on each @@ -378,8 +323,8 @@ LLSD LLSDArgsMapper::map(const LLSD& argsmap) const // by argsmap, that's a problem. if (unfilled.size()) { - LLTHROW(DispatchError(stringize(_function, " missing required arguments ", - formatlist(unfilled), " from ", argsmap))); + LLTHROW(DispatchError(_function, " missing required arguments ", + formatlist(unfilled), " from ", argsmap)); } // done @@ -399,6 +344,9 @@ std::string LLSDArgsMapper::formatlist(const LLSD& list) return out.str(); } +/***************************************************************************** +* LLEventDispatcher +*****************************************************************************/ LLEventDispatcher::LLEventDispatcher(const std::string& desc, const std::string& key): mDesc(desc), mKey(key) @@ -409,6 +357,10 @@ LLEventDispatcher::~LLEventDispatcher() { } +LLEventDispatcher::DispatchEntry::DispatchEntry(const std::string& desc): + mDesc(desc) +{} + /** * DispatchEntry subclass used for callables accepting(const LLSD&) */ @@ -423,16 +375,17 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE Callable mFunc; LLSD mRequired; - virtual void call(const std::string& desc, const LLSD& event) const + virtual LLSD call(const std::string& desc, const LLSD& event) const { // Validate the syntax of the event itself. std::string mismatch(llsd_matches(mRequired, event)); if (! mismatch.empty()) { - LLTHROW(DispatchError(stringize(desc, ": bad request: ", mismatch))); + LLTHROW(DispatchError(desc, ": bad request: ", mismatch)); } // Event syntax looks good, go for it! mFunc(event); + return {}; } virtual LLSD addMetadata(LLSD meta) const @@ -455,10 +408,9 @@ struct LLEventDispatcher::ParamsDispatchEntry: public LLEventDispatcher::Dispatc invoker_function mInvoker; - virtual void call(const std::string& desc, const LLSD& event) const + virtual LLSD call(const std::string&, const LLSD& event) const { - LLSDArgsSource src(desc, event); - mInvoker(boost::bind(&LLSDArgsSource::next, boost::ref(src))); + return mInvoker(event); } }; @@ -541,11 +493,11 @@ struct LLEventDispatcher::MapParamsDispatchEntry: public LLEventDispatcher::Para LLSD mRequired; LLSD mOptional; - virtual void call(const std::string& desc, const LLSD& event) const + virtual LLSD call(const std::string& desc, const LLSD& event) const { // Just convert from LLSD::Map to LLSD::Array using mMapper, then pass // to base-class call() method. - ParamsDispatchEntry::call(desc, mMapper.map(event)); + return ParamsDispatchEntry::call(desc, mMapper.map(event)); } virtual LLSD addMetadata(LLSD meta) const @@ -616,13 +568,19 @@ void LLEventDispatcher::operator()(const LLSD& event) const } void LLEventDispatcher::callFail(const LLSD& event, const std::string& msg) const +{ + // pass back a response that includes an "error" key with the message. + reply(llsd::map("error", msg), event); +} + +void LLEventDispatcher::reply(const LLSD& response, const LLSD& event) const { static LLSD::String key{ "reply" }; if (event.has(key)) { - // Oh good, the incoming event specifies a reply pump -- pass back a - // response that includes an "error" key with the message. - sendReply(llsd::map("error", msg), event, key); + // Oh good, the incoming event specifies a reply pump -- pass back + // our response. + sendReply(response, event, key); } } @@ -631,17 +589,30 @@ bool LLEventDispatcher::try_call(const LLSD& event) const return try_call_log(mKey, event[mKey], event).empty(); } +/*==========================================================================*| + TODO: + +* When mInvoker returns result.isDefined(), sendReply(llsd::map("data", result)) +* When try_call finds name.isArray(), construct response array from + dispatching each call, sendReply() as above +* When try_call finds name.isMap(), construct response map from dispatching + each call, sendReply() as above -- note, caller can't care about order +* Possible future transactional behavior: look up all names before calling any + +|*==========================================================================*/ bool LLEventDispatcher::try_call(const std::string& name, const LLSD& event) const { return try_call_log(std::string(), name, event).empty(); } -std::string LLEventDispatcher::try_call_log(const std::string& key, const std::string& name, +std::string LLEventDispatcher::try_call_log(const std::string& key, const LLSD& name, const LLSD& event) const { std::string error{ try_call(key, name, event) }; if (! error.empty()) { + // If we're a subclass of LLEventDispatcher, e.g. LLEventAPI, report that. + error = stringize(LLError::Log::classname(this), "(", mDesc, "): ", error); LL_WARNS("LLEventDispatcher") << error << LL_ENDL; } return error; @@ -649,33 +620,100 @@ std::string LLEventDispatcher::try_call_log(const std::string& key, const std::s // This internal method returns empty string if the call succeeded, else // non-empty error message. -std::string LLEventDispatcher::try_call(const std::string& key, const std::string& name, +std::string LLEventDispatcher::try_call(const std::string& key, const LLSD& name, const LLSD& event) const +{ + if (name.isUndefined()) + { + if (key.empty()) + { + return "attempting to call with no name"; + } + else + { + return stringize("no ", key); + } + } + else if (name.isArray()) + { + return stringize(key, " array dispatch ", name, " not yet implemented"); + } + else if (name.isMap()) + { + return stringize(key, " map dispatch ", name, " not yet implemented"); + } + else if (! name.isString()) + { + return stringize(key, " bad type ", LLSD::typeString(name.type()), ' ', name, + " -- function names are String"); + } + else // name is an LLSD::String + { + auto success{ try_call_one(key, name, event) }; + // pretend to unpack + std::string& error{ success.first }; + LLSD& result{ success.second }; + // did try_call_one() report an error? + if (! error.empty()) + { + // if we have a reply key, respond to invoker + reply(llsd::map("error", error), event); + // now tell caller + return error; + } + // try_call_one() succeeded in calling the target function -- + // should we reply to invoker? + if (result.isUndefined()) + { + // We would get result.isUndefined() if the target function has + // void return. In any case, even if the target function returns + // LLSD, isUndefined() means "don't bother sending response." + return {}; + } + // result.isDefined(): the target function returned something. + // Respond to invoker if we have a "reply" key. + if (! result.isMap()) + { + // wrap result in a map to play well with sendReply() + result = llsd::map("data", result); + } + reply(result, event); + return {}; + } +} + +std::pair +LLEventDispatcher::try_call_one(const std::string& key, const std::string& name, + const LLSD& event) const { DispatchMap::const_iterator found = mDispatch.find(name); if (found == mDispatch.end()) { if (key.empty()) { - return stringize("LLEventDispatcher(", mDesc, "): '", name, "' not found"); + return { stringize("'", name, "' not found"), {} }; } else { - return stringize("LLEventDispatcher(", mDesc, "): bad ", key, " value '", name, "'"); + return { stringize("bad ", key, " value '", name, "'"), {} }; } } try { // Found the name, so it's plausible to even attempt the call. - found->second->call(stringize("LLEventDispatcher(", mDesc, ") calling '", name, "'"), - event); + return { {}, found->second->call(stringize("calling '", name, "'"), event) }; } catch (const DispatchError& err) { - return err.what(); + // trouble preparing arguments + return { err.what(), {} }; + } + catch (const LL::apply_error& err) + { + // could also hit runtime errors with LL::apply() + return { err.what(), {} }; } - return {}; // tell caller we were able to call } LLSD LLEventDispatcher::getMetadata(const std::string& name) const @@ -691,6 +729,9 @@ LLSD LLEventDispatcher::getMetadata(const std::string& name) const return found->second->addMetadata(meta); } +/***************************************************************************** +* LLDispatchListener +*****************************************************************************/ LLDispatchListener::LLDispatchListener(const std::string& pumpname, const std::string& key): LLEventDispatcher(pumpname, key), // Do NOT tweak the passed pumpname. In practice, when someone @@ -712,8 +753,3 @@ bool LLDispatchListener::process(const LLSD& event) (*this)(event); return false; } - -LLEventDispatcher::DispatchEntry::DispatchEntry(const std::string& desc): - mDesc(desc) -{} - -- cgit v1.2.3 From b2205bde52acf82575757f74a642c40b7433bf6b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 6 Jan 2023 19:58:12 -0500 Subject: DRTVWR-558: Clean up LLEventDispatcher argument and result handling. Add a new LLEventDispatcher constructor accepting not only the map key to extract a requested function name, but a second map key to extract the arguments -- when required. In Doxygen comments, clarify the difference between the two constructors. Move interaction with the LLEventPump subsystem to LLDispatchListener. LLEventDispatcher is intended to be directly called. On error, instead of looking for a "reply" key in the invocation LLSD, throw DispatchError. Publish DispatchError, formerly an implementation detail, and its new subclass DispatchMissing. Make both LLEventDispatcher::operator()() overloads return LLSD, leveraging the new internal ReturnLLSD logic that returns a degenerate LLSD blob for a void target callable and, for compatible types, converts the returned value to LLSD. Notably, the public try_call() overloads still return bool; any value returned by the target callable is discarded. Clarify the operator() and try_call() argument requirements for target callables registered to accept an LLSD array, in Doxygen comments and in code. In particular, the 'event' passed to (event) overloads (vs. the (name, event) overloads) must be an LLSD map, so it must contain an "args" key (or the new arguments map key specified to the constructor) containing the LLSD args array. Since the use of the new args key depends on whether the target callable is registered to accept an array or a map, pass it into DispatchEntry::call() (and all subclass overrides), along with a bool to disambiguate whether we reached that method from an LLEventDispatcher (event) invocation method or a (name, event) invocation method. Allow streaming an LLEventDispatcher instance to std::ostream, primarily to facilitate construction of proper error messages. Revert the 'name' argument of internal try_call(key, name, event) to std::string. Ditch try_call_log(), try_call_one() and reply(). Fold try_call_one() logic into three-argument try_call(). Refactor callFail() as a template method accepting both the exception to throw and arbitrary stringize() arguments from which to construct the exception message. Non-static callFail() implicitly prepends the instance and a colon to the rest of the arguments, and calls static sCallFail(). The latter constructs the exception message, logs it and throws the specified exception. This obviates try_call_log(). Make implementation detail helper class LLSDArgsMapper a private member of LLEventDispatcher so it can access sCallFail(): we now want all error handling to go through that method. Add LLSDArgsMapper::callFail() resembling LLSDEventDispatcher::callFail(), but without having to specify the exception: only LLEventDispatcher will throw anything but generic DispatchError. Give LLEventDispatcher::ParamsDispatchEntry and its subclasses ArrayParamsDispatchEntry and MapParamsDispatchEntry a new 'name' argument to identify error messages. Store it and use it implicitly in new callFail() method, very like LLSDArgsMapper::callFail(). Make LLEventDispatcher:: addArrayParamsDispatchEntry() and addMapParamsDispatchEntry() pass a 'name' that includes the LLEventDispatcher instance name as well as the name of the specific registered callable. This way we need not intercept a low-level error and annotate it with contextual data: we can just let the exception propagate. Make ParamsDispatchEntry::call() override catch LL::apply_error thrown by an invoker_function, and pass its message to callFail(), i.e. rethrow as LLEventDispatcher::DispatchError. Introduce ArrayParamsDispatchEntry::call() override for the special logic to extract an arguments array from a passed LLSD map -- but only under the circumstances described in the Doxygen comment. Add similar logic to MapParamsDispatchEntry::call(), but with both argskey itself and a value for argskey optional in the passed LLSD map. Because LLEventDispatcher now has two constructor overloads, allow subclass constructor LLDispatchListener() to accept zero or more trailing arguments. This is different than giving LLDispatchListener's constructor a default final argument, in that the subclass doesn't need to specify its default value: that's up to the base-class constructor. But it does require that the subclass constructor move to the header file. Move private LLEventDispatcher::reply() method to LLDispatchListener. Extend LLDispatchListener::process() to handle DispatchError by attempting to reply with a map containing an "error" key, per convention. (In other words, move that logic from LLEventDispatcher to LLDispatchListener.) Also, for a map LLSD result, attempt to reply with that result; for other defined LLSD types, attempt to reply with a map containing a "data" key. This is backwards compatible with previous behavior because all previous LLDispatchListener subclass methods returned void, which now produces an undefined LLSD blob, which we don't bother trying to send in reply. In lleventdispatcher_test.cpp, rework tut::lleventdispatcher_data::call_exc() yet again to catch DispatchError instead of listening for an LLEventPump reply event. Similarly, make call_logerr() catch DispatchError. Since the exception should also be logged, we ignore it and focus on the log, as before. Add tests <23> to <27>, exercising calls to new class DispatchResult methods returning string, int, LLSD map, LLSD array and void. (cherry picked from commit 2f9c915dd3d5137b5b2b1a57f0179e1f7a090f8c) --- indra/llcommon/lleventdispatcher.cpp | 428 +++++++++++++++++++---------------- 1 file changed, 234 insertions(+), 194 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index e7e73125a7..7e5723c503 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -43,20 +43,9 @@ #include "llexception.h" #include "llsdutil.h" #include "stringize.h" +#include // std::quoted() #include // std::auto_ptr -/***************************************************************************** -* DispatchError -*****************************************************************************/ -struct DispatchError: public LLException -{ - // template constructor involving strings passes all arguments to - // stringize() to construct LLException's what() string - template - DispatchError(const std::string& arg0, ARGS&&... args): - LLException(stringize(arg0, std::forward(args)...)) {} -}; - /***************************************************************************** * LLSDArgsMapper *****************************************************************************/ @@ -109,7 +98,7 @@ struct DispatchError: public LLException * - Holes are filled with the default values. * - Any remaining holes constitute an error. */ -class LL_COMMON_API LLSDArgsMapper +class LL_COMMON_API LLEventDispatcher::LLSDArgsMapper { public: /// Accept description of function: function name, param names, param @@ -122,6 +111,8 @@ public: private: static std::string formatlist(const LLSD&); + template + void callFail(ARGS&&... args) const; // The function-name string is purely descriptive. We want error messages // to be able to indicate which function's LLSDArgsMapper has the problem. @@ -141,15 +132,16 @@ private: FilledVector _has_dft; }; -LLSDArgsMapper::LLSDArgsMapper(const std::string& function, - const LLSD& names, const LLSD& defaults): +LLEventDispatcher::LLSDArgsMapper::LLSDArgsMapper(const std::string& function, + const LLSD& names, + const LLSD& defaults): _function(function), _names(names), _has_dft(names.size()) { if (! (_names.isUndefined() || _names.isArray())) { - LLTHROW(DispatchError(function, " names must be an array, not ", names)); + callFail(" names must be an array, not ", names); } auto nparams(_names.size()); // From _names generate _indexes. @@ -172,8 +164,7 @@ LLSDArgsMapper::LLSDArgsMapper(const std::string& function, // defaults is a (possibly empty) array. Right-align it with names. if (ndefaults > nparams) { - LLTHROW(DispatchError(function, " names array ", names, - " shorter than defaults array ", defaults)); + callFail(" names array ", names, " shorter than defaults array ", defaults); } // Offset by which we slide defaults array right to right-align with @@ -210,23 +201,20 @@ LLSDArgsMapper::LLSDArgsMapper(const std::string& function, } if (bogus.size()) { - LLTHROW(DispatchError(function, " defaults specified for nonexistent params ", - formatlist(bogus))); + callFail(" defaults specified for nonexistent params ", formatlist(bogus)); } } else { - LLTHROW(DispatchError(function, " defaults must be a map or an array, not ", - defaults)); + callFail(" defaults must be a map or an array, not ", defaults); } } -LLSD LLSDArgsMapper::map(const LLSD& argsmap) const +LLSD LLEventDispatcher::LLSDArgsMapper::map(const LLSD& argsmap) const { if (! (argsmap.isUndefined() || argsmap.isMap() || argsmap.isArray())) { - LLTHROW(DispatchError(_function, " map() needs a map or array, not ", - argsmap)); + callFail(" map() needs a map or array, not ", argsmap); } // Initialize the args array. Indexing a non-const LLSD array grows it // to appropriate size, but we don't want to resize this one on each @@ -323,15 +311,14 @@ LLSD LLSDArgsMapper::map(const LLSD& argsmap) const // by argsmap, that's a problem. if (unfilled.size()) { - LLTHROW(DispatchError(_function, " missing required arguments ", - formatlist(unfilled), " from ", argsmap)); + callFail(" missing required arguments ", formatlist(unfilled), " from ", argsmap); } // done return args; } -std::string LLSDArgsMapper::formatlist(const LLSD& list) +std::string LLEventDispatcher::LLSDArgsMapper::formatlist(const LLSD& list) { std::ostringstream out; const char* delim = ""; @@ -344,14 +331,26 @@ std::string LLSDArgsMapper::formatlist(const LLSD& list) return out.str(); } +template +void LLEventDispatcher::LLSDArgsMapper::callFail(ARGS&&... args) const +{ + LLEventDispatcher::sCallFail + (_function, std::forward(args)...); +} + /***************************************************************************** * LLEventDispatcher *****************************************************************************/ LLEventDispatcher::LLEventDispatcher(const std::string& desc, const std::string& key): + LLEventDispatcher(desc, key, "args") +{} + +LLEventDispatcher::LLEventDispatcher(const std::string& desc, const std::string& key, + const std::string& argskey): mDesc(desc), - mKey(key) -{ -} + mKey(key), + mArgskey(argskey) +{} LLEventDispatcher::~LLEventDispatcher() { @@ -375,20 +374,21 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE Callable mFunc; LLSD mRequired; - virtual LLSD call(const std::string& desc, const LLSD& event) const + LLSD call(const std::string& desc, const LLSD& event, bool, const std::string&) const override { // Validate the syntax of the event itself. std::string mismatch(llsd_matches(mRequired, event)); if (! mismatch.empty()) { - LLTHROW(DispatchError(desc, ": bad request: ", mismatch)); + LLEventDispatcher::sCallFail + (desc, ": bad request: ", mismatch); } // Event syntax looks good, go for it! mFunc(event); return {}; } - virtual LLSD addMetadata(LLSD meta) const + LLSD addMetadata(LLSD meta) const override { meta["required"] = mRequired; return meta; @@ -401,16 +401,35 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE */ struct LLEventDispatcher::ParamsDispatchEntry: public LLEventDispatcher::DispatchEntry { - ParamsDispatchEntry(const std::string& desc, const invoker_function& func): + ParamsDispatchEntry(const std::string& name, const std::string& desc, + const invoker_function& func): DispatchEntry(desc), + mName(name), mInvoker(func) {} + std::string mName; invoker_function mInvoker; - virtual LLSD call(const std::string&, const LLSD& event) const + LLSD call(const std::string&, const LLSD& event, bool, const std::string&) const override + { + try + { + return mInvoker(event); + } + catch (const LL::apply_error& err) + { + // could hit runtime errors with LL::apply() + return callFail(err.what()); + } + } + + template + LLSD callFail(ARGS&&... args) const { - return mInvoker(event); + LLEventDispatcher::sCallFail(mName, ": ", std::forward(args)...); + // pacify the compiler + return {}; } }; @@ -420,15 +439,48 @@ struct LLEventDispatcher::ParamsDispatchEntry: public LLEventDispatcher::Dispatc */ struct LLEventDispatcher::ArrayParamsDispatchEntry: public LLEventDispatcher::ParamsDispatchEntry { - ArrayParamsDispatchEntry(const std::string& desc, const invoker_function& func, - LLSD::Integer arity): - ParamsDispatchEntry(desc, func), + ArrayParamsDispatchEntry(const std::string& name, const std::string& desc, + const invoker_function& func, LLSD::Integer arity): + ParamsDispatchEntry(name, desc, func), mArity(arity) {} LLSD::Integer mArity; - virtual LLSD addMetadata(LLSD meta) const + LLSD call(const std::string& desc, const LLSD& event, bool fromMap, const std::string& argskey) const override + { +// std::string context { stringize(desc, "(", event, ") with argskey ", std::quoted(argskey), ": ") }; + // Whether we try to extract arguments from 'event' depends on whether + // the LLEventDispatcher consumer called one of the (name, event) + // methods (! fromMap) or one of the (event) methods (fromMap). If we + // were called with (name, event), the passed event must itself be + // suitable to pass to the registered callable, no args extraction + // required or even attempted. Only if called with plain (event) do we + // consider extracting args from that event. Initially assume 'event' + // itself contains the arguments. + LLSD args{ event }; + if (fromMap) + { + if (mArity) + { + // We only require/retrieve argskey if the target function + // isn't nullary. For all others, since we require an LLSD + // array, we must have an argskey. + if (argskey.empty()) + { + return callFail("LLEventDispatcher has no args key"); + } + if ((! event.has(argskey))) + { + return callFail("missing required key ", std::quoted(argskey)); + } + args = event[argskey]; + } + } + return ParamsDispatchEntry::call(desc, args, fromMap, argskey); + } + + LLSD addMetadata(LLSD meta) const override { LLSD array(LLSD::emptyArray()); // Resize to number of arguments required @@ -449,7 +501,7 @@ struct LLEventDispatcher::MapParamsDispatchEntry: public LLEventDispatcher::Para MapParamsDispatchEntry(const std::string& name, const std::string& desc, const invoker_function& func, const LLSD& params, const LLSD& defaults): - ParamsDispatchEntry(desc, func), + ParamsDispatchEntry(name, desc, func), mMapper(name, params, defaults), mRequired(LLSD::emptyMap()) { @@ -493,14 +545,25 @@ struct LLEventDispatcher::MapParamsDispatchEntry: public LLEventDispatcher::Para LLSD mRequired; LLSD mOptional; - virtual LLSD call(const std::string& desc, const LLSD& event) const + LLSD call(const std::string& desc, const LLSD& event, bool fromMap, const std::string& argskey) const override { - // Just convert from LLSD::Map to LLSD::Array using mMapper, then pass - // to base-class call() method. - return ParamsDispatchEntry::call(desc, mMapper.map(event)); + // by default, pass the whole event as the arguments map + LLSD args{ event }; + // Were we called by one of the (event) methods (instead of the (name, + // event) methods), do we have an argskey, and does the incoming event + // have that key? + if (fromMap && (! argskey.empty()) && event.has(argskey)) + { + // if so, extract the value of argskey from the incoming event, + // and use that as the arguments map + args = event[argskey]; + } + // Now convert args from LLSD map to LLSD array using mMapper, then + // pass to base-class call() method. + return ParamsDispatchEntry::call(desc, mMapper.map(args), fromMap, argskey); } - virtual LLSD addMetadata(LLSD meta) const + LLSD addMetadata(LLSD meta) const override { meta["required"] = mRequired; meta["optional"] = mOptional; @@ -513,7 +576,11 @@ void LLEventDispatcher::addArrayParamsDispatchEntry(const std::string& name, const invoker_function& invoker, LLSD::Integer arity) { - mDispatch.emplace(name, new ArrayParamsDispatchEntry(desc, invoker, arity)); + // The first parameter to ArrayParamsDispatchEntry is solely for error + // messages. Identify our instance and this entry. + mDispatch.emplace( + name, + new ArrayParamsDispatchEntry(stringize(*this, '[', name, ']'), desc, invoker, arity)); } void LLEventDispatcher::addMapParamsDispatchEntry(const std::string& name, @@ -522,7 +589,11 @@ void LLEventDispatcher::addMapParamsDispatchEntry(const std::string& name, const LLSD& params, const LLSD& defaults) { - mDispatch.emplace(name, new MapParamsDispatchEntry(name, desc, invoker, params, defaults)); + // Pass instance info as well as this entry name for error messages. + mDispatch.emplace( + name, + new MapParamsDispatchEntry(stringize(*this, '[', name, ']'), + desc, invoker, params, defaults)); } /// Register a callable by name @@ -546,174 +617,101 @@ bool LLEventDispatcher::remove(const std::string& name) /// Call a registered callable with an explicitly-specified name. It is an /// error if no such callable exists. -void LLEventDispatcher::operator()(const std::string& name, const LLSD& event) const +LLSD LLEventDispatcher::operator()(const std::string& name, const LLSD& event) const { - std::string error{ try_call_log(std::string(), name, event) }; - if (! error.empty()) - { - callFail(event, error); - } + return try_call(std::string(), name, event); } -/// Extract the @a key value from the incoming @a event, and call the callable -/// whose name is specified by that map @a key. It is an error if no such -/// callable exists. -void LLEventDispatcher::operator()(const LLSD& event) const +bool LLEventDispatcher::try_call(const std::string& name, const LLSD& event) const { - std::string error{ try_call_log(mKey, event[mKey], event) }; - if (! error.empty()) + try { - callFail(event, error); + try_call(std::string(), name, event); + return true; } -} - -void LLEventDispatcher::callFail(const LLSD& event, const std::string& msg) const -{ - // pass back a response that includes an "error" key with the message. - reply(llsd::map("error", msg), event); -} - -void LLEventDispatcher::reply(const LLSD& response, const LLSD& event) const -{ - static LLSD::String key{ "reply" }; - if (event.has(key)) + // Note that we don't catch the generic DispatchError, only the specific + // DispatchMissing. try_call() only promises to return false if the + // specified callable name isn't found -- not for general errors. + catch (const DispatchMissing&) { - // Oh good, the incoming event specifies a reply pump -- pass back - // our response. - sendReply(response, event, key); + return false; } } -bool LLEventDispatcher::try_call(const LLSD& event) const -{ - return try_call_log(mKey, event[mKey], event).empty(); -} - -/*==========================================================================*| - TODO: - -* When mInvoker returns result.isDefined(), sendReply(llsd::map("data", result)) -* When try_call finds name.isArray(), construct response array from - dispatching each call, sendReply() as above -* When try_call finds name.isMap(), construct response map from dispatching - each call, sendReply() as above -- note, caller can't care about order -* Possible future transactional behavior: look up all names before calling any - -|*==========================================================================*/ -bool LLEventDispatcher::try_call(const std::string& name, const LLSD& event) const +/// Extract the @a key value from the incoming @a event, and call the callable +/// whose name is specified by that map @a key. It is an error if no such +/// callable exists. +LLSD LLEventDispatcher::operator()(const LLSD& event) const { - return try_call_log(std::string(), name, event).empty(); + return try_call(mKey, event[mKey], event); } -std::string LLEventDispatcher::try_call_log(const std::string& key, const LLSD& name, - const LLSD& event) const +bool LLEventDispatcher::try_call(const LLSD& event) const { - std::string error{ try_call(key, name, event) }; - if (! error.empty()) + try { - // If we're a subclass of LLEventDispatcher, e.g. LLEventAPI, report that. - error = stringize(LLError::Log::classname(this), "(", mDesc, "): ", error); - LL_WARNS("LLEventDispatcher") << error << LL_ENDL; + try_call(mKey, event[mKey], event); + return true; + } + catch (const DispatchMissing&) + { + return false; } - return error; } -// This internal method returns empty string if the call succeeded, else -// non-empty error message. -std::string LLEventDispatcher::try_call(const std::string& key, const LLSD& name, - const LLSD& event) const +LLSD LLEventDispatcher::try_call(const std::string& key, const std::string& name, + const LLSD& event) const { - if (name.isUndefined()) + if (name.empty()) { if (key.empty()) { - return "attempting to call with no name"; + callFail("attempting to call with no name"); } else { - return stringize("no ", key); - } - } - else if (name.isArray()) - { - return stringize(key, " array dispatch ", name, " not yet implemented"); - } - else if (name.isMap()) - { - return stringize(key, " map dispatch ", name, " not yet implemented"); - } - else if (! name.isString()) - { - return stringize(key, " bad type ", LLSD::typeString(name.type()), ' ', name, - " -- function names are String"); - } - else // name is an LLSD::String - { - auto success{ try_call_one(key, name, event) }; - // pretend to unpack - std::string& error{ success.first }; - LLSD& result{ success.second }; - // did try_call_one() report an error? - if (! error.empty()) - { - // if we have a reply key, respond to invoker - reply(llsd::map("error", error), event); - // now tell caller - return error; - } - // try_call_one() succeeded in calling the target function -- - // should we reply to invoker? - if (result.isUndefined()) - { - // We would get result.isUndefined() if the target function has - // void return. In any case, even if the target function returns - // LLSD, isUndefined() means "don't bother sending response." - return {}; - } - // result.isDefined(): the target function returned something. - // Respond to invoker if we have a "reply" key. - if (! result.isMap()) - { - // wrap result in a map to play well with sendReply() - result = llsd::map("data", result); + callFail("no ", key); } - reply(result, event); - return {}; } -} -std::pair -LLEventDispatcher::try_call_one(const std::string& key, const std::string& name, - const LLSD& event) const -{ DispatchMap::const_iterator found = mDispatch.find(name); if (found == mDispatch.end()) { + // Here we were passed a valid name, but there's no registered + // callable with that name. This is the one case in which we throw + // DispatchMissing instead of the generic DispatchError. + // Distinguish the public method by which our caller reached here: + // key.empty() means the name was passed explicitly, non-empty means + // we extracted the name from the incoming event using that key. if (key.empty()) { - return { stringize("'", name, "' not found"), {} }; + callFail(std::quoted(name), " not found"); } else { - return { stringize("bad ", key, " value '", name, "'"), {} }; + callFail("bad ", key, " value ", std::quoted(name)); } } - try - { - // Found the name, so it's plausible to even attempt the call. - return { {}, found->second->call(stringize("calling '", name, "'"), event) }; - } - catch (const DispatchError& err) - { - // trouble preparing arguments - return { err.what(), {} }; - } - catch (const LL::apply_error& err) - { - // could also hit runtime errors with LL::apply() - return { err.what(), {} }; - } + // Found the name, so it's plausible to even attempt the call. + return found->second->call(stringize(*this, " calling ", std::quoted(name)), + event, (! key.empty()), mArgskey); +} + +template +//static +void LLEventDispatcher::sCallFail(ARGS&&... args) +{ + auto error = stringize(std::forward(args)...); + LL_WARNS("LLEventDispatcher") << error << LL_ENDL; + LLTHROW(EXCEPTION(error)); +} + +template +void LLEventDispatcher::callFail(ARGS&&... args) const +{ + // Describe this instance in addition to the error itself. + sCallFail(*this, ": ", std::forward(args)...); } LLSD LLEventDispatcher::getMetadata(const std::string& name) const @@ -729,27 +727,69 @@ LLSD LLEventDispatcher::getMetadata(const std::string& name) const return found->second->addMetadata(meta); } +std::ostream& operator<<(std::ostream& out, const LLEventDispatcher& self) +{ + // If we're a subclass of LLEventDispatcher, e.g. LLEventAPI, report that. + return out << LLError::Log::classname(self) << '(' << self.mDesc << ')'; +} + /***************************************************************************** * LLDispatchListener *****************************************************************************/ -LLDispatchListener::LLDispatchListener(const std::string& pumpname, const std::string& key): - LLEventDispatcher(pumpname, key), - // Do NOT tweak the passed pumpname. In practice, when someone - // instantiates a subclass of our LLEventAPI subclass, they intend to - // claim that LLEventPump name in the global LLEventPumps namespace. It - // would be mysterious and distressing if we allowed name tweaking, and - // someone else claimed pumpname first for a completely unrelated - // LLEventPump. Posted events would never reach our subclass listener - // because we would have silently changed its name; meanwhile listeners - // (if any) on that other LLEventPump would be confused by the events - // intended for our subclass. - LLEventStream(pumpname, false), - mBoundListener(listen("self", [this](const LLSD& event){ return process(event); })) +/*==========================================================================*| + TODO: +* When process() finds name.isArray(), construct response array from + dispatching each call -- args must also be (array of args structures) + (could also construct response map, IF array contains unique names) +* When process() finds name.isMap(), construct response map from dispatching + each call -- value of each key is its args struct -- argskey ignored -- + note, caller can't care about order +* Possible future transactional behavior: look up all names before calling any +|*==========================================================================*/ +std::string LLDispatchListener::mReplyKey{ "reply" }; + +bool LLDispatchListener::process(const LLSD& event) const { + LLSD result; + try + { + result = (*this)(event); + } + catch (const DispatchError& err) + { + // If the incoming event contains a "reply" key, we'll respond to the + // invoker with an error message (below). But if not -- silently + // ignoring an invocation request would be confusing at best. Escalate. + if (! event.has(mReplyKey)) + { + throw; + } + + // reply with a map containing an "error" key explaining the problem + reply(llsd::map("error", err.what()), event); + return false; + } + + // We seem to have gotten a valid result. But we don't know whether the + // registered callable is void or non-void. If it's void, + // LLEventDispatcher will return isUndefined(). Otherwise, try to send it + // back to our invoker. + if (result.isDefined()) + { + if (! result.isMap()) + { + // wrap the result in a map as the "data" key + result = llsd::map("data", result); + } + reply(result, event); + } + return false; } -bool LLDispatchListener::process(const LLSD& event) +void LLDispatchListener::reply(const LLSD& reply, const LLSD& request) const { - (*this)(event); - return false; + // Call sendReply() unconditionally: sendReply() itself tests whether the + // specified reply key is present in the incoming request, and does + // nothing if there's no such key. + sendReply(reply, request, mReplyKey); } -- cgit v1.2.3 From 9553965ad661b2753d13fa9b414f529ad440000f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 10 Jan 2023 17:38:01 -0500 Subject: DRTVWR-558: Add tests for LLDispatchListener functionality. Refine the special case of calling a nullary target function from an (event) method, notably via LLDispatchListener. (cherry picked from commit edcc52a9f60b1ec9b8f53603d6e2676558d41294) --- indra/llcommon/lleventdispatcher.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 7e5723c503..7abdc8f57a 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -461,7 +461,13 @@ struct LLEventDispatcher::ArrayParamsDispatchEntry: public LLEventDispatcher::Pa LLSD args{ event }; if (fromMap) { - if (mArity) + if (! mArity) + { + // When the target function is nullary, and we're called from + // an (event) method, just ignore the rest of the map entries. + args.clear(); + } + else { // We only require/retrieve argskey if the target function // isn't nullary. For all others, since we require an LLSD -- cgit v1.2.3 From 27f19826b6ef9b4af5db8613c44988b5c973618b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 12 Jan 2023 10:49:01 -0500 Subject: DRTVWR-558: Break out new LLDispatchListener::call() method. This captures logic we intend to reuse for forthcoming LLDispatchListener batched request support. (cherry picked from commit 3cb6d374cb76e4b00dc121255e8f5aa4e777fa27) --- indra/llcommon/lleventdispatcher.cpp | 69 +++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 20 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 7abdc8f57a..8e20d7184a 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -742,6 +742,8 @@ std::ostream& operator<<(std::ostream& out, const LLEventDispatcher& self) /***************************************************************************** * LLDispatchListener *****************************************************************************/ +std::string LLDispatchListener::mReplyKey{ "reply" }; + /*==========================================================================*| TODO: * When process() finds name.isArray(), construct response array from @@ -752,33 +754,27 @@ std::ostream& operator<<(std::ostream& out, const LLEventDispatcher& self) note, caller can't care about order * Possible future transactional behavior: look up all names before calling any |*==========================================================================*/ -std::string LLDispatchListener::mReplyKey{ "reply" }; - bool LLDispatchListener::process(const LLSD& event) const { - LLSD result; - try - { - result = (*this)(event); - } - catch (const DispatchError& err) - { - // If the incoming event contains a "reply" key, we'll respond to the - // invoker with an error message (below). But if not -- silently - // ignoring an invocation request would be confusing at best. Escalate. - if (! event.has(mReplyKey)) - { - throw; - } - - // reply with a map containing an "error" key explaining the problem - reply(llsd::map("error", err.what()), event); + // Collecting errors is only meaningful with a reply key. Without one, if + // an error occurs, let the exception propagate. + auto returned = call("", event, (! event.has(mReplyKey))); + std::string& error{ returned.first }; + LLSD& result{ returned.second }; + + if (! error.empty()) + { + // Here there was an error and the incoming event has mReplyKey -- + // else DispatchError would already have propagated out of the call() + // above. Reply with a map containing an "error" key explaining the + // problem. + reply(llsd::map("error", error), event); return false; } // We seem to have gotten a valid result. But we don't know whether the // registered callable is void or non-void. If it's void, - // LLEventDispatcher will return isUndefined(). Otherwise, try to send it + // LLEventDispatcher returned isUndefined(). Otherwise, try to send it // back to our invoker. if (result.isDefined()) { @@ -792,6 +788,39 @@ bool LLDispatchListener::process(const LLSD& event) const return false; } +// Pass empty name to call LLEventDispatcher::operator()(const LLSD&), +// non-empty name to call operator()(const std::string&, const LLSD&). +// Returns (empty string, return value) on successful call. +// Returns (error message, undefined) if error and 'exception' is false. +// Throws DispatchError if error and 'exception' is true. +std::pair LLDispatchListener::call(const std::string& name, const LLSD& event, + bool exception) const +{ + try + { + if (name.empty()) + { + // unless this throws, return (empty string, real return value) + return { {}, (*this)(event) }; + } + else + { + // unless this throws, return (empty string, real return value) + return { {}, (*this)(name, event) }; + } + } + catch (const DispatchError& err) + { + if (exception) + { + // Caller asked for an exception on error. Oblige. + throw; + } + // Caller does NOT want an exception: return (error message, undefined) + return { err.what(), LLSD() }; + } +} + void LLDispatchListener::reply(const LLSD& reply, const LLSD& request) const { // Call sendReply() unconditionally: sendReply() itself tests whether the -- cgit v1.2.3 From d637229beeb3b7fa2bc7adb850ce9337b119037c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 13 Jan 2023 00:10:23 -0500 Subject: DRTVWR-558: Introduce LLDispatchListener batched requests. Now the value of the incoming event's dispatch key may be an LLSD::String (as before), a map or an array, as documented in the augmented Doxygen class comments. LLDispatchListener will attempt multiple calls before sending a reply. (cherry picked from commit 7671b37285c6cdf1afcddb0019311a822c8a4dc5) --- indra/llcommon/lleventdispatcher.cpp | 183 +++++++++++++++++++++++++++-------- 1 file changed, 141 insertions(+), 42 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 8e20d7184a..d10cf16b88 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -744,32 +744,44 @@ std::ostream& operator<<(std::ostream& out, const LLEventDispatcher& self) *****************************************************************************/ std::string LLDispatchListener::mReplyKey{ "reply" }; -/*==========================================================================*| - TODO: -* When process() finds name.isArray(), construct response array from - dispatching each call -- args must also be (array of args structures) - (could also construct response map, IF array contains unique names) -* When process() finds name.isMap(), construct response map from dispatching - each call -- value of each key is its args struct -- argskey ignored -- - note, caller can't care about order -* Possible future transactional behavior: look up all names before calling any -|*==========================================================================*/ bool LLDispatchListener::process(const LLSD& event) const { - // Collecting errors is only meaningful with a reply key. Without one, if - // an error occurs, let the exception propagate. - auto returned = call("", event, (! event.has(mReplyKey))); - std::string& error{ returned.first }; - LLSD& result{ returned.second }; + // Decide what to do based on the incoming value of the specified dispatch + // key. + LLSD name{ event[getDispatchKey()] }; + if (name.isMap()) + { + call_map(name, event); + } + else if (name.isArray()) + { + call_array(name, event); + } + else + { + call_one(name, event); + } + return false; +} - if (! error.empty()) +void LLDispatchListener::call_one(const LLSD& name, const LLSD& event) const +{ + LLSD result; + try { - // Here there was an error and the incoming event has mReplyKey -- - // else DispatchError would already have propagated out of the call() - // above. Reply with a map containing an "error" key explaining the - // problem. - reply(llsd::map("error", error), event); - return false; + result = (*this)(event); + } + catch (const DispatchError& err) + { + if (! event.has(mReplyKey)) + { + // Without a reply key, let the exception propagate. + throw; + } + + // Here there was an error and the incoming event has mReplyKey. Reply + // with a map containing an "error" key explaining the problem. + return reply(llsd::map("error", err.what()), event); } // We seem to have gotten a valid result. But we don't know whether the @@ -785,40 +797,127 @@ bool LLDispatchListener::process(const LLSD& event) const } reply(result, event); } - return false; } -// Pass empty name to call LLEventDispatcher::operator()(const LLSD&), -// non-empty name to call operator()(const std::string&, const LLSD&). -// Returns (empty string, return value) on successful call. -// Returns (error message, undefined) if error and 'exception' is false. -// Throws DispatchError if error and 'exception' is true. -std::pair LLDispatchListener::call(const std::string& name, const LLSD& event, - bool exception) const +void LLDispatchListener::call_map(const LLSD& reqmap, const LLSD& event) const { - try + // LLSD map containing returned values + LLSD result; + // collect any error messages here + std::ostringstream errors; + const char* delim = ""; + + for (const auto& pair : llsd::inMap(reqmap)) + { + const LLSD::String& name{ pair.first }; + const LLSD& args{ pair.second }; + try + { + // With this form, capture return value even if undefined: + // presence of the key in the response map can be used to detect + // which request keys succeeded. + result[name] = (*this)(name, args); + } + catch (const DispatchError& err) + { + // collect message in 'errors', with hint as to which request map + // entry failed + errors << delim << err.what() << " (" << getDispatchKey() + << '[' << name << "])"; + delim = "\n"; + } + } + + // so, were there any errors? + std::string error = errors.str(); + if (! error.empty()) { - if (name.empty()) + if (! event.has(mReplyKey)) { - // unless this throws, return (empty string, real return value) - return { {}, (*this)(event) }; + // can't send reply, throw + sCallFail(error); } else { - // unless this throws, return (empty string, real return value) - return { {}, (*this)(name, event) }; + // reply key present + result["error"] = error; } } - catch (const DispatchError& err) + + reply(result, event); +} + +void LLDispatchListener::call_array(const LLSD& reqarray, const LLSD& event) const +{ + // LLSD array containing returned values + LLSD results; + // arguments array, if present -- const because, if it's shorter than + // reqarray, we don't want to grow it + const LLSD argsarray{ event[getArgsKey()] }; + // error message, if any + std::string error; + + // classic index loop because we need the index + for (size_t i = 0, size = reqarray.size(); i < size; ++i) + { + const auto& reqentry{ reqarray[i] }; + std::string name; + LLSD args; + if (reqentry.isString()) + { + name = reqentry.asString(); + args = argsarray[i]; + } + else if (reqentry.isArray() && reqentry.size() == 2 && reqentry[0].isString()) + { + name = reqentry[0].asString(); + args = reqentry[1]; + } + else + { + // reqentry isn't in either of the documented forms + error = stringize(*this, ": ", getDispatchKey(), '[', i, "] ", + reqentry, " unsupported"); + break; + } + + // reqentry is one of the valid forms, got name and args + try + { + // With this form, capture return value even if undefined + results.append((*this)(name, args)); + } + catch (const DispatchError& err) + { + // append hint as to which requentry produced the error + error = stringize(err.what(), " (", getDispatchKey(), '[', i, ']'); + break; + } + } + + LLSD result; + // was there an error? + if (! error.empty()) { - if (exception) + if (! event.has(mReplyKey)) { - // Caller asked for an exception on error. Oblige. - throw; + // can't send reply, throw + sCallFail(error); + } + else + { + // reply key present + result["error"] = error; } - // Caller does NOT want an exception: return (error message, undefined) - return { err.what(), LLSD() }; } + + // wrap the results array as response map "data" key, as promised + if (results.isDefined()) + { + result["data"] = results; + } + + reply(result, event); } void LLDispatchListener::reply(const LLSD& reply, const LLSD& request) const -- cgit v1.2.3 From 2c894ecb25de044f5cb9c408c5264e5234b73983 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 20 Jan 2023 22:34:31 -0500 Subject: DRTVWR-558: Extend LLEventDispatcher::add() overloads. Add LL::always_return(), which takes a callable and variadic arguments. It calls the callable with those arguments and, if the returned type is convertible to T, converts it and returns it. Otherwise it returns T(). always_return() is generalized from, and supersedes, LLEventDispatcher::ReturnLLSD. Add LL::function_arity, which extends boost::function_types::function_arity by reporting results for both std::function and boost::function. Use for LL::apply(function, LLSD array) as well as for LLEventDispatcher. Make LLEventDispatcher::add() overloads uniformly distinguish between a callable (whether non-static member function or otherwise) that accepts a single LLSD parameter, versus any other signature. Accepting exactly one LLSD parameter signals that the callable will accept the composite arguments LLSD blob, instead of asking LLEventDispatcher to unpack the arguments blob into individual arguments. Support add(subclass method) overloads for arbitrary-parameters methods as well as for (const LLSD&) methods. Update tests accordingly: we need no longer pass the boilerplate lambda instance getter that binds and returns 'this'. Extract to the two LLEventDispatcher::make_invoker() overloads the LL::apply() logic formerly found in ReturnLLSD. Change lleventdispatcher_test.cpp tests from boost::bind(), which accepts variadic arguments (even though it only passes a fixed set to the target callable), to fixed-signature lambdas. This is because the revamped add() overloads care about signature. Add a test for a non-static method that accepts (const LLSD&), in other words the composite arguments LLSD blob, and likewise returns LLSD. (cherry picked from commit 95b787f7d7226ee9de79dfc9816f33c8bf199aad) --- indra/llcommon/lleventdispatcher.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index d10cf16b88..caff854753 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -384,8 +384,7 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE (desc, ": bad request: ", mismatch); } // Event syntax looks good, go for it! - mFunc(event); - return {}; + return mFunc(event); } LLSD addMetadata(LLSD meta) const override @@ -603,8 +602,8 @@ void LLEventDispatcher::addMapParamsDispatchEntry(const std::string& name, } /// Register a callable by name -void LLEventDispatcher::add(const std::string& name, const std::string& desc, - const Callable& callable, const LLSD& required) +void LLEventDispatcher::addLLSD(const std::string& name, const std::string& desc, + const Callable& callable, const LLSD& required) { mDispatch.emplace(name, new LLSDDispatchEntry(desc, callable, required)); } -- cgit v1.2.3 From c747ff0925fb85147a96745bb55e66e7e8004fd8 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 21 Jan 2023 11:13:04 -0500 Subject: DRTVWR-558: Enrich LLEventDispatcher::callFail() with current call. Now an LLEventAPI subclass method can call callFail(...) to report an error, and the error will be annotated with the leaf class name, the instance name and the way the method was reached. The enriched error message will be logged and either sent back to the invoker or propagated as an exception, depending on the invocation tactic. In other words, a business method can use callFail() to Do The Right Thing according to the LLEventDispatcher contract. Introduce a nested SetState RAII class to set and clear transient state. SetState's constructor accepts variadic stringize() arguments. The resulting message is passed to LLEventDispatcher::setState(), which requires a SetState reference because ONLY SetState should call setState(): state data really is intended to be transient. SetState guarantees it will be cleared every time it's set. setState() respects previously-set transient state. If a call from an inner function finds that transient state was already set by some ancestor, it ignores the call and informs the caller by returning false. This lets a given SetState instance recognize whether it is responsible for clearing the current transient state. operator<<(std::ostream&, const LLEventDispatcher&) now appends getState() to the data reported by streaming *this. Non-static LLEventDispatcher::callFail() already prepends *this to the reported error message. Transient state is managed by a fiber_specific_ptr, since different threads and even different fibers within a thread might be concurrently performing different operations on the same LLEventDispatcher. Introduce a back pointer to the parent LLEventDispatcher in DispatchEntry. Populate it with a new constructor parameter, propagated through every subclass constructor. Hoist ParamsDispatchEntry::callFail() up into its DispatchEntry base class. Make it call non-static LLEventDispatcher:: callFail(), which prepends the reported error with instance and transient state info. Use DispatchEntry::callFail() in LLSDDispatchEntry::call(), instead of redundantly calling LLEventDispatcher::callFail(). Similarly, introduce an LLEventDispatcher back pointer into LLSDArgsMapper for use by its own callFail() method. The above should (!) eliminate the need to replicate LLEventDispatcher instance info into every helper object's descriptive strings. In particular, since the previous info was stored in each object by its constructor, it couldn't report associated transient information about how the subject callable was actually reached. Traversing a back pointer to the live LLEventDispatcher instance gets us the most current state. Make the internal three-argument LLEventDispatcher::try_call() method, which implements each of the operator()() and public try_call() methods, use SetState to append "[name]" (for explicit operator()(name, event) calls) or "[key=name]" (for implicit operator()(event) calls) to streamed *this. In the new LLDispatchListener request array and request map operations, use SetState to indicate the current entry in the array or map, overriding the lower-level state set by three-argument LLEventDispatcher::try_call(). (cherry picked from commit 2f8d7d20f43ab411ea0fe8b756cb696954acfb3e) --- indra/llcommon/lleventdispatcher.cpp | 118 ++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 42 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index caff854753..0079b9ce36 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -103,7 +103,8 @@ class LL_COMMON_API LLEventDispatcher::LLSDArgsMapper public: /// Accept description of function: function name, param names, param /// default values - LLSDArgsMapper(const std::string& function, const LLSD& names, const LLSD& defaults); + LLSDArgsMapper(LLEventDispatcher* parent, const std::string& function, + const LLSD& names, const LLSD& defaults); /// Given arguments map, return LLSD::Array of parameter values, or /// trigger error. @@ -114,6 +115,9 @@ private: template void callFail(ARGS&&... args) const; + // store a plain dumb back-pointer because we don't have to manage the + // parent LLEventDispatcher's lifespan + LLEventDispatcher* _parent; // The function-name string is purely descriptive. We want error messages // to be able to indicate which function's LLSDArgsMapper has the problem. std::string _function; @@ -132,9 +136,11 @@ private: FilledVector _has_dft; }; -LLEventDispatcher::LLSDArgsMapper::LLSDArgsMapper(const std::string& function, +LLEventDispatcher::LLSDArgsMapper::LLSDArgsMapper(LLEventDispatcher* parent, + const std::string& function, const LLSD& names, const LLSD& defaults): + _parent(parent), _function(function), _names(names), _has_dft(names.size()) @@ -334,7 +340,7 @@ std::string LLEventDispatcher::LLSDArgsMapper::formatlist(const LLSD& list) template void LLEventDispatcher::LLSDArgsMapper::callFail(ARGS&&... args) const { - LLEventDispatcher::sCallFail + _parent->callFail (_function, std::forward(args)...); } @@ -356,7 +362,8 @@ LLEventDispatcher::~LLEventDispatcher() { } -LLEventDispatcher::DispatchEntry::DispatchEntry(const std::string& desc): +LLEventDispatcher::DispatchEntry::DispatchEntry(LLEventDispatcher* parent, const std::string& desc): + mParent(parent), mDesc(desc) {} @@ -365,8 +372,9 @@ LLEventDispatcher::DispatchEntry::DispatchEntry(const std::string& desc): */ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchEntry { - LLSDDispatchEntry(const std::string& desc, const Callable& func, const LLSD& required): - DispatchEntry(desc), + LLSDDispatchEntry(LLEventDispatcher* parent, const std::string& desc, + const Callable& func, const LLSD& required): + DispatchEntry(parent, desc), mFunc(func), mRequired(required) {} @@ -380,8 +388,7 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE std::string mismatch(llsd_matches(mRequired, event)); if (! mismatch.empty()) { - LLEventDispatcher::sCallFail - (desc, ": bad request: ", mismatch); + return callFail(desc, ": bad request: ", mismatch); } // Event syntax looks good, go for it! return mFunc(event); @@ -400,9 +407,9 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE */ struct LLEventDispatcher::ParamsDispatchEntry: public LLEventDispatcher::DispatchEntry { - ParamsDispatchEntry(const std::string& name, const std::string& desc, - const invoker_function& func): - DispatchEntry(desc), + ParamsDispatchEntry(LLEventDispatcher* parent, const std::string& name, + const std::string& desc, const invoker_function& func): + DispatchEntry(parent, desc), mName(name), mInvoker(func) {} @@ -422,14 +429,6 @@ struct LLEventDispatcher::ParamsDispatchEntry: public LLEventDispatcher::Dispatc return callFail(err.what()); } } - - template - LLSD callFail(ARGS&&... args) const - { - LLEventDispatcher::sCallFail(mName, ": ", std::forward(args)...); - // pacify the compiler - return {}; - } }; /** @@ -438,9 +437,10 @@ struct LLEventDispatcher::ParamsDispatchEntry: public LLEventDispatcher::Dispatc */ struct LLEventDispatcher::ArrayParamsDispatchEntry: public LLEventDispatcher::ParamsDispatchEntry { - ArrayParamsDispatchEntry(const std::string& name, const std::string& desc, - const invoker_function& func, LLSD::Integer arity): - ParamsDispatchEntry(name, desc, func), + ArrayParamsDispatchEntry(LLEventDispatcher* parent, const std::string& name, + const std::string& desc, const invoker_function& func, + LLSD::Integer arity): + ParamsDispatchEntry(parent, name, desc, func), mArity(arity) {} @@ -503,11 +503,11 @@ struct LLEventDispatcher::ArrayParamsDispatchEntry: public LLEventDispatcher::Pa */ struct LLEventDispatcher::MapParamsDispatchEntry: public LLEventDispatcher::ParamsDispatchEntry { - MapParamsDispatchEntry(const std::string& name, const std::string& desc, - const invoker_function& func, + MapParamsDispatchEntry(LLEventDispatcher* parent, const std::string& name, + const std::string& desc, const invoker_function& func, const LLSD& params, const LLSD& defaults): - ParamsDispatchEntry(name, desc, func), - mMapper(name, params, defaults), + ParamsDispatchEntry(parent, name, desc, func), + mMapper(parent, name, params, defaults), mRequired(LLSD::emptyMap()) { // Build the set of all param keys, then delete the ones that are @@ -581,11 +581,9 @@ void LLEventDispatcher::addArrayParamsDispatchEntry(const std::string& name, const invoker_function& invoker, LLSD::Integer arity) { - // The first parameter to ArrayParamsDispatchEntry is solely for error - // messages. Identify our instance and this entry. mDispatch.emplace( name, - new ArrayParamsDispatchEntry(stringize(*this, '[', name, ']'), desc, invoker, arity)); + new ArrayParamsDispatchEntry(this, "", desc, invoker, arity)); } void LLEventDispatcher::addMapParamsDispatchEntry(const std::string& name, @@ -597,15 +595,14 @@ void LLEventDispatcher::addMapParamsDispatchEntry(const std::string& name, // Pass instance info as well as this entry name for error messages. mDispatch.emplace( name, - new MapParamsDispatchEntry(stringize(*this, '[', name, ']'), - desc, invoker, params, defaults)); + new MapParamsDispatchEntry(this, "", desc, invoker, params, defaults)); } /// Register a callable by name void LLEventDispatcher::addLLSD(const std::string& name, const std::string& desc, const Callable& callable, const LLSD& required) { - mDispatch.emplace(name, new LLSDDispatchEntry(desc, callable, required)); + mDispatch.emplace(name, new LLSDDispatchEntry(this, desc, callable, required)); } /// Unregister a callable @@ -682,7 +679,7 @@ LLSD LLEventDispatcher::try_call(const std::string& key, const std::string& name DispatchMap::const_iterator found = mDispatch.find(name); if (found == mDispatch.end()) { - // Here we were passed a valid name, but there's no registered + // Here we were passed a non-empty name, but there's no registered // callable with that name. This is the one case in which we throw // DispatchMissing instead of the generic DispatchError. // Distinguish the public method by which our caller reached here: @@ -699,8 +696,10 @@ LLSD LLEventDispatcher::try_call(const std::string& key, const std::string& name } // Found the name, so it's plausible to even attempt the call. - return found->second->call(stringize(*this, " calling ", std::quoted(name)), - event, (! key.empty()), mArgskey); + const char* delim = (key.empty()? "" : "="); + // append either "[key=name]" or just "[name]" + SetState transient(this, '[', key, delim, name, ']'); + return found->second->call("", event, (! key.empty()), mArgskey); } template @@ -735,7 +734,34 @@ LLSD LLEventDispatcher::getMetadata(const std::string& name) const std::ostream& operator<<(std::ostream& out, const LLEventDispatcher& self) { // If we're a subclass of LLEventDispatcher, e.g. LLEventAPI, report that. - return out << LLError::Log::classname(self) << '(' << self.mDesc << ')'; + // Also report whatever transient state is active. + return out << LLError::Log::classname(self) << '(' << self.mDesc << ')' + << self.getState(); +} + +std::string LLEventDispatcher::getState() const +{ + // default value of fiber_specific_ptr is nullptr, and ~SetState() reverts + // to that; infer empty string + if (! mState.get()) + return {}; + else + return *mState; +} + +bool LLEventDispatcher::setState(SetState&, const std::string& state) const +{ + // If SetState is instantiated at multiple levels of function call, ignore + // the lower-level call because the outer call presumably provides more + // context. + if (mState.get()) + return false; + + // Pass us empty string (a la ~SetState()) to reset to nullptr, else take + // a heap copy of the passed state string so we can delete it on + // subsequent reset(). + mState.reset(state.empty()? nullptr : new std::string(state)); + return true; } /***************************************************************************** @@ -802,6 +828,8 @@ void LLDispatchListener::call_map(const LLSD& reqmap, const LLSD& event) const { // LLSD map containing returned values LLSD result; + // cache dispatch key + std::string key{ getDispatchKey() }; // collect any error messages here std::ostringstream errors; const char* delim = ""; @@ -812,6 +840,9 @@ void LLDispatchListener::call_map(const LLSD& reqmap, const LLSD& event) const const LLSD& args{ pair.second }; try { + // in case of errors, tell user the dispatch key, the fact that + // we're processing a request map and the current key in that map + SetState(this, '[', key, '[', name, "]]"); // With this form, capture return value even if undefined: // presence of the key in the response map can be used to detect // which request keys succeeded. @@ -819,10 +850,8 @@ void LLDispatchListener::call_map(const LLSD& reqmap, const LLSD& event) const } catch (const DispatchError& err) { - // collect message in 'errors', with hint as to which request map - // entry failed - errors << delim << err.what() << " (" << getDispatchKey() - << '[' << name << "])"; + // collect message in 'errors' + errors << delim << err.what(); delim = "\n"; } } @@ -850,6 +879,8 @@ void LLDispatchListener::call_array(const LLSD& reqarray, const LLSD& event) con { // LLSD array containing returned values LLSD results; + // cache the dispatch key + std::string key{ getDispatchKey() }; // arguments array, if present -- const because, if it's shorter than // reqarray, we don't want to grow it const LLSD argsarray{ event[getArgsKey()] }; @@ -883,13 +914,16 @@ void LLDispatchListener::call_array(const LLSD& reqarray, const LLSD& event) con // reqentry is one of the valid forms, got name and args try { + // in case of errors, tell user the dispatch key, the fact that + // we're processing a request array, the current entry in that + // array and the corresponding callable name + SetState(this, '[', key, '[', i, "]=", name, ']'); // With this form, capture return value even if undefined results.append((*this)(name, args)); } catch (const DispatchError& err) { - // append hint as to which requentry produced the error - error = stringize(err.what(), " (", getDispatchKey(), '[', i, ']'); + error = err.what(); break; } } -- cgit v1.2.3 From 2eb0ea9593d0e299445d2e1dde711bfe5072542e Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 23 Jan 2023 10:51:33 -0500 Subject: DRTVWR-558: Nail down LLDispatchListener exception handling for exceptions other than those thrown by base-class LLEventDispatcher. Explain in LLDispatchListener Doxygen comments that for a request lacking a "reply" key, any exception is allowed to propagate because it's likely to reach the post() call that triggered the exception in the first place. For batch LLDispatchListener operations, catch not only LLEventDispatcher:: DispatchError exceptions but any std::exception, so we can collect them to report to the invoker. "Gotta catch 'em all!" Make LLLeap catch any std::exception thrown by processing a request from the plugin child process, log it and send a reply to the plugin. No plugin should be allowed to crash the viewer. (cherry picked from commit 94e10fd039b79f71ed8d7e10807b6e4eebd1928c) --- indra/llcommon/lleventdispatcher.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 0079b9ce36..a4c0ed3766 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -848,10 +848,12 @@ void LLDispatchListener::call_map(const LLSD& reqmap, const LLSD& event) const // which request keys succeeded. result[name] = (*this)(name, args); } - catch (const DispatchError& err) + catch (const std::exception& err) { - // collect message in 'errors' - errors << delim << err.what(); + // Catch not only DispatchError, but any C++ exception thrown by + // the target callable. Collect exception name and message in + // 'errors'. + errors << delim << LLError::Log::classname(err) << ": " << err.what(); delim = "\n"; } } @@ -921,9 +923,12 @@ void LLDispatchListener::call_array(const LLSD& reqarray, const LLSD& event) con // With this form, capture return value even if undefined results.append((*this)(name, args)); } - catch (const DispatchError& err) + catch (const std::exception& err) { - error = err.what(); + // Catch not only DispatchError, but any C++ exception thrown by + // the target callable. Report the exception class as well as the + // error string. + error = stringize(LLError::Log::classname(err), ": ", err.what()); break; } } -- cgit v1.2.3 From 7c79d7a7d4d5cb1e39293cdc98fd972be5bd3012 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 24 Jan 2023 13:31:50 -0500 Subject: DRTVWR-558: Fix merge glitch: missing LLEventDispatcher::addFail() (cherry picked from commit 3be250da90dd3d361df713056b881e017684e2b3) --- indra/llcommon/lleventdispatcher.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index a4c0ed3766..5d18d8f6c4 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -605,6 +605,14 @@ void LLEventDispatcher::addLLSD(const std::string& name, const std::string& desc mDispatch.emplace(name, new LLSDDispatchEntry(this, desc, callable, required)); } +void LLEventDispatcher::addFail(const std::string& name, const char* classname) const +{ + LL_ERRS("LLEventDispatcher") << "LLEventDispatcher(" << mDesc << ")::add(" << name + << "): " << LLError::Log::demangle(classname) + << " is not a subclass of LLEventDispatcher" + << LL_ENDL; +} + /// Unregister a callable bool LLEventDispatcher::remove(const std::string& name) { -- cgit v1.2.3 From 3fce3d14d6b8e367f4136efbfe87fcfcb23a4c8e Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 7 Mar 2023 17:39:03 -0500 Subject: DRTVWR-558: Avoid extra copy of getMetadata() LLSD map. (cherry picked from commit 2c1253c8ed2a1648317e6edd768b3fda00c56ce2) --- indra/llcommon/lleventdispatcher.cpp | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 5d18d8f6c4..99e2e74376 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -394,10 +394,9 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE return mFunc(event); } - LLSD addMetadata(LLSD meta) const override + LLSD getMetadata() const override { - meta["required"] = mRequired; - return meta; + return llsd::map("required", mRequired); } }; @@ -485,15 +484,14 @@ struct LLEventDispatcher::ArrayParamsDispatchEntry: public LLEventDispatcher::Pa return ParamsDispatchEntry::call(desc, args, fromMap, argskey); } - LLSD addMetadata(LLSD meta) const override + LLSD getMetadata() const override { LLSD array(LLSD::emptyArray()); // Resize to number of arguments required if (mArity) array[mArity - 1] = LLSD(); llassert_always(array.size() == mArity); - meta["required"] = array; - return meta; + return llsd::map("required", array); } }; @@ -568,11 +566,9 @@ struct LLEventDispatcher::MapParamsDispatchEntry: public LLEventDispatcher::Para return ParamsDispatchEntry::call(desc, mMapper.map(args), fromMap, argskey); } - LLSD addMetadata(LLSD meta) const override + LLSD getMetadata() const override { - meta["required"] = mRequired; - meta["optional"] = mOptional; - return meta; + return llsd::map("required", mRequired, "optional", mOptional); } }; @@ -733,10 +729,10 @@ LLSD LLEventDispatcher::getMetadata(const std::string& name) const { return LLSD(); } - LLSD meta; + LLSD meta{ found->second->getMetadata() }; meta["name"] = name; meta["desc"] = found->second->mDesc; - return found->second->addMetadata(meta); + return meta; } std::ostream& operator<<(std::ostream& out, const LLEventDispatcher& self) -- cgit v1.2.3 From bfc9772d61cadc88b3fdf6f553c60e73c79e83ed Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 25 Jul 2023 11:18:12 -0400 Subject: DRTVWR-587: Use [[noreturn]] attribute on callFail() methods that unconditionally return. This eliminates the problem of pacifying a compiler that expects a return statement vs. a compiler that detects that callFail() unconditionally throws. Thanks, Ansariel. --- indra/llcommon/lleventdispatcher.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'indra/llcommon/lleventdispatcher.cpp') diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp index 99e2e74376..da96de18f7 100644 --- a/indra/llcommon/lleventdispatcher.cpp +++ b/indra/llcommon/lleventdispatcher.cpp @@ -113,7 +113,7 @@ public: private: static std::string formatlist(const LLSD&); template - void callFail(ARGS&&... args) const; + [[noreturn]] void callFail(ARGS&&... args) const; // store a plain dumb back-pointer because we don't have to manage the // parent LLEventDispatcher's lifespan @@ -338,7 +338,7 @@ std::string LLEventDispatcher::LLSDArgsMapper::formatlist(const LLSD& list) } template -void LLEventDispatcher::LLSDArgsMapper::callFail(ARGS&&... args) const +[[noreturn]] void LLEventDispatcher::LLSDArgsMapper::callFail(ARGS&&... args) const { _parent->callFail (_function, std::forward(args)...); @@ -388,7 +388,7 @@ struct LLEventDispatcher::LLSDDispatchEntry: public LLEventDispatcher::DispatchE std::string mismatch(llsd_matches(mRequired, event)); if (! mismatch.empty()) { - return callFail(desc, ": bad request: ", mismatch); + callFail(desc, ": bad request: ", mismatch); } // Event syntax looks good, go for it! return mFunc(event); @@ -425,7 +425,7 @@ struct LLEventDispatcher::ParamsDispatchEntry: public LLEventDispatcher::Dispatc catch (const LL::apply_error& err) { // could hit runtime errors with LL::apply() - return callFail(err.what()); + callFail(err.what()); } } }; @@ -472,11 +472,11 @@ struct LLEventDispatcher::ArrayParamsDispatchEntry: public LLEventDispatcher::Pa // array, we must have an argskey. if (argskey.empty()) { - return callFail("LLEventDispatcher has no args key"); + callFail("LLEventDispatcher has no args key"); } if ((! event.has(argskey))) { - return callFail("missing required key ", std::quoted(argskey)); + callFail("missing required key ", std::quoted(argskey)); } args = event[argskey]; } @@ -708,7 +708,7 @@ LLSD LLEventDispatcher::try_call(const std::string& key, const std::string& name template //static -void LLEventDispatcher::sCallFail(ARGS&&... args) +[[noreturn]] void LLEventDispatcher::sCallFail(ARGS&&... args) { auto error = stringize(std::forward(args)...); LL_WARNS("LLEventDispatcher") << error << LL_ENDL; @@ -716,7 +716,7 @@ void LLEventDispatcher::sCallFail(ARGS&&... args) } template -void LLEventDispatcher::callFail(ARGS&&... args) const +[[noreturn]] void LLEventDispatcher::callFail(ARGS&&... args) const { // Describe this instance in addition to the error itself. sCallFail(*this, ": ", std::forward(args)...); -- cgit v1.2.3