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

@@ -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

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;

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");
             }
         }
     }