Results from code review with Nat. Consolidate some of the coroutine/http code into a single adapter.

3 files changed, 344 insertions, 139 deletions

diff --git a/indra/llmessage/llavatarnamecache.cpp b/indra/llmessage/llavatarnamecache.cpp
index 88859819e0..616bd60f07 100755
--- a/indra/llmessage/llavatarnamecache.cpp
+++ b/indra/llmessage/llavatarnamecache.cpp
@@ -189,10 +189,6 @@ namespace LLAvatarNameCache
 // further explanation.
 void LLAvatarNameCache::requestAvatarNameCache_(LLCoros::self& self, std::string url, std::vector<LLUUID> agentIds)
 {
-    LLEventStream  replyPump("NameCacheReply", true);
-    LLCoreHttpUtil::HttpCoroHandler::ptr_t httpHandler = 
-        LLCoreHttpUtil::HttpCoroHandler::ptr_t(new LLCoreHttpUtil::HttpCoroHandler(replyPump));
-
     LL_DEBUGS("AvNameCache") << "Entering coroutine " << LLCoros::instance().getName(self)
         << " with url '" << url << "', requesting " << agentIds.size() << " Agent Ids" << LL_ENDL;
 
@@ -200,12 +196,8 @@ void LLAvatarNameCache::requestAvatarNameCache_(LLCoros::self& self, std::string
     {
         bool success = true;
 
-        LLAvatarNameCache::sHttpRequest->requestGet(
-            LLAvatarNameCache::sHttpPolicy, LLAvatarNameCache::sHttpPriority, 
-            url, LLAvatarNameCache::sHttpOptions.get(), 
-            LLAvatarNameCache::sHttpHeaders.get(), httpHandler.get());
-
-        LLSD results = waitForEventOn(self, replyPump);
+        LLCoreHttpUtil::HttpCoroutineAdapter httpAdapter("NameCache", LLAvatarNameCache::sHttpPolicy);
+        LLSD results = httpAdapter.getAndYield(self, sHttpRequest, url);
         LLSD httpResults;
 
         LL_DEBUGS() << results << LL_ENDL;
@@ -564,8 +556,6 @@ void LLAvatarNameCache::idle()
 	// By convention, start running at first idle() call
 	sRunning = true;
 
-    sHttpRequest->update(0L);
-
 	// *TODO: Possibly re-enabled this based on People API load measurements
 	// 100 ms is the threshold for "user speed" operations, so we can
 	// stall for about that long to batch up requests.
diff --git a/indra/llmessage/llcorehttputil.cpp b/indra/llmessage/llcorehttputil.cpp
index 30f5f3d3ed..882fef66bc 100644
--- a/indra/llmessage/llcorehttputil.cpp
+++ b/indra/llmessage/llcorehttputil.cpp
@@ -46,148 +46,148 @@ namespace LLCoreHttpUtil
 // headers could use it.
 bool responseToLLSD(HttpResponse * response, bool log, LLSD & out_llsd)
 {
-	// Convert response to LLSD
-	BufferArray * body(response->getBody());
-	if (! body || ! body->size())
-	{
-		return false;
-	}
-
-	LLCore::BufferArrayStream bas(body);
-	LLSD body_llsd;
-	S32 parse_status(LLSDSerialize::fromXML(body_llsd, bas, log));
-	if (LLSDParser::PARSE_FAILURE == parse_status){
-		return false;
-	}
-	out_llsd = body_llsd;
-	return true;
+    // Convert response to LLSD
+    BufferArray * body(response->getBody());
+    if (!body || !body->size())
+    {
+        return false;
+    }
+
+    LLCore::BufferArrayStream bas(body);
+    LLSD body_llsd;
+    S32 parse_status(LLSDSerialize::fromXML(body_llsd, bas, log));
+    if (LLSDParser::PARSE_FAILURE == parse_status){
+        return false;
+    }
+    out_llsd = body_llsd;
+    return true;
 }
 
 
 HttpHandle requestPostWithLLSD(HttpRequest * request,
-							   HttpRequest::policy_t policy_id,
-							   HttpRequest::priority_t priority,
-							   const std::string & url,
-							   const LLSD & body,
-							   HttpOptions * options,
-							   HttpHeaders * headers,
-							   HttpHandler * handler)
+    HttpRequest::policy_t policy_id,
+    HttpRequest::priority_t priority,
+    const std::string & url,
+    const LLSD & body,
+    HttpOptions * options,
+    HttpHeaders * headers,
+    HttpHandler * handler)
 {
-	HttpHandle handle(LLCORE_HTTP_HANDLE_INVALID);
-
-	BufferArray * ba = new BufferArray();
-	BufferArrayStream bas(ba);
-	LLSDSerialize::toXML(body, bas);
-
-	handle = request->requestPost(policy_id,
-								  priority,
-								  url,
-								  ba,
-								  options,
-								  headers,
-								  handler);
-	ba->release();
-	return handle;
+    HttpHandle handle(LLCORE_HTTP_HANDLE_INVALID);
+
+    BufferArray * ba = new BufferArray();
+    BufferArrayStream bas(ba);
+    LLSDSerialize::toXML(body, bas);
+
+    handle = request->requestPost(policy_id,
+        priority,
+        url,
+        ba,
+        options,
+        headers,
+        handler);
+    ba->release();
+    return handle;
 }
 
 HttpHandle requestPostWithLLSD(HttpRequest::ptr_t & request,
-	HttpRequest::policy_t policy_id,
-	HttpRequest::priority_t priority,
-	const std::string & url,
-	const LLSD & body,
-	HttpOptions::ptr_t & options,
-	HttpHeaders::ptr_t & headers,
-	HttpHandler * handler)
+    HttpRequest::policy_t policy_id,
+    HttpRequest::priority_t priority,
+    const std::string & url,
+    const LLSD & body,
+    HttpOptions::ptr_t & options,
+    HttpHeaders::ptr_t & headers,
+    HttpHandler * handler)
 {
-	return requestPostWithLLSD(request.get(), policy_id, priority,
-		url, body, options.get(), headers.get(), handler);
+    return requestPostWithLLSD(request.get(), policy_id, priority,
+        url, body, options.get(), headers.get(), handler);
 }
 
 HttpHandle requestPutWithLLSD(HttpRequest * request,
-							   HttpRequest::policy_t policy_id,
-							   HttpRequest::priority_t priority,
-							   const std::string & url,
-							   const LLSD & body,
-							   HttpOptions * options,
-							   HttpHeaders * headers,
-							   HttpHandler * handler)
+    HttpRequest::policy_t policy_id,
+    HttpRequest::priority_t priority,
+    const std::string & url,
+    const LLSD & body,
+    HttpOptions * options,
+    HttpHeaders * headers,
+    HttpHandler * handler)
 {
-	HttpHandle handle(LLCORE_HTTP_HANDLE_INVALID);
-
-	BufferArray * ba = new BufferArray();
-	BufferArrayStream bas(ba);
-	LLSDSerialize::toXML(body, bas);
-
-	handle = request->requestPut(policy_id,
-		priority,
-		url,
-		ba,
-		options,
-		headers,
-		handler);
-	ba->release();
-	return handle;
+    HttpHandle handle(LLCORE_HTTP_HANDLE_INVALID);
+
+    BufferArray * ba = new BufferArray();
+    BufferArrayStream bas(ba);
+    LLSDSerialize::toXML(body, bas);
+
+    handle = request->requestPut(policy_id,
+        priority,
+        url,
+        ba,
+        options,
+        headers,
+        handler);
+    ba->release();
+    return handle;
 }
 
 HttpHandle requestPutWithLLSD(HttpRequest::ptr_t & request,
-	HttpRequest::policy_t policy_id,
-	HttpRequest::priority_t priority,
-	const std::string & url,
-	const LLSD & body,
-	HttpOptions::ptr_t & options,
-	HttpHeaders::ptr_t & headers,
-	HttpHandler * handler)
+    HttpRequest::policy_t policy_id,
+    HttpRequest::priority_t priority,
+    const std::string & url,
+    const LLSD & body,
+    HttpOptions::ptr_t & options,
+    HttpHeaders::ptr_t & headers,
+    HttpHandler * handler)
 {
-	return requestPutWithLLSD(request.get(), policy_id, priority,
-		url, body, options.get(), headers.get(), handler);
+    return requestPutWithLLSD(request.get(), policy_id, priority,
+        url, body, options.get(), headers.get(), handler);
 }
 
 std::string responseToString(LLCore::HttpResponse * response)
 {
-	static const std::string empty("[Empty]");
-
-	if (! response)
-	{
-		return empty;
-	}
-
-	BufferArray * body(response->getBody());
-	if (! body || ! body->size())
-	{
-		return empty;
-	}
-
-	// Attempt to parse as LLSD regardless of content-type
-	LLSD body_llsd;
-	if (responseToLLSD(response, false, body_llsd))
-	{
-		std::ostringstream tmp;
-
-		LLSDSerialize::toPrettyNotation(body_llsd, tmp);
-		std::size_t temp_len(tmp.tellp());
-		
-		if (temp_len)
-		{
-			return tmp.str().substr(0, std::min(temp_len, std::size_t(1024)));
-		}
-	}
-	else
-	{
-		// *TODO:  More elaborate forms based on Content-Type as needed.
-		char content[1024];
-
-		size_t len(body->read(0, content, sizeof(content)));
-		if (len)
-		{
-			return std::string(content, 0, len);
-		}
-	}
-
-	// Default
-	return empty;
-}
+    static const std::string empty("[Empty]");
+
+    if (!response)
+    {
+        return empty;
+    }
+
+    BufferArray * body(response->getBody());
+    if (!body || !body->size())
+    {
+        return empty;
+    }
+
+    // Attempt to parse as LLSD regardless of content-type
+    LLSD body_llsd;
+    if (responseToLLSD(response, false, body_llsd))
+    {
+        std::ostringstream tmp;
+
+        LLSDSerialize::toPrettyNotation(body_llsd, tmp);
+        std::size_t temp_len(tmp.tellp());
+
+        if (temp_len)
+        {
+            return tmp.str().substr(0, std::min(temp_len, std::size_t(1024)));
+        }
+    }
+    else
+    {
+        // *TODO:  More elaborate forms based on Content-Type as needed.
+        char content[1024];
+
+        size_t len(body->read(0, content, sizeof(content)));
+        if (len)
+        {
+            return std::string(content, 0, len);
+        }
+    }
 
+    // Default
+    return empty;
+}
 
+//========================================================================
 HttpCoroHandler::HttpCoroHandler(LLEventStream &reply) :
     mReplyPump(reply)
 {
@@ -204,7 +204,7 @@ void HttpCoroHandler::onCompleted(LLCore::HttpHandle handle, LLCore::HttpRespons
         result = LLSD::emptyMap();
         LL_WARNS()
             << "\n--------------------------------------------------------------------------\n"
-            << " Error[" << status.toULong() << "] cannot access url '" << response->getRequestURL() 
+            << " Error[" << status.toULong() << "] cannot access url '" << response->getRequestURL()
             << "' because " << status.toString()
             << "\n--------------------------------------------------------------------------"
             << LL_ENDL;
@@ -251,11 +251,12 @@ void HttpCoroHandler::buildStatusEntry(LLCore::HttpResponse *response, LLCore::H
 {
     LLSD httpresults = LLSD::emptyMap();
 
-    httpresults["success"] = static_cast<LLSD::Boolean>(status);
-    httpresults["type"] = static_cast<LLSD::Integer>(status.getType());
-    httpresults["status"] = static_cast<LLSD::Integer>(status.getStatus());
-    httpresults["message"] = static_cast<LLSD::String>(status.getMessage());
-    httpresults["url"] = static_cast<LLSD::String>(response->getRequestURL());
+    writeStatusCodes(status, response->getRequestURL(), httpresults);
+    //     httpresults["success"] = static_cast<LLSD::Boolean>(status);
+    //     httpresults["type"] = static_cast<LLSD::Integer>(status.getType());
+    //     httpresults["status"] = static_cast<LLSD::Integer>(status.getStatus());
+    //     httpresults["message"] = static_cast<LLSD::String>(status.getMessage());
+    //     httpresults["url"] = static_cast<LLSD::String>(response->getRequestURL());
 
     LLSD httpHeaders = LLSD::emptyMap();
     LLCore::HttpHeaders * hdrs = response->getHeaders();
@@ -279,7 +280,18 @@ void HttpCoroHandler::buildStatusEntry(LLCore::HttpResponse *response, LLCore::H
     result["http_result"] = httpresults;
 }
 
-HttpRequestPumper::HttpRequestPumper(const LLCore::HttpRequest::ptr_t &request):
+void HttpCoroHandler::writeStatusCodes(LLCore::HttpStatus status, const std::string &url, LLSD &result)
+{
+    result["success"] = static_cast<LLSD::Boolean>(status);
+    result["type"] = static_cast<LLSD::Integer>(status.getType());
+    result["status"] = static_cast<LLSD::Integer>(status.getStatus());
+    result["message"] = static_cast<LLSD::String>(status.getMessage());
+    result["url"] = static_cast<LLSD::String>(url);
+
+}
+
+//========================================================================
+HttpRequestPumper::HttpRequestPumper(const LLCore::HttpRequest::ptr_t &request) :
     mHttpRequest(request)
 {
     mBoundListener = LLEventPumps::instance().obtain("mainloop").
@@ -300,6 +312,164 @@ bool HttpRequestPumper::pollRequest(const LLSD&)
     return false;
 }
 
+//========================================================================
+HttpCoroutineAdapter::HttpCoroutineAdapter(const std::string &name,
+    LLCore::HttpRequest::policy_t policyId, LLCore::HttpRequest::priority_t priority) :
+    mAdapterName(name),
+    mPolicyId(policyId),
+    mPriority(priority)
+{
+}
+
+HttpCoroutineAdapter::~HttpCoroutineAdapter()
+{
+}
+
+LLSD HttpCoroutineAdapter::postAndYield(LLCoros::self & self, LLCore::HttpRequest::ptr_t &request,
+    const std::string & url, const LLSD & body,
+    LLCore::HttpOptions::ptr_t options, LLCore::HttpHeaders::ptr_t headers)
+{
+    if (!options)
+    {
+        options = LLCore::HttpOptions::ptr_t(new LLCore::HttpOptions(), false);
+    }
+
+    if (!headers)
+    {
+        headers = LLCore::HttpHeaders::ptr_t(new LLCore::HttpHeaders(), false);
+    }
+
+    LLEventStream  replyPump(mAdapterName, true);
+    LLCoreHttpUtil::HttpCoroHandler::ptr_t httpHandler =
+        LLCoreHttpUtil::HttpCoroHandler::ptr_t(new LLCoreHttpUtil::HttpCoroHandler(replyPump));
+
+    //LL_INFOS() << "Requesting transaction " << transactionId << LL_ENDL;
+    LLCoreHttpUtil::HttpRequestPumper pumper(request);
+    // The HTTPCoroHandler does not self delete, so retrieval of a the contained 
+    // pointer from the smart pointer is safe in this case.
+    LLCore::HttpHandle hhandle = requestPostWithLLSD(request,
+        mPolicyId, mPriority, url, body, options, headers,
+        httpHandler.get());
+
+    if (hhandle == LLCORE_HTTP_HANDLE_INVALID)
+    {
+        LLCore::HttpStatus status = request->getStatus();
+        LL_WARNS() << "Error posting to " << url << " Status=" << status.getStatus() <<
+            " message = " << status.getMessage() << LL_ENDL;
+
+        // Mimic the status results returned from an http error that we had 
+        // to wait on 
+        LLSD httpresults = LLSD::emptyMap();
+
+        httpHandler->writeStatusCodes(status, url, httpresults);
+
+        LLSD errorres = LLSD::emptyMap();
+        errorres["http_result"] = httpresults;
+
+        return errorres;
+    }
+
+    LLSD results = waitForEventOn(self, replyPump);
+    //LL_INFOS() << "Results for transaction " << transactionId << LL_ENDL;
+    return results;
+}
+
+LLSD HttpCoroutineAdapter::putAndYield(LLCoros::self & self, LLCore::HttpRequest::ptr_t & request,
+    const std::string & url, const LLSD & body,
+    LLCore::HttpOptions::ptr_t options, LLCore::HttpHeaders::ptr_t headers)
+{
+    if (!options)
+    {
+        options = LLCore::HttpOptions::ptr_t(new LLCore::HttpOptions(), false);
+    }
+
+    if (!headers)
+    {
+        headers = LLCore::HttpHeaders::ptr_t(new LLCore::HttpHeaders(), false);
+    }
+
+    LLEventStream  replyPump(mAdapterName, true);
+    LLCoreHttpUtil::HttpCoroHandler::ptr_t httpHandler =
+        LLCoreHttpUtil::HttpCoroHandler::ptr_t(new LLCoreHttpUtil::HttpCoroHandler(replyPump));
+
+    //LL_INFOS() << "Requesting transaction " << transactionId << LL_ENDL;
+    LLCoreHttpUtil::HttpRequestPumper pumper(request);
+    // The HTTPCoroHandler does not self delete, so retrieval of a the contained 
+    // pointer from the smart pointer is safe in this case.
+    LLCore::HttpHandle hhandle = requestPutWithLLSD(request,
+        mPolicyId, mPriority, url, body, options, headers,
+        httpHandler.get());
+
+    if (hhandle == LLCORE_HTTP_HANDLE_INVALID)
+    {
+        LLCore::HttpStatus status = request->getStatus();
+        LL_WARNS() << "Error posting to " << url << " Status=" << status.getStatus() <<
+            " message = " << status.getMessage() << LL_ENDL;
+
+        // Mimic the status results returned from an http error that we had 
+        // to wait on 
+        LLSD httpresults = LLSD::emptyMap();
+
+        httpHandler->writeStatusCodes(status, url, httpresults);
+
+        LLSD errorres = LLSD::emptyMap();
+        errorres["http_result"] = httpresults;
+
+        return errorres;
+    }
+
+    LLSD results = waitForEventOn(self, replyPump);
+    //LL_INFOS() << "Results for transaction " << transactionId << LL_ENDL;
+    return results;
+}
+
+LLSD HttpCoroutineAdapter::getAndYield(LLCoros::self & self, LLCore::HttpRequest::ptr_t & request,
+    const std::string & url,
+    LLCore::HttpOptions::ptr_t options, LLCore::HttpHeaders::ptr_t headers)
+{
+    if (!options)
+    {
+        options = LLCore::HttpOptions::ptr_t(new LLCore::HttpOptions(), false);
+    }
+
+    if (!headers)
+    {
+        headers = LLCore::HttpHeaders::ptr_t(new LLCore::HttpHeaders(), false);
+    }
+
+    LLEventStream  replyPump(mAdapterName + "Reply", true);
+    LLCoreHttpUtil::HttpCoroHandler::ptr_t httpHandler =
+        LLCoreHttpUtil::HttpCoroHandler::ptr_t(new LLCoreHttpUtil::HttpCoroHandler(replyPump));
+
+    //LL_INFOS() << "Requesting transaction " << transactionId << LL_ENDL;
+    LLCoreHttpUtil::HttpRequestPumper pumper(request);
+    // The HTTPCoroHandler does not self delete, so retrieval of a the contained 
+    // pointer from the smart pointer is safe in this case.
+    LLCore::HttpHandle hhandle = request->requestGet(mPolicyId, mPriority, 
+            url, options.get(), headers.get(), httpHandler.get());
+
+    if (hhandle == LLCORE_HTTP_HANDLE_INVALID)
+    {
+        LLCore::HttpStatus status = request->getStatus();
+        LL_WARNS() << "Error posting to " << url << " Status=" << status.getStatus() <<
+            " message = " << status.getMessage() << LL_ENDL;
+
+        // Mimic the status results returned from an http error that we had 
+        // to wait on 
+        LLSD httpresults = LLSD::emptyMap();
+
+        httpHandler->writeStatusCodes(status, url, httpresults);
+
+        LLSD errorres = LLSD::emptyMap();
+        errorres["http_result"] = httpresults;
+
+        return errorres;
+    }
+
+    LLSD results = waitForEventOn(self, replyPump);
+    //LL_INFOS() << "Results for transaction " << transactionId << LL_ENDL;
+    return results;
+}
 
 } // end namespace LLCoreHttpUtil
 
diff --git a/indra/llmessage/llcorehttputil.h b/indra/llmessage/llcorehttputil.h
index 4ae6e112b3..10f46dd477 100644
--- a/indra/llmessage/llcorehttputil.h
+++ b/indra/llmessage/llcorehttputil.h
@@ -40,6 +40,8 @@
 #include "bufferstream.h"
 #include "llsd.h"
 #include "llevents.h"
+#include "llcoros.h"
+#include "lleventcoro.h"
 
 ///
 /// The base llcorehttp library implements many HTTP idioms
@@ -174,6 +176,7 @@ public:
 
     typedef boost::shared_ptr<HttpCoroHandler> ptr_t;
 
+    void writeStatusCodes(LLCore::HttpStatus status, const std::string &url, LLSD &result);
 private:
     void buildStatusEntry(LLCore::HttpResponse *response, LLCore::HttpStatus status, LLSD &result);
 
@@ -196,7 +199,49 @@ private:
     LLCore::HttpRequest::ptr_t mHttpRequest;
 };
 
-} // end namespace LLCoreHttpUtil
+/// An adapter to handle some of the boilerplate code surrounding HTTP and coroutine 
+/// interaction.
+/// 
+/// Construct an HttpCoroutineAdapter giving it a name and policy Id. After 
+/// any application specific setup call the post, put or get method.  The request 
+/// will be automatically pumped and the method will return with an LLSD describing
+/// the result of the operation.  See HttpCoroHandler for a description of the 
+/// decoration done to the returned LLSD.
+class HttpCoroutineAdapter
+{
+public:
+    HttpCoroutineAdapter(const std::string &name, LLCore::HttpRequest::policy_t policyId, 
+            LLCore::HttpRequest::priority_t priority = 0L);
+    ~HttpCoroutineAdapter();
+
+    /// Execute a Post transaction on the supplied URL and yield execution of 
+    /// the coroutine until a result is available.
+    LLSD postAndYield(LLCoros::self & self, LLCore::HttpRequest::ptr_t & request,
+        const std::string & url, const LLSD & body, 
+        LLCore::HttpOptions::ptr_t options = LLCore::HttpOptions::ptr_t(), 
+        LLCore::HttpHeaders::ptr_t headers = LLCore::HttpHeaders::ptr_t());
+
+    /// Execute a Put transaction on the supplied URL and yield execution of 
+    /// the coroutine until a result is available.
+    LLSD putAndYield(LLCoros::self & self, LLCore::HttpRequest::ptr_t & request,
+        const std::string & url, const LLSD & body,
+        LLCore::HttpOptions::ptr_t options = LLCore::HttpOptions::ptr_t(),
+        LLCore::HttpHeaders::ptr_t headers = LLCore::HttpHeaders::ptr_t());
+
+    /// Execute a Get transaction on the supplied URL and yield execution of 
+    /// the coroutine until a result is available.
+    LLSD getAndYield(LLCoros::self & self, LLCore::HttpRequest::ptr_t & request,
+        const std::string & url,
+        LLCore::HttpOptions::ptr_t options = LLCore::HttpOptions::ptr_t(),
+        LLCore::HttpHeaders::ptr_t headers = LLCore::HttpHeaders::ptr_t());
 
+private:
+
+    std::string                     mAdapterName;
+    LLCore::HttpRequest::priority_t mPriority;
+    LLCore::HttpRequest::policy_t   mPolicyId;
+};
+
+} // end namespace LLCoreHttpUtil
 
 #endif // LL_LLCOREHTTPUTIL_H