DRTVWR-564: Merge branch 'pick-eventapi' into lazy-eventpump

to pick up generalized LLEventAPI add() methods and softer error handling.

indra/llcommon/lleventapi.h

@@ -64,19 +64,6 @@ class LL_COMMON_API LLEventAPI: public LLDispatchListener,
     /// Get the documentation string
     std::string getDesc() const { return mDesc; }
 
-    /**
-     * Publish only selected add() methods from LLEventDispatcher.
-     * Every LLEventAPI add() @em must have a description string.
-     */
-    template <typename CALLABLE>
-    void add(const std::string& name,
-             const std::string& desc,
-             CALLABLE callable,
-             const LLSD& required=LLSD())
-    {
-        LLEventDispatcher::add(name, desc, callable, required);
-    }
-
     /**
      * Instantiate a Response object in any LLEventAPI subclass method that
      * wants to guarantee a reply (if requested) will be sent on exit from the

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

@@ -61,7 +61,6 @@ static const auto& nil(nil_);
 #include <boost/function.hpp>
 #include <boost/bind.hpp>
 #include <boost/iterator/transform_iterator.hpp>
-#include <boost/utility/enable_if.hpp>
 #include <boost/function_types/is_nonmember_callable_builtin.hpp>
 #include <boost/function_types/parameter_types.hpp>
 #include <boost/function_types/function_arity.hpp>
@@ -167,10 +166,11 @@ class LL_COMMON_API LLEventDispatcher
      * converted to the corresponding parameter type using LLSDParam.
      */
     template<typename Function>
-    typename boost::enable_if< boost::function_types::is_nonmember_callable_builtin<Function>
-                               >::type add(const std::string& name,
-                                           const std::string& desc,
-                                           Function f);
+    typename std::enable_if<
+        boost::function_types::is_nonmember_callable_builtin<Function>::value
+        >::type add(const std::string& name,
+                    const std::string& desc,
+                    Function f);
 
     /**
      * Register a nonstatic class method with arbitrary parameters.
@@ -190,11 +190,13 @@ class LL_COMMON_API LLEventDispatcher
      * converted to the corresponding parameter type using LLSDParam.
      */
     template<typename Method, typename InstanceGetter>
-    typename boost::enable_if< boost::function_types::is_member_function_pointer<Method>
-                               >::type add(const std::string& name,
-                                           const std::string& desc,
-                                           Method f,
-                                           const InstanceGetter& getter);
+    typename std::enable_if<
+        boost::function_types::is_member_function_pointer<Method>::value &&
+        ! std::is_convertible<InstanceGetter, LLSD>::value
+        >::type add(const std::string& name,
+                    const std::string& desc,
+                    Method f,
+                    const InstanceGetter& getter);
 
     /**
      * Register a free function with arbitrary parameters. (This also works
@@ -212,12 +214,13 @@ class LL_COMMON_API LLEventDispatcher
      * to the corresponding parameter type using LLSDParam.
      */
     template<typename Function>
-    typename boost::enable_if< boost::function_types::is_nonmember_callable_builtin<Function>
-                               >::type add(const std::string& name,
-                                           const std::string& desc,
-                                           Function f,
-                                           const LLSD& params,
-                                           const LLSD& defaults=LLSD());
+    typename std::enable_if<
+        boost::function_types::is_nonmember_callable_builtin<Function>::value
+        >::type add(const std::string& name,
+                    const std::string& desc,
+                    Function f,
+                    const LLSD& params,
+                    const LLSD& defaults=LLSD());
 
     /**
      * Register a nonstatic class method with arbitrary parameters.
@@ -241,41 +244,43 @@ class LL_COMMON_API LLEventDispatcher
      * to the corresponding parameter type using LLSDParam.
      */
     template<typename Method, typename InstanceGetter>
-    typename boost::enable_if< boost::function_types::is_member_function_pointer<Method>
-                               >::type add(const std::string& name,
-                                           const std::string& desc,
-                                           Method f,
-                                           const InstanceGetter& getter,
-                                           const LLSD& params,
-                                           const LLSD& defaults=LLSD());
+    typename std::enable_if<
+        boost::function_types::is_member_function_pointer<Method>::value &&
+        ! std::is_convertible<InstanceGetter, LLSD>::value
+        >::type add(const std::string& name,
+                    const std::string& desc,
+                    Method f,
+                    const InstanceGetter& getter,
+                    const LLSD& params,
+                    const LLSD& defaults=LLSD());
 
     //@}    
 
     /// 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 +345,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;
@@ -427,7 +439,25 @@ struct LLEventDispatcher::invoker
         // Instead of grabbing the first item from argsrc and making an
         // LLSDParam of it, call getter() and pass that as the instance param.
         invoker<Function, next_iter_type, To>::apply
-        ( func, argsrc, boost::fusion::push_back(boost::fusion::nil(), boost::ref(getter())));
+        ( func, argsrc, boost::fusion::push_back(boost::fusion::nil(), bindable(getter())));
+    }
+
+    template <typename T>
+    static inline
+    auto bindable(T&& value,
+                  typename std::enable_if<std::is_pointer<T>::value, bool>::type=true)
+    {
+        // if passed a pointer, just return that pointer
+        return std::forward<T>(value);
+    }
+
+    template <typename T>
+    static inline
+    auto bindable(T&& value,
+                  typename std::enable_if<! std::is_pointer<T>::value, bool>::type=true)
+    {
+        // if passed a reference, wrap it for binding
+        return std::ref(std::forward<T>(value));
     }
 };
 
@@ -447,7 +477,7 @@ struct LLEventDispatcher::invoker<Function,To,To>
 };
 
 template<typename Function>
-typename boost::enable_if< boost::function_types::is_nonmember_callable_builtin<Function> >::type
+typename std::enable_if< boost::function_types::is_nonmember_callable_builtin<Function>::value >::type
 LLEventDispatcher::add(const std::string& name, const std::string& desc, Function f)
 {
     // Construct an invoker_function, a callable accepting const args_source&.
@@ -458,7 +488,10 @@ LLEventDispatcher::add(const std::string& name, const std::string& desc, Functio
 }
 
 template<typename Method, typename InstanceGetter>
-typename boost::enable_if< boost::function_types::is_member_function_pointer<Method> >::type
+typename std::enable_if<
+    boost::function_types::is_member_function_pointer<Method>::value &&
+    ! std::is_convertible<InstanceGetter, LLSD>::value
+>::type
 LLEventDispatcher::add(const std::string& name, const std::string& desc, Method f,
                        const InstanceGetter& getter)
 {
@@ -469,7 +502,7 @@ LLEventDispatcher::add(const std::string& name, const std::string& desc, Method
 }
 
 template<typename Function>
-typename boost::enable_if< boost::function_types::is_nonmember_callable_builtin<Function> >::type
+typename std::enable_if< boost::function_types::is_nonmember_callable_builtin<Function>::value >::type
 LLEventDispatcher::add(const std::string& name, const std::string& desc, Function f,
                        const LLSD& params, const LLSD& defaults)
 {
@@ -478,7 +511,10 @@ LLEventDispatcher::add(const std::string& name, const std::string& desc, Functio
 }
 
 template<typename Method, typename InstanceGetter>
-typename boost::enable_if< boost::function_types::is_member_function_pointer<Method> >::type
+typename std::enable_if<
+    boost::function_types::is_member_function_pointer<Method>::value &&
+    ! std::is_convertible<InstanceGetter, LLSD>::value
+>::type
 LLEventDispatcher::add(const std::string& name, const std::string& desc, Method f,
                        const InstanceGetter& getter,
                        const LLSD& params, const LLSD& defaults)

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