From e8b0088d1a0c02bfa1f9768dc91fc3df4322adae Mon Sep 17 00:00:00 2001
From: Monty Brandenberg <monty@lindenlab.com>
Date: Tue, 26 Jun 2012 12:28:58 -0400
Subject: [PATCH] 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 | 8 +-
indra/llcorehttp/_httpoperation.cpp | 3 +-
indra/llcorehttp/_httppolicy.cpp | 6 +-
indra/llcorehttp/_httprequestqueue.cpp | 11 +-
indra/llcorehttp/_httprequestqueue.h | 30 +-
indra/llcorehttp/_httpservice.cpp | 43 +--
indra/llcorehttp/_httpservice.h | 10 +-
indra/llcorehttp/_thread.h | 27 +-
indra/llcorehttp/tests/test_httprequest.hpp | 309 ++++++++++----------
9 files changed, 248 insertions(+), 199 deletions(-)
diff --git a/indra/llcorehttp/_httplibcurl.cpp b/indra/llcorehttp/_httplibcurl.cpp
index bc8b3cc9be5..39abca12c5c 100644
--- a/indra/llcorehttp/_httplibcurl.cpp
+++ b/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)
diff --git a/indra/llcorehttp/_httpoperation.cpp b/indra/llcorehttp/_httpoperation.cpp
index b35ab79d656..d80a8236e66 100644
--- a/indra/llcorehttp/_httpoperation.cpp
+++ b/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();
}
diff --git a/indra/llcorehttp/_httppolicy.cpp b/indra/llcorehttp/_httppolicy.cpp
index 93e295537c5..4350ff617bb 100644
--- a/indra/llcorehttp/_httppolicy.cpp
+++ b/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
diff --git a/indra/llcorehttp/_httprequestqueue.cpp b/indra/llcorehttp/_httprequestqueue.cpp
index 6487ef6fa5e..9acac665a9e 100644
--- a/indra/llcorehttp/_httprequestqueue.cpp
+++ b/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();
}
}
diff --git a/indra/llcorehttp/_httprequestqueue.h b/indra/llcorehttp/_httprequestqueue.h
index e11fd17c909..c9c52b72339 100644
--- a/indra/llcorehttp/_httprequestqueue.h
+++ b/indra/llcorehttp/_httprequestqueue.h
@@ -76,7 +76,7 @@ class HttpRequestQueue : public LLCoreInt::RefCounted
/// 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 @@ class HttpRequestQueue : public LLCoreInt::RefCounted
/// 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();
diff --git a/indra/llcorehttp/_httpservice.cpp b/indra/llcorehttp/_httpservice.cpp
index afbab2ab719..92c15b5b8f6 100644
--- a/indra/llcorehttp/_httpservice.cpp
+++ b/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())
diff --git a/indra/llcorehttp/_httpservice.h b/indra/llcorehttp/_httpservice.h
index a74235c475b..d67e6e95a5f 100644
--- a/indra/llcorehttp/_httpservice.h
+++ b/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 @@ class HttpService
return *mTransport;
}
+ /// Threading: callable by worker thread.
+ HttpRequestQueue & getRequestQueue()
+ {
+ return *mRequestQueue;
+ }
+
/// Threading: callable by consumer thread.
HttpPolicyGlobal & getGlobalOptions()
{
@@ -191,7 +199,7 @@ class HttpService
// === shared data ===
static volatile EState sState;
HttpRequestQueue * mRequestQueue;
- volatile bool mExitRequested;
+ LLAtomicU32 mExitRequested;
LLCoreInt::HttpThread * mThread;
// === consumer-thread-only data ===
diff --git a/indra/llcorehttp/_thread.h b/indra/llcorehttp/_thread.h
index 4cf35055e9d..e058d660e56 100644
--- a/indra/llcorehttp/_thread.h
+++ b/indra/llcorehttp/_thread.h
@@ -52,12 +52,10 @@ class HttpThread : public RefCounted
}
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 @@ class HttpThread : public RefCounted
} // 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 @@ class HttpThread : public RefCounted
mThread = new boost::thread(f);
}
- virtual ~HttpThread()
- {
- delete mThread;
- }
-
inline void join()
{
mThread->join();
diff --git a/indra/llcorehttp/tests/test_httprequest.hpp b/indra/llcorehttp/tests/test_httprequest.hpp
index ed049aa09ce..ed4e239fe79 100644
--- a/indra/llcorehttp/tests/test_httprequest.hpp
+++ b/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.
--
GitLab