From a8cded0cf6b3a69ad365d998efe99e7efa3d0b06 Mon Sep 17 00:00:00 2001 From: Aleric Inglewood Date: Fri, 15 Nov 2013 17:52:52 +0100 Subject: [PATCH 1/2] Allow calling LLNotificationsUtil::add from any thread. This makes LLStringUtil thread-safe by removing a rather unnecessary LLFastTimer from LLStringUtil::format. Same thing for LLTrans::getString and LLTrans::findString, where even a comment stated that the author wasn't interested in measuring cpu time at all. In this case I added some code back to make sure that we're not calling LLTrans::getString() in an inner loop, which was the reason that the LLFastTimer was added. Made one string static to avoid 45000 look ups during login, which kinda triggered the above test. Finally, LLNotificationsUtil::add is made thread-safe by making LLNotificationChannelBase::mItems thread-safe and defering a call to LLNotifications::updateItem to the main thread when called from another thread (using a little statemachine). --- indra/llcommon/llstring.cpp | 5 - indra/llui/llnotifications.cpp | 144 ++++++++++++++++++++++++++++- indra/llui/llnotifications.h | 9 +- indra/llui/lltrans.cpp | 31 ++++--- indra/newview/llfolderviewitem.cpp | 2 +- 5 files changed, 167 insertions(+), 24 deletions(-) diff --git a/indra/llcommon/llstring.cpp b/indra/llcommon/llstring.cpp index 8953a1089..d5bc65a63 100644 --- a/indra/llcommon/llstring.cpp +++ b/indra/llcommon/llstring.cpp @@ -36,9 +36,6 @@ #include // for WideCharToMultiByte #endif -LLFastTimer::DeclareTimer FT_STRING_FORMAT("String Format"); - - std::string ll_safe_string(const char* in) { if(in) return std::string(in); @@ -1190,7 +1187,6 @@ bool LLStringUtil::formatDatetime(std::string& replacement, std::string token, template<> S32 LLStringUtil::format(std::string& s, const format_map_t& substitutions) { - LLFastTimer ft(FT_STRING_FORMAT); S32 res = 0; std::string output; @@ -1263,7 +1259,6 @@ S32 LLStringUtil::format(std::string& s, const format_map_t& substitutions) template<> S32 LLStringUtil::format(std::string& s, const LLSD& substitutions) { - LLFastTimer ft(FT_STRING_FORMAT); S32 res = 0; if (!substitutions.isMap()) diff --git a/indra/llui/llnotifications.cpp b/indra/llui/llnotifications.cpp index b8fab0994..400d80d64 100644 --- a/indra/llui/llnotifications.cpp +++ b/indra/llui/llnotifications.cpp @@ -41,6 +41,7 @@ #include "llnotifications.h" #include "aialert.h" +#include "aistatemachine.h" #include "../newview/hippogridmanager.h" @@ -50,6 +51,9 @@ #endif #include +// Two macros, used to make access to mItems thread-safe, to keep the diff to a minimum. +#define AILOCK_mItems mItems_wat mItems_w(mItems_sf); LLNotificationSet& mItems(*mItems_w) +#define AILOCK_const_mItems mItems_crat mItems_r(mItems_sf); LLNotificationSet const& mItems(*mItems_r) const std::string NOTIFICATION_PERSIST_VERSION = "0.93"; @@ -99,6 +103,7 @@ private: output["version"] = NOTIFICATION_PERSIST_VERSION; LLSD& data = output["data"]; + AILOCK_mItems; for (LLNotificationSet::iterator it = mItems.begin(); it != mItems.end(); ++it) { if (!LLNotificationTemplates::instance().templateExists((*it)->getName())) continue; @@ -156,6 +161,7 @@ private: void onDelete(LLNotificationPtr pNotification) { // we want to keep deleted notifications in our log + AILOCK_mItems; mItems.insert(pNotification); return; @@ -757,6 +763,7 @@ void LLNotificationChannelBase::connectChanged(const LLStandardSignal::slot_type // all of the notifications that are already in the channel // we use a special signal called "load" in case the channel wants to care // only about new notifications + AILOCK_mItems; for (LLNotificationSet::iterator it = mItems.begin(); it != mItems.end(); ++it) { slot(LLSD().with("sigtype", "load").with("id", (*it)->id())); @@ -795,8 +802,11 @@ bool LLNotificationChannelBase::updateItem(const LLSD& payload) // internal call, for use in avoiding lookup bool LLNotificationChannelBase::updateItem(const LLSD& payload, LLNotificationPtr pNotification) -{ +{ + llassert(AIThreadID::in_main_thread()); + std::string cmd = payload["sigtype"]; + AILOCK_mItems; LLNotificationSet::iterator foundItem = mItems.find(pNotification); bool wasFound = (foundItem != mItems.end()); bool passesFilter = mFilter(pNotification); @@ -943,6 +953,7 @@ void LLNotificationChannel::setComparator(LLNotificationComparator comparator) { mComparator = comparator; LLNotificationSet s2(mComparator); + AILOCK_mItems; s2.insert(mItems.begin(), mItems.end()); mItems.swap(s2); @@ -952,16 +963,19 @@ void LLNotificationChannel::setComparator(LLNotificationComparator comparator) bool LLNotificationChannel::isEmpty() const { + AILOCK_const_mItems; return mItems.empty(); } LLNotificationChannel::Iterator LLNotificationChannel::begin() { + AILOCK_const_mItems; return mItems.begin(); } LLNotificationChannel::Iterator LLNotificationChannel::end() { + AILOCK_const_mItems; return mItems.end(); } @@ -1488,40 +1502,158 @@ LLNotificationPtr LLNotifications::add(AIAlert::Error const& error, int type, un return add(LLNotification::Params((type == AIAlert::modal || error.is_modal()) ? "AIAlertModal" : "AIAlert").substitutions(substitutions)); } +//-------------------------------------------------------------------------------- +// class UpdateItem +// +// Allow LLNotifications::add, LLNotifications::cancel and LLNotifications::update +// to be called from any thread. + +struct UpdateItem +{ + char const* sigtype; + LLNotificationPtr pNotif; + UpdateItem(char const* st, LLNotificationPtr const& np) : sigtype(st), pNotif(np) { } + void doit(void) const; +}; + +void UpdateItem::doit(void) const +{ + LLNotifications::getInstance()->updateItem(LLSD().with("sigtype", sigtype).with("id", pNotif->id()), pNotif); + if (!strcmp(sigtype, "delete")) + { + pNotif->cancel(); + } +} + +class UpdateItemSM : public AIStateMachine +{ + protected: + typedef AIStateMachine direct_base_type; + + enum update_item_state_type { + UpdateItem_idle = direct_base_type::max_state, + UpdateItem_doit + }; + + public: + static state_type const max_state = UpdateItem_doit + 1; + + public: + UpdateItemSM(void) : AIStateMachine(CWD_ONLY(true)) { } + + static void add(UpdateItem const& ui); + + private: + static UpdateItemSM* sSelf; + typedef std::deque updateQueue_type; + AIThreadSafeSimpleDC mUpdateQueue; + typedef AIAccess mUpdateQueue_wat; + typedef AIAccess mUpdateQueue_rat; + typedef AIAccessConst mUpdateQueue_crat; + + protected: + /*virtual*/ ~UpdateItemSM() { } + + protected: + /*virtual*/ void initialize_impl(void) { set_state(UpdateItem_idle); } + /*virtual*/ void multiplex_impl(state_type run_state); + /*virtual*/ void abort_impl(void) { } + /*virtual*/ void finish_impl(void) { } + /*virtual*/ char const* state_str_impl(state_type run_state) const; +}; + +//static +UpdateItemSM* UpdateItemSM::sSelf; + +void UpdateItemSM::add(UpdateItem const& ui) +{ + if (!sSelf) + { + sSelf = new UpdateItemSM; + sSelf->run(NULL, 0, false, true, &gMainThreadEngine); + } + if (AIThreadID::in_main_thread()) + { + ui.doit(); + return; + } + mUpdateQueue_wat mUpdateQueue_w(sSelf->mUpdateQueue); + mUpdateQueue_w->push_back(ui); + sSelf->advance_state(UpdateItem_doit); +} + +char const* UpdateItemSM::state_str_impl(state_type run_state) const +{ + switch(run_state) + { + // A complete listing of hello_world_state_type. + AI_CASE_RETURN(UpdateItem_idle); + AI_CASE_RETURN(UpdateItem_doit); + } + llassert(false); + return "UNKNOWN STATE"; +} + +void UpdateItemSM::multiplex_impl(state_type run_state) +{ + switch(run_state) + { + case UpdateItem_idle: + idle(); + break; + case UpdateItem_doit: + { + mUpdateQueue_wat mUpdateQueue_w(sSelf->mUpdateQueue); + while (!mUpdateQueue_w->empty()) + { + UpdateItem const& ui(mUpdateQueue_w->front()); + ui.doit(); + mUpdateQueue_w->pop_front(); + } + set_state(UpdateItem_idle); + break; + } + } +} + +// end of UpdateItemSM +//-------------------------------------------------------------------------------- void LLNotifications::add(const LLNotificationPtr pNotif) { if (pNotif == NULL) return; // first see if we already have it -- if so, that's a problem + AILOCK_mItems; LLNotificationSet::iterator it=mItems.find(pNotif); if (it != mItems.end()) { llerrs << "Notification added a second time to the master notification channel." << llendl; } - updateItem(LLSD().with("sigtype", "add").with("id", pNotif->id()), pNotif); + UpdateItemSM::add(UpdateItem("add", pNotif)); } void LLNotifications::cancel(LLNotificationPtr pNotif) { if (pNotif == NULL || pNotif->isCancelled()) return; + AILOCK_mItems; LLNotificationSet::iterator it=mItems.find(pNotif); if (it == mItems.end()) { llerrs << "Attempted to delete nonexistent notification " << pNotif->getName() << llendl; } - updateItem(LLSD().with("sigtype", "delete").with("id", pNotif->id()), pNotif); - pNotif->cancel(); + UpdateItemSM::add(UpdateItem("delete", pNotif)); } void LLNotifications::update(const LLNotificationPtr pNotif) { + AILOCK_mItems; LLNotificationSet::iterator it=mItems.find(pNotif); if (it != mItems.end()) { - updateItem(LLSD().with("sigtype", "change").with("id", pNotif->id()), pNotif); + UpdateItemSM::add(UpdateItem("change", pNotif)); } } @@ -1529,6 +1661,7 @@ void LLNotifications::update(const LLNotificationPtr pNotif) LLNotificationPtr LLNotifications::find(LLUUID const& uuid) { LLNotificationPtr target = LLNotificationPtr(new LLNotification(uuid)); + AILOCK_mItems; LLNotificationSet::iterator it=mItems.find(target); if (it == mItems.end()) { @@ -1543,6 +1676,7 @@ LLNotificationPtr LLNotifications::find(LLUUID const& uuid) void LLNotifications::forEachNotification(NotificationProcess process) { + AILOCK_mItems; std::for_each(mItems.begin(), mItems.end(), process); } diff --git a/indra/llui/llnotifications.h b/indra/llui/llnotifications.h index bb769cae7..df0168ae7 100644 --- a/indra/llui/llnotifications.h +++ b/indra/llui/llnotifications.h @@ -107,6 +107,7 @@ #include "llxmlnode.h" #include "llnotificationptr.h" #include "llnotificationcontext.h" +#include "aithreadsafe.h" namespace AIAlert { class Error; } @@ -196,6 +197,7 @@ class LLNotification : { LOG_CLASS(LLNotification); friend class LLNotifications; +friend class UpdateItem; public: // parameter object used to instantiate a new notification @@ -566,9 +568,10 @@ class LLNotificationChannelBase : public boost::signals2::trackable { LOG_CLASS(LLNotificationChannelBase); + friend class UpdateItem; public: LLNotificationChannelBase(LLNotificationFilter filter, LLNotificationComparator comp) : - mFilter(filter), mItems(comp) + mFilter(filter), mItems_sf(comp) {} virtual ~LLNotificationChannelBase() {} // you can also connect to a Channel, so you can be notified of @@ -582,7 +585,9 @@ public: const LLNotificationFilter& getFilter() { return mFilter; } protected: - LLNotificationSet mItems; + AIThreadSafeSimpleDC mItems_sf; + typedef AIAccess mItems_wat; + typedef AIAccessConst mItems_crat; LLStandardSignal mChanged; LLStandardSignal mPassedFilter; LLStandardSignal mFailedFilter; diff --git a/indra/llui/lltrans.cpp b/indra/llui/lltrans.cpp index fa9550055..be15bd406 100644 --- a/indra/llui/lltrans.cpp +++ b/indra/llui/lltrans.cpp @@ -91,15 +91,20 @@ bool LLTrans::parseStrings(const std::string& xml_filename, const std::setrenderUTF8(load_string, 0, right_x, y, sSearchStatusColor, LLFontGL::LEFT, LLFontGL::BOTTOM, LLFontGL::NORMAL, LLFontGL::NO_SHADOW, S32_MAX, S32_MAX, &right_x, FALSE); From 7b9f854c66ff9b67f5ff6df1053008b754957ad4 Mon Sep 17 00:00:00 2001 From: Aleric Inglewood Date: Mon, 18 Nov 2013 18:07:51 +0100 Subject: [PATCH 2/2] Fix for 'with active responder' llerrs crash. This fixes at least one case (crash report 8407), which comes down to not cleanly informing a responder of failure when the request url is empty (or so badly formed that it isn't a valid url). As a result, the statemachine would abort() without informing the responder - which is bad, sort of. The previous cases where the responder needed to be informed of a failure, namely "statemachine timed_out()" and "bad_socket()" when a socket suddenly becomes bad for unknown reason, have been replaced with the more general 'aborted()' function, which must be called before the statemachine calls abort(). Clearly this has been done for all cases of abort() now, so that if the llerrs fires again in the future then that would have to be after the statemachine calls finish(), which is still as "impossible" as it was - hence the llerrs is still there to make sure. The reason that this seldom happened on SL, and more often on opensim, even more often on home-brew test grids, seems plausible: malformed urls happen more in those cases. I also took the opportunity to improve the robustness of cases where the curl error code is checked: it makes no sense to check what curl gives as error code when an internal error occurred. --- indra/llmessage/aicurl.cpp | 31 +++++++------------ .../aicurleasyrequeststatemachine.cpp | 13 ++++---- indra/llmessage/aicurlprivate.h | 9 ++---- indra/llmessage/llhttpclient.cpp | 2 +- indra/newview/lluserauth.cpp | 5 +++ indra/newview/llxmlrpcresponder.cpp | 2 +- 6 files changed, 28 insertions(+), 34 deletions(-) diff --git a/indra/llmessage/aicurl.cpp b/indra/llmessage/aicurl.cpp index 246371aab..1364b4e38 100644 --- a/indra/llmessage/aicurl.cpp +++ b/indra/llmessage/aicurl.cpp @@ -52,7 +52,6 @@ #include "llsdserialize.h" #include "aithreadsafe.h" #include "llqueuedthread.h" -#include "lltimer.h" // ms_sleep #include "llproxy.h" #include "llhttpstatuscodes.h" #include "aihttpheaders.h" @@ -1287,8 +1286,9 @@ BufferedCurlEasyRequest::~BufferedCurlEasyRequest() // If the responder is still alive, then that means that BufferedCurlEasyRequest::processOutput was // never called, which means that the removed_from_multi_handle event never happened. // This is definitely an internal error as it can only happen when libcurl is too slow, - // in which case AICurlEasyRequestStateMachine::mTimer times out, but that already - // calls BufferedCurlEasyRequest::timed_out(). + // in which case AICurlEasyRequestStateMachine::mTimer times out, a socket goes bad, or + // the state machine is aborted, but those already call BufferedCurlEasyRequest::aborted() + // which sets mResponder to NULL. llmaybeerrs << "Calling ~BufferedCurlEasyRequest() with active responder!" << llendl; if (!LLApp::isRunning()) { @@ -1298,30 +1298,23 @@ BufferedCurlEasyRequest::~BufferedCurlEasyRequest() else { // User chose to continue. - timed_out(); + aborted(HTTP_INTERNAL_ERROR_OTHER, "BufferedCurlEasyRequest destructed with active responder"); } } --AICurlInterface::Stats::BufferedCurlEasyRequest_count; } -void BufferedCurlEasyRequest::timed_out(void) +void BufferedCurlEasyRequest::aborted(U32 http_status, std::string const& reason) { - mResponder->finished(CURLE_OK, HTTP_INTERNAL_ERROR_CURL_LOCKUP, "Request timeout, aborted.", sChannels, mOutput); - if (mResponder->needsHeaders()) + if (mResponder) { - send_buffer_events_to(NULL); // Revoke buffer events: we send them to the responder. + mResponder->finished(CURLE_OK, http_status, reason, sChannels, mOutput); + if (mResponder->needsHeaders()) + { + send_buffer_events_to(NULL); // Revoke buffer events: we send them to the responder. + } + mResponder = NULL; } - mResponder = NULL; -} - -void BufferedCurlEasyRequest::bad_socket(void) -{ - mResponder->finished(CURLE_OK, HTTP_INTERNAL_ERROR_CURL_BADSOCKET, "File descriptor went bad! Aborted.", sChannels, mOutput); - if (mResponder->needsHeaders()) - { - send_buffer_events_to(NULL); // Revoke buffer events: we send them to the responder. - } - mResponder = NULL; } #if defined(CWDEBUG) || defined(DEBUG_CURLIO) diff --git a/indra/llmessage/aicurleasyrequeststatemachine.cpp b/indra/llmessage/aicurleasyrequeststatemachine.cpp index 294f41586..6b7bcf654 100644 --- a/indra/llmessage/aicurleasyrequeststatemachine.cpp +++ b/indra/llmessage/aicurleasyrequeststatemachine.cpp @@ -121,6 +121,7 @@ void AICurlEasyRequestStateMachine::multiplex_impl(state_type run_state) bool empty_url = AICurlEasyRequest_rat(*mCurlEasyRequest)->getLowercaseServicename().empty(); if (empty_url) { + AICurlEasyRequest_wat(*mCurlEasyRequest)->aborted(HTTP_INTERNAL_ERROR_OTHER, "Not a valid URL."); abort(); break; } @@ -195,16 +196,14 @@ void AICurlEasyRequestStateMachine::multiplex_impl(state_type run_state) case AICurlEasyRequestStateMachine_removed: { // The request was removed from the multi handle. - if (mTimedOut) - { - AICurlEasyRequest_wat easy_request_w(*mCurlEasyRequest); - easy_request_w->timed_out(); - } // We're done. If we timed out, abort -- or else the application will // think that getResult() will return a valid error code from libcurl. if (mTimedOut) + { + AICurlEasyRequest_wat(*mCurlEasyRequest)->aborted(HTTP_INTERNAL_ERROR_CURL_LOCKUP, "Request timeout, aborted."); abort(); + } else finish(); @@ -212,7 +211,7 @@ void AICurlEasyRequestStateMachine::multiplex_impl(state_type run_state) } case AICurlEasyRequestStateMachine_bad_file_descriptor: { - AICurlEasyRequest_wat(*mCurlEasyRequest)->bad_socket(); + AICurlEasyRequest_wat(*mCurlEasyRequest)->aborted(HTTP_INTERNAL_ERROR_CURL_BADSOCKET, "File descriptor went bad! Aborted."); abort(); } } @@ -224,7 +223,7 @@ void AICurlEasyRequestStateMachine::abort_impl(void) // Revert call to addRequest() if that was already called (and the request wasn't removed again already). if (mAdded) { - // Note that it's safe to call this even if the curl thread already removed it, or will removes it + // Note that it's safe to call this even if the curl thread already removed it, or will remove it // after we called this, before processing the remove command; only the curl thread calls // MultiHandle::remove_easy_request, which is a no-op when called twice for the same easy request. mAdded = false; diff --git a/indra/llmessage/aicurlprivate.h b/indra/llmessage/aicurlprivate.h index c5e3514a7..ddf685aca 100644 --- a/indra/llmessage/aicurlprivate.h +++ b/indra/llmessage/aicurlprivate.h @@ -385,11 +385,8 @@ class BufferedCurlEasyRequest : public CurlEasyRequest { buffer_ptr_t& getInput(void) { return mInput; } buffer_ptr_t& getOutput(void) { return mOutput; } - // Called if libcurl doesn't deliver within AIHTTPTimeoutPolicy::mMaximumTotalDelay seconds. - void timed_out(void); - - // Called if the underlaying socket went bad (ie, when accidently closed by a buggy library). - void bad_socket(void); + // Called if the state machine is (about to be) aborted due to some error. + void aborted(U32 http_status, std::string const& reason); // Called after removed_from_multi_handle was called. void processOutput(void); @@ -462,7 +459,7 @@ class BufferedCurlEasyRequest : public CurlEasyRequest { bool success(void) const { return mResult == CURLE_OK && mStatus >= 200 && mStatus < 400; } // Return true when prepRequest was already called and the object has not been - // invalidated as a result of calling timed_out(). + // invalidated as a result of calling aborted(). bool isValid(void) const { return mResponder; } // Return the capability type of this request. diff --git a/indra/llmessage/llhttpclient.cpp b/indra/llmessage/llhttpclient.cpp index 749c5df6f..533469d93 100644 --- a/indra/llmessage/llhttpclient.cpp +++ b/indra/llmessage/llhttpclient.cpp @@ -636,7 +636,7 @@ static LLSD blocking_request( response["body"] = responder->getLLSD(); } } - else if (result == CURLE_OK) + else if (result == CURLE_OK && !is_internal_http_error(http_status)) { // We expect 404s, don't spam for them. if (http_status != 404) diff --git a/indra/newview/lluserauth.cpp b/indra/newview/lluserauth.cpp index 8c2d359a1..c2f00bb68 100644 --- a/indra/newview/lluserauth.cpp +++ b/indra/newview/lluserauth.cpp @@ -298,6 +298,11 @@ LLUserAuth::UserAuthcode LLUserAuth::authResponse() // if curl was ok, parse the download area. CURLcode result = mResponder->result_code(); + if (is_internal_http_error(mResponder->http_status())) + { + // result can be a meaningless CURLE_OK in the case of an internal error. + result = CURLE_FAILED_INIT; // Just some random error to get the default case below. + } switch (result) { case CURLE_OK: diff --git a/indra/newview/llxmlrpcresponder.cpp b/indra/newview/llxmlrpcresponder.cpp index dac69a7f1..6fbc05ca1 100644 --- a/indra/newview/llxmlrpcresponder.cpp +++ b/indra/newview/llxmlrpcresponder.cpp @@ -168,7 +168,7 @@ void XMLRPCResponder::completed_headers(U32 status, std::string const& reason, A void XMLRPCResponder::completedRaw(U32 status, std::string const& reason, LLChannelDescriptors const& channels, buffer_ptr_t const& buffer) { - if (mCode == CURLE_OK) + if (mCode == CURLE_OK && !is_internal_http_error(status)) { mBufferSize = buffer->count(channels.in()); if (200 <= status && status < 400)