From e62f805bcd42f6a2c3be8d3acc83cf564e0e5399 Mon Sep 17 00:00:00 2001 From: Aleric Inglewood Date: Sun, 4 Nov 2012 17:08:05 +0100 Subject: [PATCH] Fix wrong assertion mAddedEasyRequests.size() >= (size_t)mRunningHandles Rename check_run_count to check_msg_queue, because the whole 'run count' approach is flawed anyway (the author of libcurl told me that THE way to check for finished curl handles is to just call curl_multi_info_read every time: it's extremely fast. Any test that attempts to avoid that call is nonsense anyway. The reason the assertion failed might have been caused by the fact that we're comparing the current number of easy handles with the number of running handles of 'a while ago'. It is possible that a easy handle was removed in the meantime. In order to check if that hypothesis is right, I moved the assertion to directly below the call to curl_multi_socket_action where it should hold. If this new assertion doesn't trigger than the hypothesis was right and this is fixed. --- indra/llmessage/aicurl.cpp | 6 +-- indra/llmessage/aicurl.h | 2 +- indra/llmessage/aicurlprivate.h | 2 +- indra/llmessage/aicurlthread.cpp | 66 ++++++++++++++++---------------- indra/llmessage/aicurlthread.h | 6 +-- 5 files changed, 38 insertions(+), 44 deletions(-) diff --git a/indra/llmessage/aicurl.cpp b/indra/llmessage/aicurl.cpp index 06e13e0f0..427721dbd 100644 --- a/indra/llmessage/aicurl.cpp +++ b/indra/llmessage/aicurl.cpp @@ -1036,7 +1036,7 @@ void CurlEasyRequest::finalizeRequest(std::string const& url, AIHTTPTimeoutPolic { DoutCurlEntering("CurlEasyRequest::finalizeRequest(\"" << url << "\", " << policy.name() << ", " << (void*)state_machine << ")"); llassert(!mTimeoutPolicy); // May only call finalizeRequest once! - mResult = CURLE_FAILED_INIT; // General error code; the final result code is stored here by MultiHandle::check_run_count when msg is CURLMSG_DONE. + mResult = CURLE_FAILED_INIT; // General error code; the final result code is stored here by MultiHandle::check_msg_queue when msg is CURLMSG_DONE. #ifdef SHOW_ASSERT // Do a sanity check on the headers. int content_type_count = 0; @@ -1063,13 +1063,13 @@ void CurlEasyRequest::finalizeRequest(std::string const& url, AIHTTPTimeoutPolic // will be used if the object is deleted [In fact, since this is finalizeRequest() and not addRequest(), // incrementing the reference counter would be wrong: if addRequest() is never called then the object is // destroyed shortly after and this pointer is never even used.] - // This pointer is used in MultiHandle::check_run_count, which means that addRequest() was called and + // This pointer is used in MultiHandle::check_msg_queue, which means that addRequest() was called and // the reference counter was increased and the object is being kept alive, see the comments above // command_queue in aicurlthread.cpp. In fact, this object survived until MultiHandle::add_easy_request // was called and is kept alive by MultiHandle::mAddedEasyRequests. The only way to get deleted after // that is when MultiHandle::remove_easy_request is called, which first removes the easy handle from // the multi handle. So that it's (hopefully) no longer possible that info_read() in - // MultiHandle::check_run_count returns this easy handle, after the object is destroyed by deleting + // MultiHandle::check_msg_queue returns this easy handle, after the object is destroyed by deleting // it from MultiHandle::mAddedEasyRequests. setopt(CURLOPT_PRIVATE, get_lockobj()); } diff --git a/indra/llmessage/aicurl.h b/indra/llmessage/aicurl.h index 4424e4299..c5e27db74 100644 --- a/indra/llmessage/aicurl.h +++ b/indra/llmessage/aicurl.h @@ -273,7 +273,7 @@ class AICurlEasyRequest { // it's not thread-safe in itself. AICurlEasyRequest(AICurlPrivate::BufferedCurlEasyRequestPtr const& ptr) : mBufferedCurlEasyRequest(ptr) { } - // This one is obviously dangerous. It's for use only in MultiHandle::check_run_count. + // This one is obviously dangerous. It's for use only in MultiHandle::check_msg_queue. // See also the long comment in BufferedCurlEasyRequest::finalizeRequest with regard to CURLOPT_PRIVATE. explicit AICurlEasyRequest(AICurlPrivate::ThreadSafeBufferedCurlEasyRequest* ptr) : mBufferedCurlEasyRequest(ptr) { } }; diff --git a/indra/llmessage/aicurlprivate.h b/indra/llmessage/aicurlprivate.h index 9fb62ca15..8c8bc3621 100644 --- a/indra/llmessage/aicurlprivate.h +++ b/indra/llmessage/aicurlprivate.h @@ -357,7 +357,7 @@ class CurlEasyRequest : public CurlEasyHandle { // Called by in case of an error. void print_diagnostics(CURLcode code); - // Called by MultiHandle::check_run_count() to fill info with the transfer info. + // Called by MultiHandle::check_msg_queue() to fill info with the transfer info. void getTransferInfo(AITransferInfo* info); // If result != CURLE_FAILED_INIT then also info was filled. diff --git a/indra/llmessage/aicurlthread.cpp b/indra/llmessage/aicurlthread.cpp index 91744ce23..8d3cab884 100644 --- a/indra/llmessage/aicurlthread.cpp +++ b/indra/llmessage/aicurlthread.cpp @@ -1411,7 +1411,7 @@ void AICurlThread::run(void) // that libcurl removed file descriptors which we subsequently // didn't handle. } - multi_handle_w->check_run_count(); + multi_handle_w->check_msg_queue(); } } AICurlMultiHandle::destroyInstance(); @@ -1420,7 +1420,7 @@ void AICurlThread::run(void) //----------------------------------------------------------------------------- // MultiHandle -MultiHandle::MultiHandle(void) : mRunningHandles(0), mTimeout(-1), mReadPollSet(NULL), mWritePollSet(NULL) +MultiHandle::MultiHandle(void) : mTimeout(-1), mReadPollSet(NULL), mWritePollSet(NULL) { mReadPollSet = new PollSet; mWritePollSet = new PollSet; @@ -1504,12 +1504,14 @@ int MultiHandle::timer_callback(CURLM* multi, long timeout_ms, void* userp) CURLMcode MultiHandle::socket_action(curl_socket_t sockfd, int ev_bitmask) { + int running_handles; CURLMcode res; do { - res = check_multi_code(curl_multi_socket_action(mMultiHandle, sockfd, ev_bitmask, &mRunningHandles)); + res = check_multi_code(curl_multi_socket_action(mMultiHandle, sockfd, ev_bitmask, &running_handles)); } while(res == CURLM_CALL_MULTI_PERFORM); + llassert(mAddedEasyRequests.size() >= (size_t)running_handles); return res; } @@ -1614,42 +1616,38 @@ CURLMcode MultiHandle::remove_easy_request(addedEasyRequests_type::iterator cons return res; } -void MultiHandle::check_run_count(void) +void MultiHandle::check_msg_queue(void) { - llassert(mAddedEasyRequests.size() >= (size_t)mRunningHandles); - if (mAddedEasyRequests.size() - (size_t)mRunningHandles > 0) // There is no need to do this when all easy handles are accounted for. + CURLMsg const* msg; + int msgs_left; + while ((msg = info_read(&msgs_left))) { - CURLMsg const* msg; - int msgs_left; - while ((msg = info_read(&msgs_left))) + if (msg->msg == CURLMSG_DONE) { - if (msg->msg == CURLMSG_DONE) + CURL* easy = msg->easy_handle; + ThreadSafeBufferedCurlEasyRequest* ptr; + CURLcode rese = curl_easy_getinfo(easy, CURLINFO_PRIVATE, &ptr); + llassert_always(rese == CURLE_OK); + AICurlEasyRequest easy_request(ptr); + llassert(*AICurlEasyRequest_wat(*easy_request) == easy); + // Store result and trigger events for the easy request. + finish_easy_request(easy_request, msg->data.result); + // This invalidates msg, but not easy_request. + CURLMcode res = remove_easy_request(easy_request); + // This should hold, I think, because the handles are obviously ok and + // the only error we could get is when remove_easy_request() was already + // called before (by this thread); but if that was the case then the easy + // handle should not have been be returned by info_read()... + llassert(res == CURLM_OK); + // Nevertheless, if it was already removed then just ignore it. + if (res == CURLM_OK) { - CURL* easy = msg->easy_handle; - ThreadSafeBufferedCurlEasyRequest* ptr; - CURLcode rese = curl_easy_getinfo(easy, CURLINFO_PRIVATE, &ptr); - llassert_always(rese == CURLE_OK); - AICurlEasyRequest easy_request(ptr); - llassert(*AICurlEasyRequest_wat(*easy_request) == easy); - // Store result and trigger events for the easy request. - finish_easy_request(easy_request, msg->data.result); - // This invalidates msg, but not easy_request. - CURLMcode res = remove_easy_request(easy_request); - // This should hold, I think, because the handles are obviously ok and - // the only error we could get is when remove_easy_request() was already - // called before (by this thread); but if that was the case then the easy - // handle should not have been be returned by info_read()... - llassert(res == CURLM_OK); - // Nevertheless, if it was already removed then just ignore it. - if (res == CURLM_OK) - { - } - else if (res == -2) - { - llwarns << "Curl easy handle returned by curl_multi_info_read() that is not (anymore) in MultiHandle::mAddedEasyRequests!?!" << llendl; - } - // Destruction of easy_request at this point, causes the CurlEasyRequest to be deleted. } + else if (res == -2) + { + llwarns << "Curl easy handle returned by curl_multi_info_read() that is not (anymore) in MultiHandle::mAddedEasyRequests!?!" << llendl; + } + // Destruction of easy_request at this point, causes the CurlEasyRequest to be deleted. } } } diff --git a/indra/llmessage/aicurlthread.h b/indra/llmessage/aicurlthread.h index 1ca677990..32ebad685 100644 --- a/indra/llmessage/aicurlthread.h +++ b/indra/llmessage/aicurlthread.h @@ -75,7 +75,6 @@ class MultiHandle : public CurlMultiHandle private: typedef std::set addedEasyRequests_type; addedEasyRequests_type mAddedEasyRequests; // All easy requests currently added to the multi handle. - int mRunningHandles; // The last value returned by curl_multi_socket_action. long mTimeout; // The last timeout in ms as set by the callback CURLMOPT_TIMERFUNCTION. private: @@ -89,14 +88,11 @@ class MultiHandle : public CurlMultiHandle static int timer_callback(CURLM* multi, long timeout_ms, void* userp); public: - // Returns the number of active easy handles as reported by the last call to curl_multi_socket_action. - int getRunningHandles(void) const { return mRunningHandles; } - // Returns how long to wait for socket action before calling socket_action(CURL_SOCKET_TIMEOUT, 0), in ms. int getTimeout(void) const { return mTimeout; } // This is called before sleeping, after calling (one or more times) socket_action. - void check_run_count(void); + void check_msg_queue(void); // Called from the main loop every time select() timed out. void handle_stalls(void);