From 0419f8bee948d70a6b0e9b8c8a9ed7e6f73e6828 Mon Sep 17 00:00:00 2001 From: Aleric Inglewood Date: Tue, 10 Jul 2012 05:09:08 +0200 Subject: [PATCH] Add an AITimer to AICurlEasyRequestStateMachine. Fixes AIStateMachine to work thread-safe with the timer. --- indra/llmessage/aicurlthread.cpp | 6 +- indra/newview/llxmlrpctransaction.cpp | 11 ++- .../aicurleasyrequeststatemachine.cpp | 44 ++++++--- .../aicurleasyrequeststatemachine.h | 2 + indra/newview/statemachine/aistatemachine.cpp | 90 ++++++++++++++----- indra/newview/statemachine/aistatemachine.h | 2 +- indra/newview/statemachine/aitimer.h | 1 - 7 files changed, 115 insertions(+), 41 deletions(-) diff --git a/indra/llmessage/aicurlthread.cpp b/indra/llmessage/aicurlthread.cpp index f91d49cdb..e1a9ba70a 100644 --- a/indra/llmessage/aicurlthread.cpp +++ b/indra/llmessage/aicurlthread.cpp @@ -831,11 +831,13 @@ void AICurlThread::run(void) while (ready > 0 && iter.next(fd, ev_bitmask)) { ready -= (ev_bitmask == (CURL_CSELECT_IN|CURL_CSELECT_OUT)) ? 2 : 1; + // This can cause libcurl to do callbacks and remove filedescriptors, causing us to reset their bits in the poll sets. multi_handle_w->socket_action(fd, ev_bitmask); llassert(ready >= 0); } - // Should have handled them all. - llassert(ready == 0); + // Note that ready is not necessarily 0 here, because it's possible + // that libcurl removed file descriptors which we subsequently + // didn't handle. } multi_handle_w->check_run_count(); } diff --git a/indra/newview/llxmlrpctransaction.cpp b/indra/newview/llxmlrpctransaction.cpp index 72ccfb936..aceb63e65 100644 --- a/indra/newview/llxmlrpctransaction.cpp +++ b/indra/newview/llxmlrpctransaction.cpp @@ -325,7 +325,16 @@ void LLXMLRPCTransaction::Impl::curlEasyRequestCallback(bool success) if (!success) { - setStatus(LLXMLRPCTransaction::StatusOtherError, "Statemachine failed"); + // AICurlEasyRequestStateMachine did abort. + // This currently only happens when libcurl didn't finish before the timer expired. + std::ostringstream msg; + F32 timeout_value = gSavedSettings.getF32("CurlRequestTimeOut"); + msg << "Connection to " << mURI << " timed out (" << timeout_value << " s)!"; + if (timeout_value < 40) + { + msg << "\nTry increasing CurlRequestTimeOut in Debug Settings."; + } + setStatus(LLXMLRPCTransaction::StatusOtherError, msg.str()); return; } diff --git a/indra/newview/statemachine/aicurleasyrequeststatemachine.cpp b/indra/newview/statemachine/aicurleasyrequeststatemachine.cpp index 1886c7165..48bdb09c3 100644 --- a/indra/newview/statemachine/aicurleasyrequeststatemachine.cpp +++ b/indra/newview/statemachine/aicurleasyrequeststatemachine.cpp @@ -30,11 +30,13 @@ #include "linden_common.h" #include "aicurleasyrequeststatemachine.h" +#include "llcontrol.h" enum curleasyrequeststatemachine_state_type { AICurlEasyRequestStateMachine_addRequest = AIStateMachine::max_state, AICurlEasyRequestStateMachine_waitAdded, AICurlEasyRequestStateMachine_waitRemoved, + AICurlEasyRequestStateMachine_timedOut, // Only _finished has a higher priority than _timedOut. AICurlEasyRequestStateMachine_finished }; @@ -45,6 +47,7 @@ char const* AICurlEasyRequestStateMachine::state_str_impl(state_type run_state) AI_CASE_RETURN(AICurlEasyRequestStateMachine_addRequest); AI_CASE_RETURN(AICurlEasyRequestStateMachine_waitAdded); AI_CASE_RETURN(AICurlEasyRequestStateMachine_waitRemoved); + AI_CASE_RETURN(AICurlEasyRequestStateMachine_timedOut); AI_CASE_RETURN(AICurlEasyRequestStateMachine_finished); } return "UNKNOWN STATE"; @@ -89,6 +92,15 @@ void AICurlEasyRequestStateMachine::multiplex_impl(void) // ignored when the statemachine is not idle, and theoretically the callbacks could be called // immediately after this call. mCurlEasyRequest.addRequest(); + + // Set an inactivity timer. + // This shouldn't really be necessary, except in the case of a bug + // in libcurl; but lets be sure and set a timer for inactivity. + static LLCachedControl CurlRequestTimeOut("CurlRequestTimeOut", 40.f); + mTimer = new AIPersistentTimer; // Do not delete timer upon expiration. + mTimer->setInterval(CurlRequestTimeOut); + mTimer->run(this, AICurlEasyRequestStateMachine_timedOut); + break; } case AICurlEasyRequestStateMachine_waitRemoved: @@ -96,6 +108,13 @@ void AICurlEasyRequestStateMachine::multiplex_impl(void) idle(AICurlEasyRequestStateMachine_waitRemoved); // Wait till AICurlEasyRequestStateMachine::removed_from_multi_handle() is called. break; } + case AICurlEasyRequestStateMachine_timedOut: + { + // Libcurl failed to end on error(?)... abort operation in order to free + // this curl easy handle and to notify the application that it didn't work. + abort(); + break; + } case AICurlEasyRequestStateMachine_finished: { if (mBuffered) @@ -112,16 +131,10 @@ void AICurlEasyRequestStateMachine::multiplex_impl(void) void AICurlEasyRequestStateMachine::abort_impl(void) { - Dout(dc::curl, "AICurlEasyRequestStateMachine::abort_impl called for = " << (void*)mCurlEasyRequest.get()); - // We must first revoke the events, or the curl thread might change mRunState still. + DoutEntering(dc::curl, "AICurlEasyRequestStateMachine::abort_impl() [" << (void*)this << "] [" << (void*)mCurlEasyRequest.get() << "]"); + // Revert call to addRequest() if that was already called (and the request wasn't removed already again). + if (AICurlEasyRequestStateMachine_waitAdded <= mRunState && mRunState < AICurlEasyRequestStateMachine_finished) { - AICurlEasyRequest_wat curl_easy_request_w(*mCurlEasyRequest); - curl_easy_request_w->send_events_to(NULL); - curl_easy_request_w->revokeCallbacks(); - } - if (mRunState >= AICurlEasyRequestStateMachine_waitAdded && mRunState < AICurlEasyRequestStateMachine_finished) - { - // Revert call to addRequest(). // Note that it's safe to call this even if the curl thread already removed it, or will removes 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. @@ -131,14 +144,19 @@ void AICurlEasyRequestStateMachine::abort_impl(void) void AICurlEasyRequestStateMachine::finish_impl(void) { - Dout(dc::curl, "AICurlEasyRequestStateMachine::finish_impl called for = " << (void*)mCurlEasyRequest.get()); - if (!aborted()) + DoutEntering(dc::curl, "AICurlEasyRequestStateMachine::finish_impl() [" << (void*)this << "] [" << (void*)mCurlEasyRequest.get() << "]"); + // Revoke callbacks. { - AICurlEasyRequest_wat curl_easy_request_w(*mCurlEasyRequest); + AICurlEasyRequest_wat curl_easy_request_w(*mCurlEasyRequest); curl_easy_request_w->send_events_to(NULL); curl_easy_request_w->revokeCallbacks(); } - // Auto clean up. + // Note that even if the timer expired, it wasn't deleted because we used AIPersistentTimer; so mTimer is still valid. + // Stop the timer. + mTimer->abort(); + // And delete it here. + mTimer->kill(); + // Auto clean up ourselves. kill(); } diff --git a/indra/newview/statemachine/aicurleasyrequeststatemachine.h b/indra/newview/statemachine/aicurleasyrequeststatemachine.h index 41c3c7256..4dcd3b72d 100644 --- a/indra/newview/statemachine/aicurleasyrequeststatemachine.h +++ b/indra/newview/statemachine/aicurleasyrequeststatemachine.h @@ -32,6 +32,7 @@ #define AICURLEASYREQUEST_H #include "aistatemachine.h" +#include "aitimer.h" #include "aicurl.h" // A curl easy request state machine. @@ -58,6 +59,7 @@ class AICurlEasyRequestStateMachine : public AIStateMachine, public AICurlEasyHa private: bool mBuffered; // Argument used for construction of mCurlEasyRequest. + AITimer* mTimer; // Expiration timer. protected: // AICurlEasyRequest Events. diff --git a/indra/newview/statemachine/aistatemachine.cpp b/indra/newview/statemachine/aistatemachine.cpp index faf9a75f1..5e0a5a200 100644 --- a/indra/newview/statemachine/aistatemachine.cpp +++ b/indra/newview/statemachine/aistatemachine.cpp @@ -121,7 +121,9 @@ void AIStateMachine::run(AIStateMachine* parent, state_type new_parent_state, bo // Mark that run() has been called, in case we're being called from a callback function. mState = bs_initialize; - cont(); + // Set mIdle to false and add statemachine to continued_statemachines. + mSetStateLock.lock(); + locked_cont(); } void AIStateMachine::run(callback_type::signal_type::slot_type const& slot) @@ -146,7 +148,9 @@ void AIStateMachine::run(callback_type::signal_type::slot_type const& slot) // Mark that run() has been called, in case we're being called from a callback function. mState = bs_initialize; - cont(); + // Set mIdle to false and add statemachine to continued_statemachines. + mSetStateLock.lock(); + locked_cont(); } void AIStateMachine::idle(void) @@ -163,7 +167,7 @@ void AIStateMachine::idle(void) void AIStateMachine::idle(state_type current_run_state) { - DoutEntering(dc::statemachine, "AIStateMachine::idle() [" << (void*)this << "]"); + DoutEntering(dc::statemachine, "AIStateMachine::idle(" << state_str(current_run_state) << ") [" << (void*)this << "]"); llassert(is_main_thread()); llassert(!mIdle); mSetStateLock.lock(); @@ -190,7 +194,7 @@ void AIStateMachine::idle(state_type current_run_state) // thread(s) are ignored, as if they never called cont(). void AIStateMachine::locked_cont(void) { - DoutEntering(dc::statemachine, "AIStateMachine::cont() [" << (void*)this << "]"); + DoutEntering(dc::statemachine, "AIStateMachine::locked_cont() [" << (void*)this << "]"); llassert(mIdle); // Atomic test mActive and change mIdle. mIdleActive.lock(); @@ -221,6 +225,7 @@ void AIStateMachine::locked_cont(void) // See above: it is not possible that mActive was changed since not_active // was set to true above. llassert_always(mActive == as_idle); + Dout(dc::statemachine, "Adding " << (void*)this << " to continued_statemachines"); cscm_w->continued_statemachines.push_back(this); if (!cscm_w->calling_mainloop) { @@ -236,11 +241,12 @@ void AIStateMachine::locked_cont(void) void AIStateMachine::set_state(state_type state) { DoutEntering(dc::statemachine, "AIStateMachine::set_state(" << state_str(state) << ") [" << (void*)this << "]"); - // Do not call abort(), finish() or kill() before cancelling the possibility that other threads call set_state(). + + // Stop race condition of multiple threads calling cont() or set_state() here. + mSetStateLock.lock(); + // Do not call set_state() unless running. - llassert(mState == bs_run); - // Do not call idle() when set_state is called from another thread. - llassert(!mCalledThreadUnsafeIdle || LLThread::is_main_thread()); + llassert(mState == bs_run || !LLThread::is_main_thread()); // If this function is called from another thread than the main thread, then we have to ignore // it if we're not idle and the state is less than the current state. The main thread must @@ -262,17 +268,16 @@ void AIStateMachine::set_state(state_type state) // Concurrent calls from non-main threads however, always result in the largest state // to prevail. - // Stop race condition of multiple threads calling cont() or set_state() here. - mSetStateLock.lock(); - // If the state machine is already running, and we are not the main-thread and the new // state is less than the current state, ignore it. - if (!mIdle && !LLThread::is_main_thread() && state <= mRunState) + // Also, if abort() or finish() was called, then we should just ignore it. + if (mState != bs_run || + (!mIdle && !LLThread::is_main_thread() && state <= mRunState)) { #ifdef SHOW_ASSERT // It's a bit weird if the same thread does two calls on a row where the second call // has a smaller value: warn about that. - if (mContThread == apr_os_thread_current()) + if (mState == bs_run && mContThread == apr_os_thread_current()) { llwarns << "Ignoring call to set_state(" << state_str(state) << ") by non-main thread before main-thread could react on previous call, " @@ -283,6 +288,9 @@ void AIStateMachine::set_state(state_type state) return; // Ignore. } + // Do not call idle() when set_state is called from another thread; use idle(state_type) instead. + llassert(!mCalledThreadUnsafeIdle || LLThread::is_main_thread()); + // Change mRunState to the requested value. if (mRunState != state) { @@ -305,23 +313,56 @@ void AIStateMachine::set_state(state_type state) void AIStateMachine::abort(void) { DoutEntering(dc::statemachine, "AIStateMachine::abort() [" << (void*)this << "]"); - llassert(mState == bs_run); - mState = bs_abort; - abort_impl(); - mAborted = true; - finish(); + // It's possible that abort() is called before calling AIStateMachine::multiplex. + // In that case the statemachine wasn't initialized yet and we should just kill() it. + if (LL_UNLIKELY(mState == bs_initialize)) + { + // It's ok to use the thread-unsafe idle() here, because if the statemachine + // wasn't started yet, then other threads won't call set_state() on it. + if (!mIdle) + idle(); + // run() calls locked_cont() after which the top of the mainloop adds this + // state machine to active_statemachines. Therefore, if the following fails + // then either the same statemachine called run() immediately followed by abort(), + // which is not allowed; or there were two active statemachines running, + // the first created a new statemachine and called run() on it, and then + // the other (before reaching the top of the mainloop) called abort() on + // that freshly created statemachine. Obviously, this is highly unlikely, + // but if that is the case then here we bump the statemachine into + // continued_statemachines to prevent kill() to delete this statemachine: + // the caller of abort() does not expect that. + if (LL_UNLIKELY(mActive == as_idle)) + { + mSetStateLock.lock(); + locked_cont(); + idle(); + } + kill(); + } + else + { + llassert(mState == bs_run); + mSetStateLock.lock(); + mState = bs_abort; // Causes additional calls to set_state to be ignored. + mSetStateLock.unlock(); + abort_impl(); + mAborted = true; + finish(); + } } void AIStateMachine::finish(void) { DoutEntering(dc::statemachine, "AIStateMachine::finish() [" << (void*)this << "]"); + mSetStateLock.lock(); llassert(mState == bs_run || mState == bs_abort); // It is possible that mIdle is true when abort or finish was called from // outside multiplex_impl. However, that only may be done by the main thread. llassert(!mIdle || is_main_thread()); if (!mIdle) - idle(); - mState = bs_finish; + idle(); // After calling this, we don't want other threads to call set_state() anymore. + mState = bs_finish; // Causes additional calls to set_state to be ignored. + mSetStateLock.unlock(); finish_impl(); // Did finish_impl call kill()? Then that is only the default. Remember it. bool default_delete = (mState == bs_killed); @@ -370,18 +411,21 @@ void AIStateMachine::finish(void) if (mState == bs_killed && mActive == as_idle) { // Bump the statemachine onto the active statemachine list, or else it won't be deleted. - cont(); + mSetStateLock.lock(); + locked_cont(); idle(); } } void AIStateMachine::kill(void) { + DoutEntering(dc::statemachine, "AIStateMachine::kill() [" << (void*)this << "]"); // Should only be called from finish() (or when not running (bs_initialize)). - llassert(mIdle && (mState == bs_callback || mState == bs_finish || mState == bs_initialize)); + // However, also allow multiple calls to kill() on a row (bs_killed) (which effectively don't do anything). + llassert(mIdle && (mState == bs_callback || mState == bs_finish || mState == bs_initialize || mState == bs_killed)); base_state_type prev_state = mState; mState = bs_killed; - if (prev_state == bs_initialize) + if (prev_state == bs_initialize && mActive == as_idle) { // We're not running (ie being deleted by a parent statemachine), delete it immediately. delete this; diff --git a/indra/newview/statemachine/aistatemachine.h b/indra/newview/statemachine/aistatemachine.h index 0fc71c2f4..6901f1055 100644 --- a/indra/newview/statemachine/aistatemachine.h +++ b/indra/newview/statemachine/aistatemachine.h @@ -267,7 +267,7 @@ class AIStateMachine { mSetStateLock.lock(); // Ignore calls to cont() if the statemachine isn't idle. See comments in set_state(). // Calling cont() twice or after calling set_state(), without first calling idle(), is an error. - if (!mIdle) { llassert(mContThread != apr_os_thread_current()); mSetStateLock.unlock(); return; } + if (mState != bs_run || !mIdle) { llassert(mState != bs_run || mContThread != apr_os_thread_current()); mSetStateLock.unlock(); return; } locked_cont(); } private: diff --git a/indra/newview/statemachine/aitimer.h b/indra/newview/statemachine/aitimer.h index 06d3e2ef4..c10559429 100644 --- a/indra/newview/statemachine/aitimer.h +++ b/indra/newview/statemachine/aitimer.h @@ -70,7 +70,6 @@ class AITimer : public AIStateMachine { * @brief Set the interval after which the timer should expire. * * @param interval Amount of time in seconds before the timer will expire. - * @param True if the timer should be deleted after it expires; false means it will keep firing at regular intervals. * * Call abort() at any time to stop the timer (and delete the AITimer object). */