DEV-31979: Introduce LLReqID, a class to help individual event API listeners

implement the ["reqid"] convention. This convention dictates that a response
LLSD from each such API should contain a ["reqid"] key whose value echoes the
["reqid"] value, if any, in the request LLSD.
Add LLReqID support to LLAresListener's "rewriteURI" service, LLSDMessage,
LLCapabilityListener and LLXMLRPCListener.

8 files changed, 147 insertions, 25 deletions

diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp
index 7e3c6964dc..c2fa79a524 100644
--- a/indra/llcommon/llevents.cpp
+++ b/indra/llcommon/llevents.cpp
@@ -39,6 +39,8 @@
 #endif
 // other Linden headers
 #include "stringize.h"
+#include "llerror.h"
+#include "llsdutil.h"
 
 /*****************************************************************************
 *   queue_names: specify LLEventPump names that should be instantiated as
@@ -506,3 +508,26 @@ bool LLListenerOrPumpName::operator()(const LLSD& event) const
     }
     return (*mListener)(event);
 }
+
+void LLReqID::stamp(LLSD& response) const
+{
+    if (! (response.isUndefined() || response.isMap()))
+    {
+        // If 'response' was previously completely empty, it's okay to
+        // turn it into a map. If it was already a map, then it should be
+        // okay to add a key. But if it was anything else (e.g. a scalar),
+        // assigning a ["reqid"] key will DISCARD the previous value,
+        // replacing it with a map. That would be Bad.
+        LL_INFOS("LLReqID") << "stamp(" << mReqid << ") leaving non-map response unmodified: "
+                            << response << LL_ENDL;
+        return;
+    }
+    LLSD oldReqid(response["reqid"]);
+    if (! (oldReqid.isUndefined() || llsd_equals(oldReqid, mReqid)))
+    {
+        LL_INFOS("LLReqID") << "stamp(" << mReqid << ") preserving existing [\"reqid\"] value "
+                            << oldReqid << " in response: " << response << LL_ENDL;
+        return;
+    }
+    response["reqid"] = mReqid;
+}
diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h
index e84d9a50ee..73a35af035 100644
--- a/indra/llcommon/llevents.h
+++ b/indra/llcommon/llevents.h
@@ -564,6 +564,87 @@ private:
 };
 
 /*****************************************************************************
+*   LLReqID
+*****************************************************************************/
+/**
+ * This class helps the implementer of a given event API to honor the
+ * ["reqid"] convention. By this convention, each event API stamps into its
+ * response LLSD a ["reqid"] key whose value echoes the ["reqid"] value, if
+ * any, from the corresponding request.
+ *
+ * This supports an (atypical, but occasionally necessary) use case in which
+ * two or more asynchronous requests are multiplexed onto the same ["reply"]
+ * LLEventPump. Since the response events could arrive in arbitrary order, the
+ * caller must be able to demux them. It does so by matching the ["reqid"]
+ * value in each response with the ["reqid"] value in the corresponding
+ * request.
+ *
+ * It is the caller's responsibility to ensure distinct ["reqid"] values for
+ * that case. Though LLSD::UUID is guaranteed to work, it might be overkill:
+ * the "namespace" of unique ["reqid"] values is simply the set of requests
+ * specifying the same ["reply"] LLEventPump name.
+ *
+ * Making a given event API echo the request's ["reqid"] into the response is
+ * nearly trivial. This helper is mostly for mnemonic purposes, to serve as a
+ * place to put these comments. We hope that each time a coder implements a
+ * new event API based on some existing one, s/he will say, "Huh, what's an
+ * LLReqID?" and look up this material.
+ *
+ * The hardest part about the convention is deciding where to store the
+ * ["reqid"] value. Ironically, LLReqID can't help with that: you must store
+ * an LLReqID instance in whatever storage will persist until the reply is
+ * sent. For example, if the request ultimately ends up using a Responder
+ * subclass, storing an LLReqID instance in the Responder works.
+ *
+ * @note
+ * The @em implementer of an event API must honor the ["reqid"] convention.
+ * However, the @em caller of an event API need only use it if s/he is sharing
+ * the same ["reply"] LLEventPump for two or more asynchronous event API
+ * requests.
+ *
+ * In most cases, it's far easier for the caller to instantiate a local
+ * LLEventStream and pass its name to the event API in question. Then it's
+ * perfectly reasonable not to set a ["reqid"] key in the request, ignoring
+ * the @c isUndefined() ["reqid"] value in the response.
+ */
+class LLReqID
+{
+public:
+    /**
+     * If you have the request in hand at the time you instantiate the
+     * LLReqID, pass that request to extract its ["reqid"].
+     */
+    LLReqID(const LLSD& request):
+        mReqid(request["reqid"])
+    {}
+    /// If you don't yet have the request, use setFrom() later.
+    LLReqID() {}
+
+    /// Extract and store the ["reqid"] value from an incoming request.
+    void setFrom(const LLSD& request)
+    {
+        mReqid = request["reqid"];
+    }
+
+    /// Set ["reqid"] key into a pending response LLSD object.
+    void stamp(LLSD& response) const;
+
+    /// Make a whole new response LLSD object with our ["reqid"].
+    LLSD makeResponse() const
+    {
+        LLSD response;
+        stamp(response);
+        return response;
+    }
+
+    /// Not really sure of a use case for this accessor...
+    LLSD getReqID() const { return mReqid; }
+
+private:
+    LLSD mReqid;
+};
+
+/*****************************************************************************
 *   Underpinnings
 *****************************************************************************/
 /**
diff --git a/indra/llmessage/llareslistener.cpp b/indra/llmessage/llareslistener.cpp
index 8e1176cdd9..4bf375069d 100644
--- a/indra/llmessage/llareslistener.cpp
+++ b/indra/llmessage/llareslistener.cpp
@@ -24,6 +24,7 @@
 #include "llares.h"
 #include "llerror.h"
 #include "llevents.h"
+#include "llsdutil.h"
 
 LLAresListener::LLAresListener(const std::string& pumpname, LLAres* llares):
     mAres(llares),
@@ -31,6 +32,8 @@ LLAresListener::LLAresListener(const std::string& pumpname, LLAres* llares):
                    obtain(pumpname).
                    listen("LLAresListener", boost::bind(&LLAresListener::process, this, _1)))
 {
+    // Insert an entry into our mDispatch map for every method we want to be
+    // able to invoke via this event API.
     mDispatch["rewriteURI"] = boost::bind(&LLAresListener::rewriteURI, this, _1);
 }
 
@@ -60,9 +63,13 @@ bool LLAresListener::process(const LLSD& command)
 class UriRewriteResponder: public LLAres::UriRewriteResponder
 {
 public:
-    /// Specify the event pump name on which to send the reply
-    UriRewriteResponder(const std::string& pumpname):
-        mPumpName(pumpname)
+    /**
+     * Specify the request, containing the event pump name on which to send
+     * the reply.
+     */
+    UriRewriteResponder(const LLSD& request):
+        mReqID(request),
+        mPumpName(request["reply"])
     {}
 
     /// Called by base class with results. This is called in both the
@@ -76,33 +83,27 @@ public:
         {
             result.append(*ui);
         }
+        // This call knows enough to avoid trying to insert a map key into an
+        // LLSD array. It's there so that if, for any reason, we ever decide
+        // to change the response from array to map, it will Just Start Working.
+        mReqID.stamp(result);
         LLEventPumps::instance().obtain(mPumpName).post(result);
     }
 
 private:
+    LLReqID mReqID;
     const std::string mPumpName;
 };
 
 void LLAresListener::rewriteURI(const LLSD& data)
 {
-    const std::string uri(data["uri"]);
-    const std::string reply(data["reply"]);
+    static LLSD required(LLSD().insert("uri", LLSD()).insert("reply", LLSD()));
     // Validate that the request is well-formed
-    if (uri.empty() || reply.empty())
+    std::string mismatch(llsd_matches(required, data));
+    if (! mismatch.empty())
     {
-        LL_ERRS("LLAresListener") << "rewriteURI request missing";
-        std::string separator;
-        if (uri.empty())
-        {
-            LL_CONT << " 'uri'";
-            separator = " and";
-        }
-        if (reply.empty())
-        {
-            LL_CONT << separator << " 'reply'";
-        }
-        LL_CONT << LL_ENDL;
+        LL_ERRS("LLAresListener") << "bad rewriteURI request: " << mismatch << LL_ENDL;
     }
     // Looks as though we have what we need; issue the request
-    mAres->rewriteURI(uri, new UriRewriteResponder(reply));
+    mAres->rewriteURI(data["uri"], new UriRewriteResponder(data));
 }
diff --git a/indra/llmessage/llsdmessage.cpp b/indra/llmessage/llsdmessage.cpp
index f663268466..ad6b8284aa 100644
--- a/indra/llmessage/llsdmessage.cpp
+++ b/indra/llmessage/llsdmessage.cpp
@@ -68,6 +68,7 @@ bool LLSDMessage::httpListener(const LLSD& request)
     }
     LLHTTPClient::post(url, payload,
                        new LLSDMessage::EventResponder(LLEventPumps::instance(),
+                                                       request,
                                                        url, "POST", reply, error),
                        LLSD(),      // headers
                        timeout);
@@ -81,7 +82,9 @@ void LLSDMessage::EventResponder::result(const LLSD& data)
     // to the pump whose name is "".
     if (! mReplyPump.empty())
     {
-        mPumps.obtain(mReplyPump).post(data);
+        LLSD response(data);
+        mReqID.stamp(response);
+        mPumps.obtain(mReplyPump).post(response);
     }
     else                            // default success handling
     {
@@ -98,7 +101,7 @@ void LLSDMessage::EventResponder::error(U32 status, const std::string& reason, c
     // explicit pump name.
     if (! mErrorPump.empty())
     {
-        LLSD info;
+        LLSD info(mReqID.makeResponse());
         info["target"]  = mTarget;
         info["message"] = mMessage;
         info["status"]  = LLSD::Integer(status);
diff --git a/indra/llmessage/llsdmessage.h b/indra/llmessage/llsdmessage.h
index 8ae9451243..672da6d3a6 100644
--- a/indra/llmessage/llsdmessage.h
+++ b/indra/llmessage/llsdmessage.h
@@ -121,9 +121,11 @@ private:
          * (e.g. "POST") as @a message.
          */
         EventResponder(LLEventPumps& pumps,
+                       const LLSD& request,
                        const std::string& target, const std::string& message,
                        const std::string& replyPump, const std::string& errorPump):
             mPumps(pumps),
+            mReqID(request),
             mTarget(target),
             mMessage(message),
             mReplyPump(replyPump),
@@ -135,6 +137,7 @@ private:
     
     private:
         LLEventPumps& mPumps;
+        LLReqID mReqID;
         const std::string mTarget, mMessage, mReplyPump, mErrorPump;
     };
 
diff --git a/indra/llmessage/tests/llareslistener_test.cpp b/indra/llmessage/tests/llareslistener_test.cpp
index b8306d0fd9..215a3806f8 100644
--- a/indra/llmessage/tests/llareslistener_test.cpp
+++ b/indra/llmessage/tests/llareslistener_test.cpp
@@ -149,7 +149,9 @@ namespace tut
         {
             threw = e.what();
         }
-        ensure_contains("LLAresListener bad op", threw, "missing 'uri' and 'reply'");
+        ensure_contains("LLAresListener bad req", threw, "missing");
+        ensure_contains("LLAresListener bad req", threw, "reply");
+        ensure_contains("LLAresListener bad req", threw, "uri");
     }
 
     template<> template<>
@@ -169,7 +171,9 @@ namespace tut
         {
             threw = e.what();
         }
-        ensure_contains("LLAresListener bad op", threw, "missing 'uri'");
+        ensure_contains("LLAresListener bad req", threw, "missing");
+        ensure_contains("LLAresListener bad req", threw, "uri");
+        ensure_does_not_contain("LLAresListener bad req", threw, "reply");
     }
 
     template<> template<>
@@ -189,6 +193,8 @@ namespace tut
         {
             threw = e.what();
         }
-        ensure_contains("LLAresListener bad op", threw, "missing 'reply'");
+        ensure_contains("LLAresListener bad req", threw, "missing");
+        ensure_contains("LLAresListener bad req", threw, "reply");
+        ensure_does_not_contain("LLAresListener bad req", threw, "uri");
     }
 }
diff --git a/indra/newview/llcapabilitylistener.cpp b/indra/newview/llcapabilitylistener.cpp
index 3277da8930..0a41ad614e 100644
--- a/indra/newview/llcapabilitylistener.cpp
+++ b/indra/newview/llcapabilitylistener.cpp
@@ -90,6 +90,7 @@ bool LLCapabilityListener::capListener(const LLSD& request)
         // This capability is supported by the region to which we're talking.
         LLHTTPClient::post(url, payload,
                            new LLSDMessage::EventResponder(LLEventPumps::instance(),
+                                                           request,
                                                            mProvider.getDescription(),
                                                            cap, reply, error),
                            LLSD(),  // headers
diff --git a/indra/newview/llxmlrpclistener.cpp b/indra/newview/llxmlrpclistener.cpp
index 2821e6c59f..71e2427c99 100644
--- a/indra/newview/llxmlrpclistener.cpp
+++ b/indra/newview/llxmlrpclistener.cpp
@@ -217,6 +217,7 @@ public:
     /// populate an XMLRPC_REQUEST and an associated LLXMLRPCTransaction. Send
     /// the request.
     Poller(const LLSD& command):
+        mReqID(command),
         mUri(command["uri"]),
         mMethod(command["method"]),
         mReplyPump(command["reply"])
@@ -325,7 +326,7 @@ public:
             curlcode = CURLcode(curlint);
         }
 
-        LLSD data;
+        LLSD data(mReqID.makeResponse());
         data["status"] = sStatusMapper.lookup(status);
         data["errorcode"] = sCURLcodeMapper.lookup(curlcode);
         data["error"] = "";
@@ -476,6 +477,7 @@ private:
         return responses;
     }
 
+    const LLReqID mReqID;
     const std::string mUri;
     const std::string mMethod;
     const std::string mReplyPump;