Add an AITimer to AICurlEasyRequestStateMachine.

Fixes AIStateMachine to work thread-safe with the timer.

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();
 	}

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;
 	}
 

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<F32> 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();
 }
 

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.

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;

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:

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).
 	 */