From bbf9de9c6717f38a77a39d42d8493d275d558db9 Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Tue, 26 Aug 2014 18:33:14 -0400 Subject: Bring better error handling to inventory item and folder fetching. First, introduced some LLSD-based interfaces to the llcorehttp code using utils classes (in llcorehttputil). I've kept LLSD out of the llcorehttp library up to now and will continue to do that. Functions provide a requestPost based on LLSD body and conversion utils for HttpResponse-to-LLSD and HttpResponse-to-string conversions. Inventory fetch operations now do more thorough error checking including 200-with-error status checking. Still do retry forever on folders though I don't like that. --- indra/newview/llinventorymodel.cpp | 76 ++++++++------ indra/newview/llinventorymodelbackgroundfetch.cpp | 115 ++++++++++++++++------ 2 files changed, 130 insertions(+), 61 deletions(-) (limited to 'indra/newview') diff --git a/indra/newview/llinventorymodel.cpp b/indra/newview/llinventorymodel.cpp index 2dd31f047f..5273fb6d96 100755 --- a/indra/newview/llinventorymodel.cpp +++ b/indra/newview/llinventorymodel.cpp @@ -55,7 +55,7 @@ #include "llsdutil.h" #include "bufferarray.h" #include "bufferstream.h" -#include "llsdserialize.h" +#include "llcorehttputil.h" //#define DIFF_INVENTORY_FILES #ifdef DIFF_INVENTORY_FILES @@ -75,12 +75,6 @@ BOOL LLInventoryModel::sFirstTimeInViewer2 = TRUE; static const char CACHE_FORMAT_STRING[] = "%s.inv"; static const char * const LOG_INV("Inventory"); -static std::string dumpResponse() -{ - return std::string("ADD SOMETHING MEANINGFUL HERE"); -} - - struct InventoryIDPtrLess { bool operator()(const LLViewerInventoryCategory* i1, const LLViewerInventoryCategory* i2) const @@ -2468,18 +2462,15 @@ LLCore::HttpHandle LLInventoryModel::requestPost(bool foreground, LLCore::HttpRequest * request(foreground ? mHttpRequestFG : mHttpRequestBG); LLCore::HttpHandle handle(LLCORE_HTTP_HANDLE_INVALID); - LLCore::BufferArray * ba = new LLCore::BufferArray; - LLCore::BufferArrayStream bas(ba); - LLSDSerialize::toXML(body, bas); - handle = request->requestPost(mHttpPolicyClass, - (foreground ? mHttpPriorityFG : mHttpPriorityBG), - url, - ba, - mHttpOptions, - mHttpHeaders, - handler); - ba->release(); + handle = LLCoreHttpUtil::requestPostWithLLSD(request, + mHttpPolicyClass, + (foreground ? mHttpPriorityFG : mHttpPriorityBG), + url, + body, + mHttpOptions, + mHttpHeaders, + handler); if (LLCORE_HTTP_HANDLE_INVALID == handle) { LLCore::HttpStatus status(request->getStatus()); @@ -3981,6 +3972,7 @@ void LLInventoryModel::FetchItemHttpHandler::onCompleted(LLCore::HttpHandle hand LLCore::HttpResponse * response) { LLCore::HttpStatus status(response->getStatus()); + // status = LLCore::HttpStatus(404); // Dev tool to force error handling if (! status) { processFailure(status, response); @@ -3988,23 +3980,41 @@ void LLInventoryModel::FetchItemHttpHandler::onCompleted(LLCore::HttpHandle hand else { LLCore::BufferArray * body(response->getBody()); + // body = NULL; // Dev tool to force error handling if (! body || ! body->size()) { LL_WARNS(LOG_INV) << "Missing data in inventory item query." << LL_ENDL; processFailure("HTTP response for inventory item query missing body", response); goto only_exit; } - - LLCore::BufferArrayStream bas(body); + + // body->write(0, "Garbage Response", 16); // Dev tool to force error handling LLSD body_llsd; - S32 parse_status(LLSDSerialize::fromXML(body_llsd, bas)); - if (LLSDParser::PARSE_FAILURE == parse_status) + if (! LLCoreHttpUtil::responseToLLSD(response, true, body_llsd)) { // INFOS-level logging will occur on the parsed failure processFailure("HTTP response for inventory item query has malformed LLSD", response); goto only_exit; } + // Expect top-level structure to be a map + // body_llsd = LLSD::emptyArray(); // Dev tool to force error handling + if (! body_llsd.isMap()) + { + processFailure("LLSD response for inventory item not a map", response); + goto only_exit; + } + + // Check for 200-with-error failures + // body_llsd["error"] = LLSD::emptyMap(); // Dev tool to force error handling + // body_llsd["error"]["identifier"] = "Development"; + // body_llsd["error"]["message"] = "You left development code in the viewer"; + if (body_llsd.has("error")) + { + processFailure("Inventory application error (200-with-error)", response); + goto only_exit; + } + // Okay, process data if possible processData(body_llsd, response); } @@ -4016,12 +4026,6 @@ only_exit: void LLInventoryModel::FetchItemHttpHandler::processData(LLSD & content, LLCore::HttpResponse * response) { - if (! content.isMap()) - { - processFailure("LLSD response for inventory item not a map", response); - return; - } - start_new_inventory_observer(); #if 0 @@ -4083,7 +4087,7 @@ void LLInventoryModel::FetchItemHttpHandler::processData(LLSD & content, LLCore: { changes |= gInventory.updateItem(*it); } - // *HUH: Have computed changes, nothing uses it. + // *HUH: Have computed 'changes', nothing uses it. gInventory.notifyObservers(); gViewerWindow->getWindow()->decBusyCount(); @@ -4092,13 +4096,23 @@ void LLInventoryModel::FetchItemHttpHandler::processData(LLSD & content, LLCore: void LLInventoryModel::FetchItemHttpHandler::processFailure(LLCore::HttpStatus status, LLCore::HttpResponse * response) { - LL_WARNS(LOG_INV) << dumpResponse() << LL_ENDL; + const std::string & ct(response->getContentType()); + LL_WARNS(LOG_INV) << "Inventory item fetch failure\n" + << "[Status: " << status.toTerseString() << "]\n" + << "[Reason: " << status.toString() << "]\n" + << "[Content-type: " << ct << "]\n" + << "[Content (abridged): " + << LLCoreHttpUtil::responseToString(response) << "]" << LL_ENDL; gInventory.notifyObservers(); } void LLInventoryModel::FetchItemHttpHandler::processFailure(const char * const reason, LLCore::HttpResponse * response) { - LL_WARNS(LOG_INV) << dumpResponse() << LL_ENDL; + LL_WARNS(LOG_INV) << "Inventory item fetch failure\n" + << "[Status: internal error]\n" + << "[Reason: " << reason << "]\n" + << "[Content (abridged): " + << LLCoreHttpUtil::responseToString(response) << "]" << LL_ENDL; gInventory.notifyObservers(); } diff --git a/indra/newview/llinventorymodelbackgroundfetch.cpp b/indra/newview/llinventorymodelbackgroundfetch.cpp index 7b944edf45..0c04a9c039 100755 --- a/indra/newview/llinventorymodelbackgroundfetch.cpp +++ b/indra/newview/llinventorymodelbackgroundfetch.cpp @@ -40,17 +40,24 @@ #include "llhttpconstants.h" #include "bufferarray.h" #include "bufferstream.h" -#include "llsdserialize.h" +#include "llcorehttputil.h" namespace { +// // Http request handler class for single inventory item requests. // // We'll use a handler-per-request pattern here rather than // a shared handler. Mainly convenient as this was converted // from a Responder class model. // +// Derives from and is identical to the normal FetchItemHttpHandler +// except that: 1) it uses the background request object which is +// updated more slowly than the foreground and 2) keeps a count of +// active requests on the LLInventoryModelBackgroundFetch object +// to indicate outstanding operations are in-flight. +// class BGItemHttpHandler : public LLInventoryModel::FetchItemHttpHandler { LOG_CLASS(BGItemHttpHandler); @@ -74,6 +81,10 @@ protected: // Http request handler class for folders. +// +// Handler for FetchInventoryDescendents2 and FetchLibDescendents2 +// caps requests for folders. +// class BGFolderHttpHandler : public LLCore::HttpHandler { LOG_CLASS(BGFolderHttpHandler); @@ -116,12 +127,6 @@ const S32 MAX_FETCH_RETRIES = 10; const char * const LOG_INV("Inventory"); -std::string dumpResponse() -{ - return std::string("ADD SOMETHING MEANINGFUL HERE"); -} - - } // end of namespace anonymous @@ -471,6 +476,10 @@ void LLInventoryModelBackgroundFetch::bulkFetch() return; } + // *TODO: These values could be tweaked at runtime to effect + // a fast/slow fetch throttle. Once login is complete and the scene + // is mostly loaded, we could turn up the throttle and fill missing + // inventory more quickly. static const S32 max_concurrent_fetches(12); // Outstanding requests, not connections static const F32 new_min_time(0.5f); // *HACK: Clean this up when old code goes away entirely. static const U32 max_batch_size(10); @@ -482,7 +491,7 @@ void LLInventoryModelBackgroundFetch::bulkFetch() if (mFetchCount) { - // Process completed HTTP requests + // Process completed background HTTP requests gInventory.handleResponses(false); } @@ -497,6 +506,8 @@ void LLInventoryModelBackgroundFetch::bulkFetch() const U32 sort_order(gSavedSettings.getU32(LLInventoryPanel::DEFAULT_SORT_ORDER) & 0x1); + // *TODO: Think I'd like to get a shared pointer to this and share it + // among all the folder requests. uuid_vec_t recursive_cats; LLSD folder_request_body; @@ -673,7 +684,9 @@ bool LLInventoryModelBackgroundFetch::fetchQueueContainsNoDescendentsOf(const LL } +// =============================== // Anonymous Namespace Definitions +// =============================== namespace { @@ -683,30 +696,53 @@ namespace void BGFolderHttpHandler::onCompleted(LLCore::HttpHandle handle, LLCore::HttpResponse * response) { LLCore::HttpStatus status(response->getStatus()); + // status = LLCore::HttpStatus(404); // Dev tool to force error handling if (! status) { processFailure(status, response); } else { + // Response body should be present. LLCore::BufferArray * body(response->getBody()); + // body = NULL; // Dev tool to force error handling if (! body || ! body->size()) { LL_WARNS(LOG_INV) << "Missing data in inventory folder query." << LL_ENDL; processFailure("HTTP response missing expected body", response); goto only_exit; } - - LLCore::BufferArrayStream bas(body); + + // Could test 'Content-Type' header but probably unreliable. + + // Convert response to LLSD + // body->write(0, "Garbage Response", 16); // Dev tool to force error handling LLSD body_llsd; - S32 parse_status(LLSDSerialize::fromXML(body_llsd, bas)); - if (LLSDParser::PARSE_FAILURE == parse_status) + if (! LLCoreHttpUtil::responseToLLSD(response, true, body_llsd)) { // INFOS-level logging will occur on the parsed failure processFailure("HTTP response contained malformed LLSD", response); goto only_exit; } + // Expect top-level structure to be a map + // body_llsd = LLSD::emptyArray(); // Dev tool to force error handling + if (! body_llsd.isMap()) + { + processFailure("LLSD response not a map", response); + goto only_exit; + } + + // Check for 200-with-error failures + // body_llsd["error"] = LLSD::emptyMap(); // Dev tool to force error handling + // body_llsd["error"]["identifier"] = "Development"; + // body_llsd["error"]["message"] = "You left development code in the viewer"; + if (body_llsd.has("error")) + { + processFailure("Inventory application error (200-with-error)", response); + goto only_exit; + } + // Okay, process data if possible processData(body_llsd, response); } @@ -719,13 +755,12 @@ only_exit: void BGFolderHttpHandler::processData(LLSD & content, LLCore::HttpResponse * response) { - if (! content.isMap()) - { - processFailure("LLSD response not a map", response); - return; - } - LLInventoryModelBackgroundFetch * fetcher(LLInventoryModelBackgroundFetch::getInstance()); + + // API V2 and earlier should probably be testing for "error" map + // in response as an application-level error. + + // Instead, we assume success and attempt to extract information. if (content.has("folders")) { LLSD folders(content["folders"]); @@ -836,7 +871,7 @@ void BGFolderHttpHandler::processData(LLSD & content, LLCore::HttpResponse * res folder_it != bad_folders.endArray(); ++folder_it) { - // *TODO: Stop copying data + // *TODO: Stop copying data [ed: this isn't copying data] LLSD folder_sd(*folder_it); // These folders failed on the dataserver. We probably don't want to retry them. @@ -857,14 +892,26 @@ void BGFolderHttpHandler::processData(LLSD & content, LLCore::HttpResponse * res void BGFolderHttpHandler::processFailure(LLCore::HttpStatus status, LLCore::HttpResponse * response) { - LL_WARNS(LOG_INV) << dumpResponse() << LL_ENDL; + const std::string & ct(response->getContentType()); + LL_WARNS(LOG_INV) << "Inventory folder fetch failure\n" + << "[Status: " << status.toTerseString() << "]\n" + << "[Reason: " << status.toString() << "]\n" + << "[Content-type: " << ct << "]\n" + << "[Content (abridged): " + << LLCoreHttpUtil::responseToString(response) << "]" << LL_ENDL; + + // Could use a 404 test here to try to detect revoked caps... + + // This was originally the request retry logic for the inventory + // request which tested on HTTP_INTERNAL_ERROR status. This + // retry logic was unbounded and lacked discrimination as to the + // cause of the retry. The new http library should be doing + // adquately on retries but I want to keep the structure of a + // retry for reference. LLInventoryModelBackgroundFetch *fetcher = LLInventoryModelBackgroundFetch::getInstance(); - - LL_INFOS(LOG_INV) << dumpResponse() << LL_ENDL; - - // *FIX: Not the correct test here... - if (status == LLCore::HttpStatus(408)) // timed out or curl failure + if (false) { + // timed out or curl failure for (LLSD::array_const_iterator folder_it = mRequestSD["folders"].beginArray(); folder_it != mRequestSD["folders"].endArray(); ++folder_it) @@ -888,12 +935,20 @@ void BGFolderHttpHandler::processFailure(LLCore::HttpStatus status, LLCore::Http void BGFolderHttpHandler::processFailure(const char * const reason, LLCore::HttpResponse * response) { - LL_WARNS(LOG_INV) << dumpResponse() << LL_ENDL; + LL_WARNS(LOG_INV) << "Inventory folder fetch failure\n" + << "[Status: internal error]\n" + << "[Reason: " << reason << "]\n" + << "[Content (abridged): " + << LLCoreHttpUtil::responseToString(response) << "]" << LL_ENDL; + + // Reverse of previous processFailure() method, this is invoked + // when response structure is found to be invalid. Original + // always re-issued the request (without limit). This does + // the same but be aware that this may be a source of problems. + // Philosophy is that inventory folders are so essential to + // operation that this is a reasonable action. LLInventoryModelBackgroundFetch *fetcher = LLInventoryModelBackgroundFetch::getInstance(); - - LL_INFOS(LOG_INV) << dumpResponse() << LL_ENDL; - - if (true /* status == LLCore::HttpStatus(408)*/) // timed out or curl failure + if (true) { for (LLSD::array_const_iterator folder_it = mRequestSD["folders"].beginArray(); folder_it != mRequestSD["folders"].endArray(); -- cgit v1.2.3