From 9deb3e433cff98e63f46ff206d17aaaf2e8e5045 Mon Sep 17 00:00:00 2001 From: Aleric Inglewood Date: Mon, 16 Jul 2012 22:35:04 +0200 Subject: [PATCH] LLCurlRequest time out fixes. Also some more cleanup on exit improvements. --- indra/llmessage/aicurl.cpp | 4 ++-- indra/llmessage/aicurlprivate.h | 12 ++++++++++++ indra/llmessage/aicurlthread.cpp | 7 +++++++ indra/llmessage/lliopipe.h | 2 +- indra/llmessage/llpumpio.cpp | 31 ++++++++++++++++++------------- indra/llmessage/llurlrequest.cpp | 18 ++++++++++++++++-- 6 files changed, 56 insertions(+), 18 deletions(-) diff --git a/indra/llmessage/aicurl.cpp b/indra/llmessage/aicurl.cpp index 4636fae7c..85e184ef5 100644 --- a/indra/llmessage/aicurl.cpp +++ b/indra/llmessage/aicurl.cpp @@ -554,7 +554,7 @@ void CurlEasyHandle::handle_easy_error(CURLcode code) } // Throws AICurlNoEasyHandle. -CurlEasyHandle::CurlEasyHandle(void) : mActiveMultiHandle(NULL), mErrorBuffer(NULL) +CurlEasyHandle::CurlEasyHandle(void) : mActiveMultiHandle(NULL), mErrorBuffer(NULL), mQueuedForRemoval(false) #ifdef SHOW_ASSERT , mRemovedPerCommand(true) #endif @@ -815,7 +815,7 @@ void CurlEasyRequest::revokeCallbacks(void) mWriteCallback = &noWriteCallback; mReadCallback = &noReadCallback; mSSLCtxCallback = &noSSLCtxCallback; - if (active() && !LLApp::isExiting()) + if (active() && !no_warning()) { llwarns << "Revoking callbacks on a still active CurlEasyRequest object!" << llendl; } diff --git a/indra/llmessage/aicurlprivate.h b/indra/llmessage/aicurlprivate.h index 36a8b3f23..38ce00b5c 100644 --- a/indra/llmessage/aicurlprivate.h +++ b/indra/llmessage/aicurlprivate.h @@ -98,10 +98,17 @@ class CurlEasyHandle : public boost::noncopyable, protected AICurlEasyHandleEven // Pause and unpause a connection. CURLcode pause(int bitmask); + // Called when a request is queued for removal. In that case a race between the actual removal + // and revoking of the callbacks is harmless (and happens for the raw non-statemachine version). + void remove_queued(void) { mQueuedForRemoval = true; } + // In case it's added after being removed. + void add_queued(void) { mQueuedForRemoval = false; } + private: CURL* mEasyHandle; CURLM* mActiveMultiHandle; char* mErrorBuffer; + bool mQueuedForRemoval; // Set if the easy handle is (probably) added to the multi handle, but is queued for removal. #ifdef SHOW_ASSERT public: bool mRemovedPerCommand; // Set if mActiveMultiHandle was reset as per command from the main thread. @@ -121,6 +128,11 @@ class CurlEasyHandle : public boost::noncopyable, protected AICurlEasyHandleEven // Only valid when setErrorBuffer was called and the curl_easy function returned an error. std::string getErrorString(void) const { return mErrorBuffer ? mErrorBuffer : "(null)"; } + // Returns true when it is expected that the parent will revoke callbacks before the curl + // easy handle is removed from the multi handle; that usually happens when an external + // error demands termination of the request (ie, an expiration). + bool no_warning(void) const { return mQueuedForRemoval || LLApp::isExiting(); } + // Used for debugging purposes. bool operator==(CURL* easy_handle) const { return mEasyHandle == easy_handle; } diff --git a/indra/llmessage/aicurlthread.cpp b/indra/llmessage/aicurlthread.cpp index 3c1ce5999..039253d6e 100644 --- a/indra/llmessage/aicurlthread.cpp +++ b/indra/llmessage/aicurlthread.cpp @@ -617,6 +617,7 @@ void AICurlThread::cleanup_wakeup_fds(void) void AICurlThread::wakeup_thread(void) { DoutEntering(dc::curl, "AICurlThread::wakeup_thread"); + llassert(is_main_thread()); #ifdef WINDOWS #error Missing implementation @@ -1084,6 +1085,9 @@ void stopCurlThread(void) ms_sleep(10); } Dout(dc::curl, "Curl thread" << (curlThreadIsRunning() ? " not" : "") << " stopped after " << ((100 - count) * 10) << "ms."); + // Clear the command queue, for a cleaner cleanup. + command_queue_wat command_queue_w(command_queue); + command_queue_w->clear(); } } @@ -1136,6 +1140,7 @@ void AICurlEasyRequest::addRequest(void) #endif // Add a command to add the new request to the multi session to the command queue. command_queue_w->push_back(Command(*this, cmd_add)); + AICurlEasyRequest_wat(*get())->add_queued(); } // Something was added to the queue, wake up the thread to get it. wakeUpCurlThread(); @@ -1188,6 +1193,8 @@ void AICurlEasyRequest::removeRequest(void) #endif // Add a command to remove this request from the multi session to the command queue. command_queue_w->push_back(Command(*this, cmd_remove)); + // Suppress warning that would otherwise happen if the callbacks are revoked before the curl thread removed the request. + AICurlEasyRequest_wat(*get())->remove_queued(); } // Something was added to the queue, wake up the thread to get it. wakeUpCurlThread(); diff --git a/indra/llmessage/lliopipe.h b/indra/llmessage/lliopipe.h index 86ae7e425..0e71d7a00 100644 --- a/indra/llmessage/lliopipe.h +++ b/indra/llmessage/lliopipe.h @@ -149,7 +149,7 @@ public: // The connection was lost. STATUS_LOST_CONNECTION = -5, - // The totoal process time has exceeded the timeout. + // The total process time has exceeded the timeout. STATUS_EXPIRED = -6, // Keep track of the count of codes here. diff --git a/indra/llmessage/llpumpio.cpp b/indra/llmessage/llpumpio.cpp index c249d7132..e6c6a42d6 100644 --- a/indra/llmessage/llpumpio.cpp +++ b/indra/llmessage/llpumpio.cpp @@ -207,14 +207,6 @@ bool LLPumpIO::addChain(chain_t const& chain, F32 timeout) if (it == end) return false; LLChainInfo info; - for(; it != end; ++it) - { - if ((*it)->hasExpiration()) - { - info.mHasExpiration = true; - break; - } - } info.setTimeoutSeconds(timeout); info.mData = LLIOPipe::buffer_ptr_t(new LLBufferArray); LLLinkInfo link; @@ -224,13 +216,14 @@ bool LLPumpIO::addChain(chain_t const& chain, F32 timeout) #else lldebugs << "LLPumpIO::addChain() " << chain[0] <hasExpiration(); link.mPipe = (*it); link.mChannels = info.mData->nextChannel(); info.mChainLinks.push_back(link); } + #if LL_THREADS_APR LLScopedLock lock(mChainsMutex); #endif @@ -250,11 +243,10 @@ bool LLPumpIO::addChain( // description, we need to have that description matched to a // particular buffer. if(!data) return false; - if(links.empty()) return false; + links_t::const_iterator link = links.begin(); + links_t::const_iterator const end = links.end(); + if (link == end) return false; -#if LL_THREADS_APR - LLScopedLock lock(mChainsMutex); -#endif #if LL_DEBUG_PIPE_TYPE_IN_PUMP lldebugs << "LLPumpIO::addChain() " << links[0].mPipe << " '" << typeid(*(links[0].mPipe)).name() << "'" << llendl; @@ -266,6 +258,17 @@ bool LLPumpIO::addChain( info.mChainLinks = links; info.mData = data; info.mContext = context; + for (; link != end; ++link) + { + if (link->mPipe->hasExpiration()) + { + info.mHasExpiration = true; + break; + } + } +#if LL_THREADS_APR + LLScopedLock lock(mChainsMutex); +#endif mPendingChains.push_back(info); return true; } @@ -1114,6 +1117,8 @@ bool LLPumpIO::handleChainError( LLChainInfo& chain, LLIOPipe::EStatus error) { + DoutEntering(dc::notice, "LLPumpIO::handleChainError(" << (void*)&chain << ", " << LLIOPipe::lookupStatusString(error) << ")"); + LLMemType m1(LLMemType::MTYPE_IO_PUMP); links_t::reverse_iterator rit; if(chain.mHead == chain.mChainLinks.end()) diff --git a/indra/llmessage/llurlrequest.cpp b/indra/llmessage/llurlrequest.cpp index 9d5b18074..8bb52c55a 100644 --- a/indra/llmessage/llurlrequest.cpp +++ b/indra/llmessage/llurlrequest.cpp @@ -283,10 +283,24 @@ LLIOPipe::EStatus LLURLRequest::handleError( LLIOPipe::EStatus status, LLPumpIO* pump) { + DoutEntering(dc::curl, "LLURLRequest::handleError(" << LLIOPipe::lookupStatusString(status) << ", " << (void*)pump << ")"); LLMemType m1(LLMemType::MTYPE_IO_URL_REQUEST); - - if(!hasNotExpired()) + + if (LL_LIKELY(!mDetail->mCurlEasyRequest.isBuffered())) // Currently always true. { + // The last reference will be deleted when the pump that this chain belongs to + // is removed from the running chains vector, upon returning from this function. + // This keeps the CurlEasyRequest object alive until the curl thread cleanly removed it. + Dout(dc::curl, "Calling mDetail->mCurlEasyRequest.removeRequest()"); + mDetail->mCurlEasyRequest.removeRequest(); + } + else if (!hasNotExpired()) + { + // The buffered version has it's own time out handling, and that already expired, + // so we can ignore the expiration of this timer (currently never happens). + // I left it here because it's what LL did (in the form if (!isValid() ...), + // and it would be relevant if this characteristic of mDetail->mCurlEasyRequest + // would change. --Aleric return STATUS_EXPIRED ; }