SH-3184/SH-3221 More work on cleanup with better unit test work and more aggressive shutdown of a thread.

Some additional work let me enable a memory check for the clean shutdown case and
generally do a better job on other interfaces.  Request queue waiters now awake
on shutdown and don't sleep once the queue is turned off.  Much better semantically
for how this will be used.

indra/llcorehttp/_httplibcurl.cpp

@@ -57,11 +57,11 @@ void HttpLibcurl::shutdown()
 {
 	while (! mActiveOps.empty())
 	{
-		active_set_t::iterator item(mActiveOps.begin());
+		HttpOpRequest * op(* mActiveOps.begin());
+		mActiveOps.erase(mActiveOps.begin());
 
-		cancelRequest(*item);
-		(*item)->release();
-		mActiveOps.erase(item);
+		cancelRequest(op);
+		op->release();
 	}
 
 	if (mMultiHandles)

indra/llcorehttp/_httpoperation.cpp

@@ -235,8 +235,9 @@ void HttpOpSpin::stageFromRequest(HttpService * service)
 	}
 	else
 	{
+		ms_sleep(1);			// backoff interlock plumbing a bit
 		this->addRef();
-		if (! HttpRequestQueue::instanceOf()->addOp(this))
+		if (! service->getRequestQueue().addOp(this))
 		{
 			this->release();
 		}

indra/llcorehttp/_httppolicy.cpp

@@ -90,20 +90,20 @@ void HttpPolicy::shutdown()
 		while (! retryq.empty())
 		{
 			HttpOpRequest * op(retryq.top());
+			retryq.pop();
 		
 			op->cancel();
 			op->release();
-			retryq.pop();
 		}
 
 		HttpReadyQueue & readyq(mState[policy_class].mReadyQueue);
 		while (! readyq.empty())
 		{
 			HttpOpRequest * op(readyq.top());
+			readyq.pop();
 		
 			op->cancel();
 			op->release();
-			readyq.pop();
 		}
 	}
 	delete [] mState;
@@ -218,7 +218,7 @@ HttpService::ELoopSpeed HttpPolicy::processReadyQueue()
 		if (! readyq.empty() || ! retryq.empty())
 		{
 			// If anything is ready, continue looping...
-			result = (std::min)(result, HttpService::NORMAL);
+			result = HttpService::NORMAL;
 		}
 	} // end foreach policy_class
 

indra/llcorehttp/_httprequestqueue.cpp

@@ -104,7 +104,7 @@ HttpOperation * HttpRequestQueue::fetchOp(bool wait)
 
 		while (mQueue.empty())
 		{
-			if (! wait)
+			if (! wait || mQueueStopped)
 				return NULL;
 			mQueueCV.wait(lock);
 		}
@@ -129,7 +129,7 @@ void HttpRequestQueue::fetchAll(bool wait, OpContainer & ops)
 
 		while (mQueue.empty())
 		{
-			if (! wait)
+			if (! wait || mQueueStopped)
 				return;
 			mQueueCV.wait(lock);
 		}
@@ -142,12 +142,19 @@ void HttpRequestQueue::fetchAll(bool wait, OpContainer & ops)
 }
 
 
+void HttpRequestQueue::wakeAll()
+{
+	mQueueCV.notify_all();
+}
+
+
 void HttpRequestQueue::stopQueue()
 {
 	{
 		HttpScopedLock lock(mQueueMutex);
 
 		mQueueStopped = true;
+		wakeAll();
 	}
 }
 

indra/llcorehttp/_httprequestqueue.h

@@ -76,7 +76,7 @@ public:
 	/// Insert an object at the back of the request queue.
 	///
 	/// Caller must provide one refcount to the queue which takes
-	/// possession of the count.
+	/// possession of the count on success.
 	///
 	/// @return			Standard status.  On failure, caller
 	///					must dispose of the operation with
@@ -85,17 +85,41 @@ public:
 	/// Threading:  callable by any thread.
 	HttpStatus addOp(HttpOperation * op);
 
-	/// Caller acquires reference count on returned operation
+	/// Return the operation on the front of the queue.  If
+	/// the queue is empty and @wait is false, call returns
+	/// immediately and a NULL pointer is returned.  If true,
+	/// caller will sleep until explicitly woken.  Wakeups
+	/// can be spurious and callers must expect NULL pointers
+	/// even if waiting is indicated.
+	///
+	/// Caller acquires reference count any returned operation
 	///
 	/// Threading:  callable by any thread.
 	HttpOperation * fetchOp(bool wait);
 
+	/// Return all queued requests to caller.  The @ops argument
+	/// should be empty when called and will be swap()'d with
+	/// current contents.  Handling of the @wait argument is
+	/// identical to @fetchOp.
+	///
 	/// Caller acquires reference count on each returned operation
 	///
 	/// Threading:  callable by any thread.
 	void fetchAll(bool wait, OpContainer & ops);
 
-	/// Disallow further request queuing
+	/// Wake any sleeping threads.  Normal queuing operations
+	/// won't require this but it may be necessary for termination
+	/// requests.
+	///
+	/// Threading:  callable by any thread.
+	void wakeAll();
+
+	/// Disallow further request queuing.  Callers to @addOp will
+	/// get a failure status (LLCORE, HE_SHUTTING_DOWN).  Callers
+	/// to @fetchAll or @fetchOp will get requests that are on the
+	/// queue but the calls will no longer wait.  Instead they'll
+	/// return immediately.  Also wakes up all sleepers to send
+	/// them on their way.
 	///
 	/// Threading:  callable by any thread.
 	void stopQueue();

indra/llcorehttp/_httpservice.cpp

@@ -48,7 +48,7 @@ volatile HttpService::EState HttpService::sState(NOT_INITIALIZED);
 
 HttpService::HttpService()
 	: mRequestQueue(NULL),
-	  mExitRequested(false),
+	  mExitRequested(0U),
 	  mThread(NULL),
 	  mPolicy(NULL),
 	  mTransport(NULL)
@@ -64,18 +64,23 @@ HttpService::HttpService()
 
 HttpService::~HttpService()
 {
-	mExitRequested = true;
+	mExitRequested = 1U;
 	if (RUNNING == sState)
 	{
 		// Trying to kill the service object with a running thread
 		// is a bit tricky.
+		if (mRequestQueue)
+		{
+			mRequestQueue->stopQueue();
+		}
+		
 		if (mThread)
 		{
-			mThread->cancel();
-			
-			if (! mThread->timedJoin(2000))
+			if (! mThread->timedJoin(250))
 			{
-				// Failed to join, expect problems ahead...
+				// Failed to join, expect problems ahead so do a hard termination.
+				mThread->cancel();
+
 				LL_WARNS("CoreHttp") << "Destroying HttpService with running thread.  Expect problems."
 									 << LL_ENDL;
 			}
@@ -120,18 +125,19 @@ void HttpService::term()
 {
 	if (sInstance)
 	{
-		if (RUNNING == sState)
+		if (RUNNING == sState && sInstance->mThread)
 		{
 			// Unclean termination.  Thread appears to be running.  We'll
 			// try to give the worker thread a chance to cancel using the
 			// exit flag...
-			sInstance->mExitRequested = true;
-
+			sInstance->mExitRequested = 1U;
+			sInstance->mRequestQueue->stopQueue();
+			
 			// And a little sleep
-			ms_sleep(1000);
-
-			// Dtor will make some additional efforts and issue any final
-			// warnings...
+			for (int i(0); i < 10 && RUNNING == sState; ++i)
+			{
+				ms_sleep(100);
+			}
 		}
 
 		delete sInstance;
@@ -170,6 +176,7 @@ bool HttpService::isStopped()
 }
 
 
+/// Threading:  callable by consumer thread *once*.
 void HttpService::startThread()
 {
 	llassert_always(! mThread || STOPPED == sState);
@@ -183,19 +190,20 @@ void HttpService::startThread()
 	// Push current policy definitions, enable policy & transport components
 	mPolicy->start(mPolicyGlobal, mPolicyClasses);
 	mTransport->start(mPolicyClasses.size());
-	
+
 	mThread = new LLCoreInt::HttpThread(boost::bind(&HttpService::threadRun, this, _1));
-	mThread->addRef();		// Need an explicit reference, implicit one is used internally
 	sState = RUNNING;
 }
 
 
+/// Threading:  callable by worker thread.
 void HttpService::stopRequested()
 {
-	mExitRequested = true;
+	mExitRequested = 1U;
 }
 
 
+/// Threading:  callable by worker thread.
 bool HttpService::changePriority(HttpHandle handle, HttpRequest::priority_t priority)
 {
 	bool found(false);
@@ -211,12 +219,13 @@ bool HttpService::changePriority(HttpHandle handle, HttpRequest::priority_t prio
 }
 
 
+/// Threading:  callable by worker thread.
 void HttpService::shutdown()
 {
 	// Disallow future enqueue of requests
 	mRequestQueue->stopQueue();
 
-	// Cancel requests alread on the request queue
+	// Cancel requests already on the request queue
 	HttpRequestQueue::OpContainer ops;
 	mRequestQueue->fetchAll(false, ops);
 	while (! ops.empty())

indra/llcorehttp/_httpservice.h

@@ -30,6 +30,8 @@
 
 #include <vector>
 
+#include "linden_common.h"
+#include "llapr.h"
 #include "httpcommon.h"
 #include "httprequest.h"
 #include "_httppolicyglobal.h"
@@ -164,6 +166,12 @@ public:
 			return *mTransport;
 		}
 
+	/// Threading:  callable by worker thread.
+	HttpRequestQueue & getRequestQueue()
+		{
+			return *mRequestQueue;
+		}
+
 	/// Threading:  callable by consumer thread.
 	HttpPolicyGlobal & getGlobalOptions()
 		{
@@ -191,7 +199,7 @@ protected:
 	// === shared data ===
 	static volatile EState				sState;
 	HttpRequestQueue *					mRequestQueue;
-	volatile bool						mExitRequested;
+	LLAtomicU32							mExitRequested;
 	LLCoreInt::HttpThread *				mThread;
 	
 	// === consumer-thread-only data ===

indra/llcorehttp/_thread.h

@@ -52,12 +52,10 @@ private:
 		}
 
 	void run()
-		{ // THREAD CONTEXT 
+		{ // THREAD CONTEXT
 
-			// The implicit reference to this object is taken for the at_exit
-			// function so that the HttpThread instance doesn't disappear out
-			// from underneath it.  Other holders of the object may want to
-			// take a reference as well.
+			// Take out additional reference for the at_exit handler
+			addRef();
 			boost::this_thread::at_thread_exit(boost::bind(&HttpThread::at_exit, this));
 
 			// run the thread function
@@ -65,13 +63,17 @@ private:
 
 		} // THREAD CONTEXT
 
+protected:
+	virtual ~HttpThread()
+		{
+			delete mThread;
+		}
+
 public:
 	/// Constructs a thread object for concurrent execution but does
-	/// not start running.  Unlike other classes that mixin RefCounted,
-	/// this does take out a reference but it is used internally for
-	/// final cleanup during at_exit processing.  Callers needing to
-	/// keep a reference must increment it themselves.
-	///
+	/// not start running.  Caller receives on refcount on the thread
+	/// instance.  If the thread is started, another will be taken
+	/// out for the exit handler.
 	explicit HttpThread(boost::function<void (HttpThread *)> threadFunc)
 		: RefCounted(true), // implicit reference
 		  mThreadFunc(threadFunc)
@@ -83,11 +85,6 @@ public:
 			mThread = new boost::thread(f);
 		}
 
-	virtual ~HttpThread()
-		{
-			delete mThread;
-		}
-
 	inline void join()
 		{
 			mThread->join();

indra/llcorehttp/tests/test_httprequest.hpp

@@ -416,6 +416,156 @@ void HttpRequestTestObjectType::test<4>()
 
 template <> template <>
 void HttpRequestTestObjectType::test<5>()
+{
+	ScopedCurlInit ready;
+	
+	set_test_name("HttpRequest Spin (soft) + NoOp + hard termination");
+
+	// Handler can be stack-allocated *if* there are no dangling
+	// references to it after completion of this method.
+	// Create before memory record as the string copy will bump numbers.
+	TestHandler2 handler(this, "handler");
+		
+	// record the total amount of dynamically allocated memory
+	mMemTotal = GetMemTotal();
+	mHandlerCalls = 0;
+
+	HttpRequest * req = NULL;
+	
+	try
+	{
+		
+		// Get singletons created
+		HttpRequest::createService();
+		
+		// Start threading early so that thread memory is invariant
+		// over the test.
+		HttpRequest::startThread();
+
+		// create a new ref counted object with an implicit reference
+		req = new HttpRequest();
+		ensure("Memory allocated on construction", mMemTotal < GetMemTotal());
+
+		// Issue a Spin
+		HttpHandle handle = req->requestSpin(1);
+		ensure("Valid handle returned for spin request", handle != LLCORE_HTTP_HANDLE_INVALID);
+
+		// Issue a NoOp
+		handle = req->requestNoOp(&handler);
+		ensure("Valid handle returned for no-op request", handle != LLCORE_HTTP_HANDLE_INVALID);
+
+		// Run the notification pump.
+		int count(0);
+		int limit(10);
+		while (count++ < limit && mHandlerCalls < 1)
+		{
+			req->update(1000);
+			usleep(100000);
+		}
+		ensure("NoOp notification received", mHandlerCalls == 1);
+
+		// release the request object
+		delete req;
+		req = NULL;
+
+		// Shut down service
+		HttpRequest::destroyService();
+
+		// Check memory usage
+		// printf("Old mem:  %d, New mem:  %d\n", mMemTotal, GetMemTotal());
+		ensure("Memory usage back to that at entry", mMemTotal == GetMemTotal());
+		// This memory test should work but could give problems as it
+		// relies on the worker thread picking up a friendly request
+		// to shutdown.  Doing so, it drops references to things and
+		// we should go back to where we started.  If it gives you
+		// problems, look into the code before commenting things out.
+	}
+	catch (...)
+	{
+		stop_thread(req);
+		delete req;
+		HttpRequest::destroyService();
+		throw;
+	}
+}
+
+
+template <> template <>
+void HttpRequestTestObjectType::test<6>()
+{
+	ScopedCurlInit ready;
+	
+	set_test_name("HttpRequest Spin + NoOp + hard termination");
+
+	// Handler can be stack-allocated *if* there are no dangling
+	// references to it after completion of this method.
+	// Create before memory record as the string copy will bump numbers.
+	TestHandler2 handler(this, "handler");
+		
+	// record the total amount of dynamically allocated memory
+	mMemTotal = GetMemTotal();
+	mHandlerCalls = 0;
+
+	HttpRequest * req = NULL;
+	
+	try
+	{
+		
+		// Get singletons created
+		HttpRequest::createService();
+		
+		// Start threading early so that thread memory is invariant
+		// over the test.
+		HttpRequest::startThread();
+
+		// create a new ref counted object with an implicit reference
+		req = new HttpRequest();
+		ensure("Memory allocated on construction", mMemTotal < GetMemTotal());
+
+		// Issue a Spin
+		HttpHandle handle = req->requestSpin(0);		// Hard spin
+		ensure("Valid handle returned for spin request", handle != LLCORE_HTTP_HANDLE_INVALID);
+
+		// Issue a NoOp
+		handle = req->requestNoOp(&handler);
+		ensure("Valid handle returned for no-op request", handle != LLCORE_HTTP_HANDLE_INVALID);
+
+		// Run the notification pump.
+		int count(0);
+		int limit(10);
+		while (count++ < limit && mHandlerCalls < 1)
+		{
+			req->update(1000);
+			usleep(100000);
+		}
+		ensure("No notifications received", mHandlerCalls == 0);
+
+		// release the request object
+		delete req;
+		req = NULL;
+
+		// Shut down service
+		HttpRequest::destroyService();
+
+		// Check memory usage
+		// printf("Old mem:  %d, New mem:  %d\n", mMemTotal, GetMemTotal());
+		// ensure("Memory usage back to that at entry", mMemTotal == GetMemTotal());
+		// This memory test won't work because we're killing the thread
+		// hard with the hard spinner.  There's no opportunity to join
+		// nicely so many things leak or get destroyed unilaterally.
+	}
+	catch (...)
+	{
+		stop_thread(req);
+		delete req;
+		HttpRequest::destroyService();
+		throw;
+	}
+}
+
+
+template <> template <>
+void HttpRequestTestObjectType::test<7>()
 {
 	ScopedCurlInit ready;
 
@@ -535,7 +685,7 @@ void HttpRequestTestObjectType::test<5>()
 
 
 template <> template <>
-void HttpRequestTestObjectType::test<6>()
+void HttpRequestTestObjectType::test<8>()
 {
 	ScopedCurlInit ready;
 
@@ -643,7 +793,7 @@ void HttpRequestTestObjectType::test<6>()
 
 
 template <> template <>
-void HttpRequestTestObjectType::test<7>()
+void HttpRequestTestObjectType::test<9>()
 {
 	ScopedCurlInit ready;
 
@@ -753,7 +903,7 @@ void HttpRequestTestObjectType::test<7>()
 
 
 template <> template <>
-void HttpRequestTestObjectType::test<8>()
+void HttpRequestTestObjectType::test<10>()
 {
 	ScopedCurlInit ready;
 
@@ -872,7 +1022,7 @@ void HttpRequestTestObjectType::test<8>()
 }
 
 template <> template <>
-void HttpRequestTestObjectType::test<9>()
+void HttpRequestTestObjectType::test<11>()
 {
 	ScopedCurlInit ready;
 
@@ -991,7 +1141,7 @@ void HttpRequestTestObjectType::test<9>()
 }
 
 template <> template <>
-void HttpRequestTestObjectType::test<10>()
+void HttpRequestTestObjectType::test<12>()
 {
 	ScopedCurlInit ready;
 
@@ -1104,7 +1254,7 @@ void HttpRequestTestObjectType::test<10>()
 
 
 template <> template <>
-void HttpRequestTestObjectType::test<11>()
+void HttpRequestTestObjectType::test<13>()
 {
 	ScopedCurlInit ready;
 
@@ -1230,153 +1380,6 @@ void HttpRequestTestObjectType::test<11>()
 	}
 }
 
-template <> template <>
-void HttpRequestTestObjectType::test<12>()
-{
-	ScopedCurlInit ready;
-	
-	set_test_name("HttpRequest Spin + NoOp + hard termination");
-
-	// Handler can be stack-allocated *if* there are no dangling
-	// references to it after completion of this method.
-	// Create before memory record as the string copy will bump numbers.
-	TestHandler2 handler(this, "handler");
-		
-	// record the total amount of dynamically allocated memory
-	mMemTotal = GetMemTotal();
-	mHandlerCalls = 0;
-
-	HttpRequest * req = NULL;
-	
-	try
-	{
-		
-		// Get singletons created
-		HttpRequest::createService();
-		
-		// Start threading early so that thread memory is invariant
-		// over the test.
-		HttpRequest::startThread();
-
-		// create a new ref counted object with an implicit reference
-		req = new HttpRequest();
-		ensure("Memory allocated on construction", mMemTotal < GetMemTotal());
-
-		// Issue a Spin
-		HttpHandle handle = req->requestSpin(0);		// Hard spin
-		ensure("Valid handle returned for spin request", handle != LLCORE_HTTP_HANDLE_INVALID);
-
-		// Issue a NoOp
-		handle = req->requestNoOp(&handler);
-		ensure("Valid handle returned for no-op request", handle != LLCORE_HTTP_HANDLE_INVALID);
-
-		// Run the notification pump.
-		int count(0);
-		int limit(10);
-		while (count++ < limit && mHandlerCalls < 1)
-		{
-			req->update(1000);
-			usleep(100000);
-		}
-		ensure("No notifications received", mHandlerCalls == 0);
-
-		// release the request object
-		delete req;
-		req = NULL;
-
-		// Shut down service
-		HttpRequest::destroyService();
-
-		// Check memory usage
-		// printf("Old mem:  %d, New mem:  %d\n", mMemTotal, GetMemTotal());
-		// ensure("Memory usage back to that at entry", mMemTotal == GetMemTotal());
-		// This memory test won't work because we're killing the thread
-		// hard with the hard spinner.  There's no opportunity to join
-		// nicely so many things leak or get destroyed unilaterally.
-	}
-	catch (...)
-	{
-		stop_thread(req);
-		delete req;
-		HttpRequest::destroyService();
-		throw;
-	}
-}
-
-template <> template <>
-void HttpRequestTestObjectType::test<13>()
-{
-	ScopedCurlInit ready;
-	
-	set_test_name("HttpRequest Spin (soft) + NoOp + hard termination");
-
-	// Handler can be stack-allocated *if* there are no dangling
-	// references to it after completion of this method.
-	// Create before memory record as the string copy will bump numbers.
-	TestHandler2 handler(this, "handler");
-		
-	// record the total amount of dynamically allocated memory
-	mMemTotal = GetMemTotal();
-	mHandlerCalls = 0;
-
-	HttpRequest * req = NULL;
-	
-	try
-	{
-		
-		// Get singletons created
-		HttpRequest::createService();
-		
-		// Start threading early so that thread memory is invariant
-		// over the test.
-		HttpRequest::startThread();
-
-		// create a new ref counted object with an implicit reference
-		req = new HttpRequest();
-		ensure("Memory allocated on construction", mMemTotal < GetMemTotal());
-
-		// Issue a Spin
-		HttpHandle handle = req->requestSpin(1);
-		ensure("Valid handle returned for spin request", handle != LLCORE_HTTP_HANDLE_INVALID);
-
-		// Issue a NoOp
-		handle = req->requestNoOp(&handler);
-		ensure("Valid handle returned for no-op request", handle != LLCORE_HTTP_HANDLE_INVALID);
-
-		// Run the notification pump.
-		int count(0);
-		int limit(10);
-		while (count++ < limit && mHandlerCalls < 1)
-		{
-			req->update(1000);
-			usleep(100000);
-		}
-		ensure("NoOp notification received", mHandlerCalls == 1);
-
-		// release the request object
-		delete req;
-		req = NULL;
-
-		// Shut down service
-		HttpRequest::destroyService();
-
-		// Check memory usage
-		// printf("Old mem:  %d, New mem:  %d\n", mMemTotal, GetMemTotal());
-		ensure("Memory usage back to that at entry", mMemTotal == GetMemTotal());
-		// This memory test should work but could give problems as it
-		// relies on the worker thread picking up a friendly request
-		// to shutdown.  Doing so, it drops references to things and
-		// we should go back to where we started.  If it gives you
-		// problems, look into the code before commenting things out.
-	}
-	catch (...)
-	{
-		stop_thread(req);
-		delete req;
-		HttpRequest::destroyService();
-		throw;
-	}
-}
 
 // *NB:  This test must be last.  The sleeping webserver
 // won't respond for a long time.