diff options
author | Monty Brandenberg <monty@lindenlab.com> | 2012-06-26 12:28:58 -0400 |
---|---|---|
committer | Monty Brandenberg <monty@lindenlab.com> | 2012-06-26 12:28:58 -0400 |
commit | e8b0088d1a0c02bfa1f9768dc91fc3df4322adae (patch) | |
tree | 336f5484cf97d0538599250c4c405ac28866234c /indra | |
parent | e172ec84fa217aae8d1e51c1e0673322c30891fe (diff) |
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.
Diffstat (limited to 'indra')
-rw-r--r-- | indra/llcorehttp/_httplibcurl.cpp | 8 | ||||
-rw-r--r-- | indra/llcorehttp/_httpoperation.cpp | 3 | ||||
-rw-r--r-- | indra/llcorehttp/_httppolicy.cpp | 6 | ||||
-rw-r--r-- | indra/llcorehttp/_httprequestqueue.cpp | 11 | ||||
-rw-r--r-- | indra/llcorehttp/_httprequestqueue.h | 30 | ||||
-rw-r--r-- | indra/llcorehttp/_httpservice.cpp | 43 | ||||
-rw-r--r-- | indra/llcorehttp/_httpservice.h | 10 | ||||
-rw-r--r-- | indra/llcorehttp/_thread.h | 27 | ||||
-rw-r--r-- | 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 bc8b3cc9be..39abca12c5 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 b35ab79d65..d80a8236e6 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 93e295537c..4350ff617b 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 6487ef6fa5..9acac665a9 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 e11fd17c90..c9c52b7233 100644 --- a/indra/llcorehttp/_httprequestqueue.h +++ b/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(); diff --git a/indra/llcorehttp/_httpservice.cpp b/indra/llcorehttp/_httpservice.cpp index afbab2ab71..92c15b5b8f 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 a74235c475..d67e6e95a5 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 @@ 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 === diff --git a/indra/llcorehttp/_thread.h b/indra/llcorehttp/_thread.h index 4cf35055e9..e058d660e5 100644 --- a/indra/llcorehttp/_thread.h +++ b/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(); diff --git a/indra/llcorehttp/tests/test_httprequest.hpp b/indra/llcorehttp/tests/test_httprequest.hpp index ed049aa09c..ed4e239fe7 100644 --- a/indra/llcorehttp/tests/test_httprequest.hpp +++ b/indra/llcorehttp/tests/test_httprequest.hpp @@ -418,6 +418,156 @@ 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; set_test_name("HttpRequest GET to dead port + Stop execution"); @@ -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. |