From b26e516d2b93a442d09f5c3b1b4d8d60139c42f5 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Wed, 6 Apr 2022 17:34:28 -0400
Subject: [PATCH] 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 ++++++++++++------
indra/llcommon/lleventdispatcher.h | 29 ++--
.../llcommon/tests/lleventdispatcher_test.cpp | 62 ++++++---
3 files changed, 152 insertions(+), 66 deletions(-)
diff --git a/indra/llcommon/lleventdispatcher.cpp b/indra/llcommon/lleventdispatcher.cpp
index 5b6d4efbe98..742d6cf51f8 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 <memory> // 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 @@ class LL_COMMON_API LLSDArgsMapper
/// 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
diff --git a/indra/llcommon/lleventdispatcher.h b/indra/llcommon/lleventdispatcher.h
index 9e1244ef5bf..b78c77f8b3b 100644
--- a/indra/llcommon/lleventdispatcher.h
+++ b/indra/llcommon/lleventdispatcher.h
@@ -254,28 +254,28 @@ class LL_COMMON_API LLEventDispatcher
/// Unregister a callable
bool remove(const std::string& name);
- /// Call a registered callable with an explicitly-specified name. If no
- /// such callable exists, die with LL_ERRS. If the @a event fails to match
- /// the @a required prototype specified at add() time, die with LL_ERRS.
+ /// Call a registered callable with an explicitly-specified name. It is an
+ /// error if no such callable exists. It is an error if the @a event fails
+ /// to match the @a required prototype specified at add() time.
void operator()(const std::string& name, const LLSD& event) const;
/// Call a registered callable with an explicitly-specified name and
/// return <tt>true</tt>. If no such callable exists, return
- /// <tt>false</tt>. If the @a event fails to match the @a required
- /// prototype specified at add() time, die with LL_ERRS.
+ /// <tt>false</tt>. It is an error if the @a event fails to match the @a
+ /// required prototype specified at add() time.
bool 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. If no such
- /// callable exists, die with LL_ERRS. If the @a event fails to match the
- /// @a required prototype specified at add() time, die with LL_ERRS.
+ /// callable whose name is specified by that map @a key. It is an error if
+ /// no such callable exists. It is an error if the @a event fails to match
+ /// the @a required prototype specified at add() time.
void operator()(const LLSD& event) const;
/// Extract the @a key value from the incoming @a event, call the callable
/// whose name is specified by that map @a key and return <tt>true</tt>.
- /// If no such callable exists, return <tt>false</tt>. If the @a event
- /// fails to match the @a required prototype specified at add() time, die
- /// with LL_ERRS.
+ /// If no such callable exists, return <tt>false</tt>. It is an error if
+ /// the @a event fails to match the @a required prototype specified at
+ /// add() time.
bool try_call(const LLSD& event) const;
/// @name Iterate over defined names
@@ -340,6 +340,13 @@ class LL_COMMON_API LLEventDispatcher
}
}
void addFail(const std::string& name, const std::string& classname) const;
+ std::string try_call_log(const std::string& key, const std::string& name,
+ const LLSD& event) const;
+ std::string try_call(const std::string& key, const std::string& name,
+ const LLSD& event) const;
+ // Implement "it is an error" semantics for attempted call operations: if
+ // the incoming event includes a "reply" key, log and send an error reply.
+ void callFail(const LLSD& event, const std::string& msg) const;
std::string mDesc, mKey;
DispatchMap mDispatch;
diff --git a/indra/llcommon/tests/lleventdispatcher_test.cpp b/indra/llcommon/tests/lleventdispatcher_test.cpp
index 9da1ecfd67a..82a0ddf61b4 100644
--- a/indra/llcommon/tests/lleventdispatcher_test.cpp
+++ b/indra/llcommon/tests/lleventdispatcher_test.cpp
@@ -20,6 +20,7 @@
#include "../test/lltut.h"
#include "llsd.h"
#include "llsdutil.h"
+#include "llevents.h"
#include "stringize.h"
#include "tests/wrapllerrs.h"
#include "../test/catch_and_store_what_in.h"
@@ -644,12 +645,45 @@ namespace tut
outer.find(inner) != std::string::npos);
}
- void call_exc(const std::string& func, const LLSD& args, const std::string& exc_frag)
+ std::string call_exc(const std::string& func, const LLSD& args, const std::string& exc_frag)
{
- std::string threw = catch_what<std::runtime_error>([this, &func, &args](){
- work(func, args);
- });
- ensure_has(threw, exc_frag);
+ // This method was written when LLEventDispatcher responded to
+ // name or argument errors with LL_ERRS, hence the name: we used
+ // to have to intercept LL_ERRS by making it throw. Now we set up
+ // to catch an error response instead. But -- for that we need to
+ // be able to sneak a "reply" key into args, which must be a Map.
+ if (! (args.isUndefined() or args.isMap()))
+ fail(stringize("can't test call_exc() with ", args));
+ LLEventStream replypump("reply");
+ LLSD reply;
+ LLTempBoundListener bound{
+ replypump.listen(
+ "listener",
+ [&reply](const LLSD& event)
+ {
+ reply = event;
+ return false;
+ }) };
+ LLSD modargs{ args };
+ modargs["reply"] = replypump.getName();
+ if (func.empty())
+ {
+ work(modargs);
+ }
+ else
+ {
+ work(func, modargs);
+ }
+ ensure("no error response", reply.has("error"));
+ ensure_has(reply["error"], exc_frag);
+ return reply["error"];
+ }
+
+ void call_logerr(const std::string& func, const LLSD& args, const std::string& frag)
+ {
+ CaptureLog capture;
+ work(func, args);
+ capture.messageWith(frag);
}
LLSD getMetadata(const std::string& name)
@@ -1031,13 +1065,7 @@ namespace tut
{
set_test_name("call with bad name");
call_exc("freek", LLSD(), "not found");
- // We don't have a comparable helper function for the one-arg
- // operator() method, and it's not worth building one just for this
- // case. Write it out.
- std::string threw = catch_what<std::runtime_error>([this](){
- work(LLSDMap("op", "freek"));
- });
- ensure_has(threw, "bad");
+ std::string threw = call_exc("", LLSDMap("op", "freek"), "bad");
ensure_has(threw, "op");
ensure_has(threw, "freek");
}
@@ -1087,7 +1115,7 @@ namespace tut
ensure_equals("answer mismatch", tr.llsd, answer);
// Should NOT be able to pass 'answer' to Callables registered
// with 'required'.
- call_exc(tr.name_req, answer, "bad request");
+ call_logerr(tr.name_req, answer, "bad request");
// But SHOULD be able to pass 'matching' to Callables registered
// with 'required'.
work(tr.name_req, matching);
@@ -1107,11 +1135,11 @@ namespace tut
// args. We should only need to engage it for one map-style
// registration and one array-style registration.
std::string array_exc("needs an args array");
- call_exc("free0_array", 17, array_exc);
- call_exc("free0_array", LLSDMap("pi", 3.14), array_exc);
+ call_logerr("free0_array", 17, array_exc);
+ call_logerr("free0_array", LLSDMap("pi", 3.14), array_exc);
std::string map_exc("needs a map");
- call_exc("free0_map", 17, map_exc);
+ call_logerr("free0_map", 17, map_exc);
// Passing an array to a map-style function works now! No longer an
// error case!
// call_exc("free0_map", LLSDArray("a")("b"), map_exc);
@@ -1158,7 +1186,7 @@ namespace tut
{
foreach(const llsd::MapEntry& e, inMap(funcsab))
{
- call_exc(e.second, tooshort, "requires more arguments");
+ call_logerr(e.second, tooshort, "requires more arguments");
}
}
}
--
GitLab