From e79a88c8ccfadcd260892000d4dec2ae921b26de Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Tue, 12 Aug 2014 18:21:26 -0400 Subject: Better support for dynamic option changes in llcorehttp. Libcurl has some problems disabling pipelining on a multi handle with outstanding requests so build a more conservative system that allows requests to drain before setting curl multi options. Would rather not have this but it is significantly safer. "HttpPipelining" debug setting is now fully dynamic. Connection limits can also be made dynamic in the near future. Upped the default connection count back to 8 for now but will revisit this in the tuning phase. It might be time to combine mesh and textures into a single asset class. For normal server operations that would be a clear path, but for server under load, the current scheme may be better. Minor cleanup in logging to elminate some redundant strings. Might add some more tracing to the stall logic 'just in case'. --- indra/llcorehttp/_httpoprequest.cpp | 46 +++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 22 deletions(-) (limited to 'indra/llcorehttp/_httpoprequest.cpp') diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp index 43dd069bc6..eb664fdced 100755 --- a/indra/llcorehttp/_httpoprequest.cpp +++ b/indra/llcorehttp/_httpoprequest.cpp @@ -4,7 +4,7 @@ * * $LicenseInfo:firstyear=2012&license=viewerlgpl$ * Second Life Viewer Source Code - * Copyright (C) 2012-2013, Linden Research, Inc. + * Copyright (C) 2012-2014, Linden Research, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -94,6 +94,8 @@ void os_strlower(char * str); void check_curl_easy_code(CURLcode code); void check_curl_easy_code(CURLcode code, int curl_setopt_option); +static const char * const LOG_CORE("CoreHttp"); + } // end anonymous namespace @@ -416,8 +418,8 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) if (! mCurlHandle) { // We're in trouble. We'll continue but it won't go well. - LL_WARNS("CoreHttp") << "Failed to allocate libcurl easy handle. Continuing." - << LL_ENDL; + LL_WARNS(LOG_CORE) << "Failed to allocate libcurl easy handle. Continuing." + << LL_ENDL; return HttpStatus(HttpStatus::LLCORE, HE_BAD_ALLOC); } code = curl_easy_setopt(mCurlHandle, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4); @@ -538,9 +540,9 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) break; default: - LL_ERRS("CoreHttp") << "Invalid HTTP method in request: " - << int(mReqMethod) << ". Can't recover." - << LL_ENDL; + LL_ERRS(LOG_CORE) << "Invalid HTTP method in request: " + << int(mReqMethod) << ". Can't recover." + << LL_ENDL; break; } @@ -652,8 +654,8 @@ size_t HttpOpRequest::readCallback(void * data, size_t size, size_t nmemb, void { // Warn but continue if the read position moves beyond end-of-body // for some reason. - LL_WARNS("CoreHttp") << "Request body position beyond body size. Truncating request body." - << LL_ENDL; + LL_WARNS(LOG_CORE) << "Request body position beyond body size. Truncating request body." + << LL_ENDL; } return 0; } @@ -790,10 +792,10 @@ size_t HttpOpRequest::headerCallback(void * data, size_t size, size_t nmemb, voi else { // Ignore the unparsable. - LL_INFOS_ONCE("CoreHttp") << "Problem parsing odd Content-Range header: '" - << std::string(hdr_data, wanted_hdr_size) - << "'. Ignoring." - << LL_ENDL; + LL_INFOS_ONCE(LOG_CORE) << "Problem parsing odd Content-Range header: '" + << std::string(hdr_data, wanted_hdr_size) + << "'. Ignoring." + << LL_ENDL; } } @@ -895,11 +897,11 @@ int HttpOpRequest::debugCallback(CURL * handle, curl_infotype info, char * buffe if (logit) { - LL_INFOS("CoreHttp") << "TRACE, LibcurlDebug, Handle: " - << static_cast(op) - << ", Type: " << tag - << ", Data: " << safe_line - << LL_ENDL; + LL_INFOS(LOG_CORE) << "TRACE, LibcurlDebug, Handle: " + << static_cast(op) + << ", Type: " << tag + << ", Data: " << safe_line + << LL_ENDL; } return 0; @@ -1094,9 +1096,9 @@ void check_curl_easy_code(CURLcode code, int curl_setopt_option) // // linux appears to throw a curl error once per session for a bad initialization // at a pretty random time (when enabling cookies). - LL_WARNS("CoreHttp") << "libcurl error detected: " << curl_easy_strerror(code) - << ", curl_easy_setopt option: " << curl_setopt_option - << LL_ENDL; + LL_WARNS(LOG_CORE) << "libcurl error detected: " << curl_easy_strerror(code) + << ", curl_easy_setopt option: " << curl_setopt_option + << LL_ENDL; } } @@ -1109,8 +1111,8 @@ void check_curl_easy_code(CURLcode code) // // linux appears to throw a curl error once per session for a bad initialization // at a pretty random time (when enabling cookies). - LL_WARNS("CoreHttp") << "libcurl error detected: " << curl_easy_strerror(code) - << LL_ENDL; + LL_WARNS(LOG_CORE) << "libcurl error detected: " << curl_easy_strerror(code) + << LL_ENDL; } } -- cgit v1.3 From 0c20beda6800149ee71a307ca4e943b5bba56908 Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Thu, 4 Sep 2014 16:57:44 -0400 Subject: Pipelining work. Extend transfer timeout by the pipeline depth as transfers can appear delayed with deep pipelining and more requests in the pool. Added bad HTTP status error (typically getting a 0 back as HTTP status from libcurl) to the list of retryable errors. There's a response stream problem with libcurl and pipelining that induces this problem. Retrying helps but may not be entirely safe. Watch bug 1420 on the libcurl sourceforge bug tracker. Extend options of test/example program to include un-ranged requests. Document the excessive data transfer induced when ranged requests are disabled. This is an abnormal mode for very rare users so we'll just eat that for now. --- indra/llcorehttp/_httplibcurl.cpp | 15 +++++++---- indra/llcorehttp/_httpoprequest.cpp | 34 ++++++++++++++++++------- indra/llcorehttp/examples/http_texture_load.cpp | 21 ++++++++++++--- indra/llcorehttp/httpcommon.cpp | 13 ++++++++-- indra/newview/app_settings/settings.xml | 4 +-- indra/newview/llmeshrepository.cpp | 13 +++++++++- 6 files changed, 78 insertions(+), 22 deletions(-) (limited to 'indra/llcorehttp/_httpoprequest.cpp') diff --git a/indra/llcorehttp/_httplibcurl.cpp b/indra/llcorehttp/_httplibcurl.cpp index b46833a1f3..cfbe0fd2bb 100755 --- a/indra/llcorehttp/_httplibcurl.cpp +++ b/indra/llcorehttp/_httplibcurl.cpp @@ -217,8 +217,17 @@ void HttpLibcurl::addOp(HttpOpRequest * op) } // Make the request live - curl_multi_add_handle(mMultiHandles[op->mReqPolicy], op->mCurlHandle); + CURLMcode code; + code = curl_multi_add_handle(mMultiHandles[op->mReqPolicy], op->mCurlHandle); + if (CURLM_OK != code) + { + // *TODO: Better cleanup and recovery but not much we can do here. + check_curl_multi_code(code); + return; + } op->mCurlActive = true; + mActiveOps.insert(op); + ++mActiveHandles[op->mReqPolicy]; if (op->mTracing > HTTP_TRACE_OFF) { @@ -230,10 +239,6 @@ void HttpLibcurl::addOp(HttpOpRequest * op) << ", Readies: " << policy.getReadyCount(op->mReqPolicy) << LL_ENDL; } - - // On success, make operation active - mActiveOps.insert(op); - ++mActiveHandles[op->mReqPolicy]; } diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp index eb664fdced..38c1f1e78a 100755 --- a/indra/llcorehttp/_httpoprequest.cpp +++ b/indra/llcorehttp/_httpoprequest.cpp @@ -378,6 +378,7 @@ void HttpOpRequest::setupCommon(HttpRequest::policy_t policy_id, // Junk may be left around from a failed request and that // needs to be cleaned out. // +// *TODO: Move this to _httplibcurl where it belongs. HttpStatus HttpOpRequest::prepareRequest(HttpService * service) { CURLcode code; @@ -411,8 +412,9 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) // *FIXME: better error handling later HttpStatus status; - // Get global policy options - HttpPolicyGlobal & policy(service->getPolicy().getGlobalOptions()); + // Get global and class policy options + HttpPolicyGlobal & gpolicy(service->getPolicy().getGlobalOptions()); + HttpPolicyClass & cpolicy(service->getPolicy().getClassOptions(mReqPolicy)); mCurlHandle = LLCurl::createStandardCurlHandle(); if (! mCurlHandle) @@ -462,30 +464,30 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) code = curl_easy_setopt(mCurlHandle, CURLOPT_SSL_VERIFYHOST, 0); check_curl_easy_code(code, CURLOPT_SSL_VERIFYHOST); - if (policy.mUseLLProxy) + if (gpolicy.mUseLLProxy) { // Use the viewer-based thread-safe API which has a // fast/safe check for proxy enable. Would like to // encapsulate this someway... LLProxy::getInstance()->applyProxySettings(mCurlHandle); } - else if (policy.mHttpProxy.size()) + else if (gpolicy.mHttpProxy.size()) { // *TODO: This is fine for now but get fuller socks5/ // authentication thing going later.... - code = curl_easy_setopt(mCurlHandle, CURLOPT_PROXY, policy.mHttpProxy.c_str()); + code = curl_easy_setopt(mCurlHandle, CURLOPT_PROXY, gpolicy.mHttpProxy.c_str()); check_curl_easy_code(code, CURLOPT_PROXY); code = curl_easy_setopt(mCurlHandle, CURLOPT_PROXYTYPE, CURLPROXY_HTTP); check_curl_easy_code(code, CURLOPT_PROXYTYPE); } - if (policy.mCAPath.size()) + if (gpolicy.mCAPath.size()) { - code = curl_easy_setopt(mCurlHandle, CURLOPT_CAPATH, policy.mCAPath.c_str()); + code = curl_easy_setopt(mCurlHandle, CURLOPT_CAPATH, gpolicy.mCAPath.c_str()); check_curl_easy_code(code, CURLOPT_CAPATH); } - if (policy.mCAFile.size()) + if (gpolicy.mCAFile.size()) { - code = curl_easy_setopt(mCurlHandle, CURLOPT_CAINFO, policy.mCAFile.c_str()); + code = curl_easy_setopt(mCurlHandle, CURLOPT_CAINFO, gpolicy.mCAFile.c_str()); check_curl_easy_code(code, CURLOPT_CAINFO); } @@ -594,6 +596,20 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) { xfer_timeout = timeout; } + if (cpolicy.mPipelining > 1L) + { + // Pipelining affects both connection and transfer timeout values. + // Requests that are added to a pipeling immediately have completed + // their connection so the connection delay tends to be less than + // the non-pipelined value. Transfers are the opposite. Transfer + // timeout starts once the connection is established and completion + // can be delayed due to the pipelined requests ahead. So, it's + // a handwave but bump the transfer timeout up by the pipelining + // depth to give some room. + // + // *TODO: Find a better scheme than timeouts to guarantee liveness. + xfer_timeout *= cpolicy.mPipelining; + } code = curl_easy_setopt(mCurlHandle, CURLOPT_TIMEOUT, xfer_timeout); check_curl_easy_code(code, CURLOPT_TIMEOUT); code = curl_easy_setopt(mCurlHandle, CURLOPT_CONNECTTIMEOUT, timeout); diff --git a/indra/llcorehttp/examples/http_texture_load.cpp b/indra/llcorehttp/examples/http_texture_load.cpp index 88692c3f69..b76c874557 100755 --- a/indra/llcorehttp/examples/http_texture_load.cpp +++ b/indra/llcorehttp/examples/http_texture_load.cpp @@ -102,6 +102,7 @@ public: public: bool mVerbose; bool mRandomRange; + bool mNoRange; int mRequestLowWater; int mRequestHighWater; handle_set_t mHandles; @@ -162,10 +163,11 @@ int main(int argc, char** argv) { LLCore::HttpStatus status; bool do_random(false); + bool do_whole(false); bool do_verbose(false); int option(-1); - while (-1 != (option = getopt(argc, argv, "u:c:h?RvH:p:t:"))) + while (-1 != (option = getopt(argc, argv, "u:c:h?RwvH:p:t:"))) { switch (option) { @@ -236,6 +238,12 @@ int main(int argc, char** argv) case 'R': do_random = true; + do_whole = false; + break; + + case 'w': + do_whole = true; + do_random = false; break; case 'v': @@ -307,6 +315,7 @@ int main(int argc, char** argv) ws.mUrl = url_format; ws.loadAssetUuids(uuids); ws.mRandomRange = do_random; + ws.mNoRange = do_whole; ws.mVerbose = do_verbose; ws.mRequestHighWater = highwater; ws.mRequestLowWater = ws.mRequestHighWater / 2; @@ -381,6 +390,7 @@ void usage(std::ostream & out) " -u printf-style format string for URL generation\n" " Default: " << url_format << "\n" " -R Issue GETs with random Range: headers\n" + " -w Issue GETs without Range: headers to get whole object\n" " -c Maximum connection concurrency. Range: [1..100]\n" " Default: " << concurrency_limit << "\n" " -H HTTP request highwater (requests fed to llcorehttp).\n" @@ -400,6 +410,7 @@ WorkingSet::WorkingSet() : LLCore::HttpHandler(), mVerbose(false), mRandomRange(false), + mNoRange(false), mRemaining(200), mLimit(200), mAt(0), @@ -449,8 +460,12 @@ bool WorkingSet::reload(LLCore::HttpRequest * hr, LLCore::HttpOptions * opt) #else snprintf(buffer, sizeof(buffer), mUrl.c_str(), mAssets[mAt].mUuid.c_str()); #endif - int offset(mRandomRange ? ((unsigned long) rand()) % 1000000UL : mAssets[mAt].mOffset); - int length(mRandomRange ? ((unsigned long) rand()) % 1000000UL : mAssets[mAt].mLength); + int offset(mNoRange + ? 0 + : (mRandomRange ? ((unsigned long) rand()) % 1000000UL : mAssets[mAt].mOffset)); + int length(mNoRange + ? 0 + : (mRandomRange ? ((unsigned long) rand()) % 1000000UL : mAssets[mAt].mLength)); LLCore::HttpHandle handle; if (offset || length) diff --git a/indra/llcorehttp/httpcommon.cpp b/indra/llcorehttp/httpcommon.cpp index c2f15155ac..9bcf7ac5e3 100755 --- a/indra/llcorehttp/httpcommon.cpp +++ b/indra/llcorehttp/httpcommon.cpp @@ -4,7 +4,7 @@ * * $LicenseInfo:firstyear=2012&license=viewerlgpl$ * Second Life Viewer Source Code - * Copyright (C) 2012-2013, Linden Research, Inc. + * Copyright (C) 2012-2014, Linden Research, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -219,6 +219,13 @@ std::string HttpStatus::toTerseString() const // Pass true on statuses that might actually be cleared by a // retry. Library failures, calling problems, etc. aren't // going to be fixed by squirting bits all over the Net. +// +// HE_INVALID_HTTP_STATUS is special. As of 7.37.0, there are +// some scenarios where response processing in libcurl appear +// to go wrong and response data is corrupted. A side-effect +// of this is that the HTTP status is read as 0 from the library. +// See libcurl bug report 1420 (https://sourceforge.net/p/curl/bugs/1420/) +// for details. bool HttpStatus::isRetryable() const { static const HttpStatus cant_connect(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_CONNECT); @@ -231,6 +238,7 @@ bool HttpStatus::isRetryable() const static const HttpStatus post_error(HttpStatus::EXT_CURL_EASY, CURLE_HTTP_POST_ERROR); static const HttpStatus partial_file(HttpStatus::EXT_CURL_EASY, CURLE_PARTIAL_FILE); static const HttpStatus inv_cont_range(HttpStatus::LLCORE, HE_INV_CONTENT_RANGE_HDR); + static const HttpStatus inv_status(HttpStatus::LLCORE, HE_INVALID_HTTP_STATUS); return ((isHttpStatus() && mType >= 499 && mType <= 599) || // Include special 499 in retryables *this == cant_connect || // Connection reset/endpoint problems @@ -242,7 +250,8 @@ bool HttpStatus::isRetryable() const *this == op_timedout || // Timer expired *this == post_error || // Transport problem *this == partial_file || // Data inconsistency in response - *this == inv_cont_range); // Short data read disagrees with content-range + *this == inv_cont_range || // Short data read disagrees with content-range + *this == inv_status); // Inv status can reflect internal state problem in libcurl } } // end namespace LLCore diff --git a/indra/newview/app_settings/settings.xml b/indra/newview/app_settings/settings.xml index 0607579a08..ab15a03229 100755 --- a/indra/newview/app_settings/settings.xml +++ b/indra/newview/app_settings/settings.xml @@ -4459,7 +4459,7 @@ HttpPipelining Comment - If true, viewer will pipeline HTTP requests to servers. + If true, viewer will attempt to pipeline HTTP requests. Persist 1 Type @@ -4470,7 +4470,7 @@ HttpRangeRequestsDisable Comment - If true, viewer will not issued range GET requests for meshes and textures. May resolve problems with certain ISPs and networking gear. + If true, viewer will not issue GET requests with 'Range:' headers for meshes and textures. May resolve problems with certain ISPs and networking gear. Persist 1 Type diff --git a/indra/newview/llmeshrepository.cpp b/indra/newview/llmeshrepository.cpp index 6477389d4c..a6707392fe 100755 --- a/indra/newview/llmeshrepository.cpp +++ b/indra/newview/llmeshrepository.cpp @@ -2644,6 +2644,17 @@ void LLMeshRepository::cacheOutgoingMesh(LLMeshUploadData& data, LLSD& header) } +// Handle failed or successful requests for mesh assets. +// +// Support for 200 responses was added for several reasons. One, +// a service or cache can ignore range headers and give us a +// 200 with full asset should it elect to. We also support +// a debug flag which disables range requests for those very +// few users that have some sort of problem with their networking +// services. But the 200 response handling is suboptimal: rather +// than cache the whole asset, we just extract the part that would +// have been sent in a 206 and process that. Inefficient but these +// are cases far off the norm. void LLMeshHandlerBase::onCompleted(LLCore::HttpHandle handle, LLCore::HttpResponse * response) { mProcessed = true; @@ -2685,7 +2696,7 @@ void LLMeshHandlerBase::onCompleted(LLCore::HttpHandle handle, LLCore::HttpRespo if (par_status == status) { - // 216 case + // 206 case response->getRange(&offset, &length, &full_length); if (! offset && ! length) { -- cgit v1.3 From 3057d246f0c63be1b3015313b9ad9824a66f4e0f Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Thu, 18 Sep 2014 18:42:30 -0400 Subject: Documentation. Describe curl bug 1420 testing and how to reproduce data corruption via timeouts. --- indra/llcorehttp/_httpoprequest.cpp | 15 +++++++++++++++ indra/llcorehttp/httpcommon.cpp | 4 ++++ 2 files changed, 19 insertions(+) (limited to 'indra/llcorehttp/_httpoprequest.cpp') diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp index 38c1f1e78a..4453bf2922 100755 --- a/indra/llcorehttp/_httpoprequest.cpp +++ b/indra/llcorehttp/_httpoprequest.cpp @@ -47,6 +47,19 @@ #include "llhttpconstants.h" #include "llproxy.h" +// *DEBUG: "[curl:bugs] #1420" problem and testing. +// +// A pipelining problem, https://sourceforge.net/p/curl/bugs/1420/, +// was a source of Core_9 failures. Code related to this can be +// identified and tested by: +// * Looking for '[curl:bugs]' strings in source and following +// instructions there. +// * Set 'QAModeHttpTrace' to 2 or 3 in settings.xml and look for +// 'timed out' events in the log. +// * Enable the HttpRangeRequestsDisable debug setting which causes +// full asset fetches. These slow the pipelines down a bit. +// + namespace { @@ -610,6 +623,8 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) // *TODO: Find a better scheme than timeouts to guarantee liveness. xfer_timeout *= cpolicy.mPipelining; } + // *DEBUG: Useful for timeout handling and "[curl:bugs] #1420" tests + // xfer_timeout = 3L; code = curl_easy_setopt(mCurlHandle, CURLOPT_TIMEOUT, xfer_timeout); check_curl_easy_code(code, CURLOPT_TIMEOUT); code = curl_easy_setopt(mCurlHandle, CURLOPT_CONNECTTIMEOUT, timeout); diff --git a/indra/llcorehttp/httpcommon.cpp b/indra/llcorehttp/httpcommon.cpp index 9bcf7ac5e3..8714915fa2 100755 --- a/indra/llcorehttp/httpcommon.cpp +++ b/indra/llcorehttp/httpcommon.cpp @@ -240,6 +240,10 @@ bool HttpStatus::isRetryable() const static const HttpStatus inv_cont_range(HttpStatus::LLCORE, HE_INV_CONTENT_RANGE_HDR); static const HttpStatus inv_status(HttpStatus::LLCORE, HE_INVALID_HTTP_STATUS); + // *DEBUG: For "[curl:bugs] #1420" tests. + // Disable the '*this == inv_status' test and look for 'Core_9' + // failures in log files. + return ((isHttpStatus() && mType >= 499 && mType <= 599) || // Include special 499 in retryables *this == cant_connect || // Connection reset/endpoint problems *this == cant_res_proxy || // DNS problems -- cgit v1.3 From 79ab7c20703c092a4416a4f9a885e0246fc17ee0 Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Fri, 19 Sep 2014 15:34:09 -0400 Subject: Introduce libcurl handle cache. Create a private cache of used handles and a fast handle factory that's thread- correct. --- indra/llcorehttp/_httplibcurl.cpp | 83 +++++++++++++++++++++++++++++++++++-- indra/llcorehttp/_httplibcurl.h | 78 ++++++++++++++++++++++++++++++++-- indra/llcorehttp/_httpoprequest.cpp | 5 ++- 3 files changed, 158 insertions(+), 8 deletions(-) (limited to 'indra/llcorehttp/_httpoprequest.cpp') diff --git a/indra/llcorehttp/_httplibcurl.cpp b/indra/llcorehttp/_httplibcurl.cpp index cfbe0fd2bb..81b44ab90b 100755 --- a/indra/llcorehttp/_httplibcurl.cpp +++ b/indra/llcorehttp/_httplibcurl.cpp @@ -51,6 +51,7 @@ namespace LLCore HttpLibcurl::HttpLibcurl(HttpService * service) : mService(service), + mHandleCache(), mPolicyCount(0), mMultiHandles(NULL), mActiveHandles(NULL), @@ -61,7 +62,7 @@ HttpLibcurl::HttpLibcurl(HttpService * service) HttpLibcurl::~HttpLibcurl() { shutdown(); - + mService = NULL; } @@ -279,7 +280,7 @@ void HttpLibcurl::cancelRequest(HttpOpRequest * op) // Detach from multi and recycle handle curl_multi_remove_handle(mMultiHandles[op->mReqPolicy], op->mCurlHandle); - curl_easy_cleanup(op->mCurlHandle); + mHandleCache.freeHandle(op->mCurlHandle); op->mCurlHandle = NULL; // Tracing @@ -356,7 +357,7 @@ bool HttpLibcurl::completeRequest(CURLM * multi_handle, CURL * handle, CURLcode // Detach from multi and recycle handle curl_multi_remove_handle(multi_handle, handle); - curl_easy_cleanup(handle); + mHandleCache.freeHandle(op->mCurlHandle); op->mCurlHandle = NULL; // Tracing @@ -471,6 +472,82 @@ void HttpLibcurl::policyUpdated(int policy_class) } } +// --------------------------------------- +// HttpLibcurl::HandleCache +// --------------------------------------- + +HttpLibcurl::HandleCache::HandleCache() + : mHandleTemplate(NULL) +{ + mCache.reserve(50); +} + + +HttpLibcurl::HandleCache::~HandleCache() +{ + if (mHandleTemplate) + { + curl_easy_cleanup(mHandleTemplate); + mHandleTemplate = NULL; + } + + for (handle_cache_t::iterator it(mCache.begin()); mCache.end() != it; ++it) + { + curl_easy_cleanup(*it); + } + mCache.clear(); +} + + +CURL * HttpLibcurl::HandleCache::getHandle() +{ + CURL * ret(NULL); + + if (! mCache.empty()) + { + // Fastest path to handle + ret = mCache.back(); + mCache.pop_back(); + } + else if (mHandleTemplate) + { + // Still fast path + ret = curl_easy_duphandle(mHandleTemplate); + } + else + { + // When all else fails + ret = curl_easy_init(); + } + + return ret; +} + + +void HttpLibcurl::HandleCache::freeHandle(CURL * handle) +{ + if (! handle) + { + return; + } + + curl_easy_reset(handle); + if (! mHandleTemplate) + { + // Save the first freed handle as a template. + mHandleTemplate = handle; + } + else + { + // Otherwise add it to the cache + if (mCache.size() >= mCache.capacity()) + { + mCache.reserve(mCache.capacity() + 50); + } + mCache.push_back(handle); + } +} + // --------------------------------------- // Free functions diff --git a/indra/llcorehttp/_httplibcurl.h b/indra/llcorehttp/_httplibcurl.h index 2c7ad1fa8e..ffc24c63a8 100755 --- a/indra/llcorehttp/_httplibcurl.h +++ b/indra/llcorehttp/_httplibcurl.h @@ -124,6 +124,23 @@ public: /// Threading: called by worker thread. void policyUpdated(int policy_class); + /// Allocate a curl handle for caller. May be freed using + /// either the freeHandle() method or calling curl_easy_cleanup() + /// directly. + /// + /// @return Libcurl handle (CURL *) or NULL on allocation + /// problem. Handle will be in curl_easy_reset() + /// condition. + /// + /// Threading: callable by worker thread. + /// + /// Deprecation: Expect this to go away after _httpoprequest is + /// refactored bringing code into this class. + CURL * getHandle() + { + return mHandleCache.getHandle(); + } + protected: /// Invoked when libcurl has indicated a request has been processed /// to completion and we need to move the request to a new state. @@ -135,14 +152,67 @@ protected: protected: typedef std::set active_set_t; + + /// Simple request handle cache for libcurl. + /// + /// Handle creation is somewhat slow and chunky in libcurl and there's + /// a pretty good speedup to be had from handle re-use. So, a simple + /// vector is kept of 'freed' handles to be reused as needed. When + /// that is empty, the first freed handle is kept as a template for + /// handle duplication. This is still faster than creation from nothing. + /// And when that fails, we init fresh from curl_easy_init(). + /// + /// Handles allocated with getHandle() may be freed with either + /// freeHandle() or curl_easy_cleanup(). Choice may be dictated + /// by thread constraints. + /// + /// Threading: Single-threaded. May only be used by a single thread, + /// typically the worker thread. If freeing requests' handles in an + /// unknown threading context, use curl_easy_cleanup() for safety. + + class HandleCache + { + public: + HandleCache(); + ~HandleCache(); + + private: + HandleCache(const HandleCache &); // Not defined + void operator=(const HandleCache &); // Not defined + + public: + /// Allocate a curl handle for caller. May be freed using + /// either the freeHandle() method or calling curl_easy_cleanup() + /// directly. + /// + /// @return Libcurl handle (CURL *) or NULL on allocation + /// problem. + /// + /// Threading: Single-thread (worker) only. + CURL * getHandle(); + + /// Free a libcurl handle acquired by whatever means. Thread + /// safety is left to the caller. + /// + /// Threading: Single-thread (worker) only. + void freeHandle(CURL * handle); + + protected: + typedef std::vector handle_cache_t; + + protected: + CURL * mHandleTemplate; // Template for duplicating new handles + handle_cache_t mCache; // Cache of old handles + }; // end class HandleCache protected: - HttpService * mService; // Simple reference, not owner + HttpService * mService; // Simple reference, not owner + HandleCache mHandleCache; // Handle allocator, owner active_set_t mActiveOps; int mPolicyCount; - CURLM ** mMultiHandles; // One handle per policy class - int * mActiveHandles; // Active count per policy class - bool * mDirtyPolicy; // Dirty policy update waiting for stall (per pc) + CURLM ** mMultiHandles; // One handle per policy class + int * mActiveHandles; // Active count per policy class + bool * mDirtyPolicy; // Dirty policy update waiting for stall (per pc) }; // end class HttpLibcurl diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp index 4453bf2922..bbda0b82fd 100755 --- a/indra/llcorehttp/_httpoprequest.cpp +++ b/indra/llcorehttp/_httpoprequest.cpp @@ -170,6 +170,8 @@ HttpOpRequest::~HttpOpRequest() if (mCurlHandle) { + // Uncertain of thread context so free using + // safest method. curl_easy_cleanup(mCurlHandle); mCurlHandle = NULL; } @@ -429,7 +431,7 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) HttpPolicyGlobal & gpolicy(service->getPolicy().getGlobalOptions()); HttpPolicyClass & cpolicy(service->getPolicy().getClassOptions(mReqPolicy)); - mCurlHandle = LLCurl::createStandardCurlHandle(); + mCurlHandle = service->getTransport().getHandle(); if (! mCurlHandle) { // We're in trouble. We'll continue but it won't go well. @@ -437,6 +439,7 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) << LL_ENDL; return HttpStatus(HttpStatus::LLCORE, HE_BAD_ALLOC); } + code = curl_easy_setopt(mCurlHandle, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4); check_curl_easy_code(code, CURLOPT_IPRESOLVE); code = curl_easy_setopt(mCurlHandle, CURLOPT_NOSIGNAL, 1); -- cgit v1.3 From ec4fd2f0e226bb2cae5982760317e1d6ea2d2d69 Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Fri, 10 Oct 2014 16:43:04 -0400 Subject: MAINT-4564 HTTP Pipelining is not happening in Drano HTTP Phase 4 Incorporate the new libcurl 7.38.0 build with curl bug 1420 workaround. Add developer-centric testing code to evaluate the workaround or a future fix for 1420. --- autobuild.xml | 12 ++++++------ indra/llcorehttp/_httpoprequest.cpp | 4 ++-- indra/llcorehttp/_httppolicy.cpp | 12 ++++++++++++ indra/llcorehttp/httpcommon.cpp | 5 +++-- 4 files changed, 23 insertions(+), 10 deletions(-) (limited to 'indra/llcorehttp/_httpoprequest.cpp') diff --git a/autobuild.xml b/autobuild.xml index b9db04c28b..1045e11b6d 100755 --- a/autobuild.xml +++ b/autobuild.xml @@ -282,9 +282,9 @@ archive hash - ed283b163e8f74d2c9d6ea5874fcca54 + 40b1c6b3727ebedafc2f1a172797ccd1 url - http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3pl_3p-curl-update/rev/294562/arch/Darwin/installer/curl-7.38.0-darwin-20140923.tar.bz2 + http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3pl_3p-curl-update/rev/295367/arch/Darwin/installer/curl-7.38.0-darwin-20141010.tar.bz2 name darwin @@ -294,9 +294,9 @@ archive hash - 6a0a62b6c026fa0b33c0978f4afd152e + 06149da3d7a34adf40853f813ae55328 url - http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3pl_3p-curl-update/rev/294562/arch/Linux/installer/curl-7.38.0-linux-20140923.tar.bz2 + http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3pl_3p-curl-update/rev/295367/arch/Linux/installer/curl-7.38.0-linux-20141010.tar.bz2 name linux @@ -306,9 +306,9 @@ archive hash - 56ff4698d5b39a37994f4cc8acba19f0 + e4280eae792a5f13bc9d01d8cfb7c557 url - http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3pl_3p-curl-update/rev/294562/arch/CYGWIN/installer/curl-7.38.0-windows-20140923.tar.bz2 + http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3pl_3p-curl-update/rev/295367/arch/CYGWIN/installer/curl-7.38.0-windows-20141010.tar.bz2 name windows diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp index bbda0b82fd..fbbb1614fb 100755 --- a/indra/llcorehttp/_httpoprequest.cpp +++ b/indra/llcorehttp/_httpoprequest.cpp @@ -626,8 +626,8 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) // *TODO: Find a better scheme than timeouts to guarantee liveness. xfer_timeout *= cpolicy.mPipelining; } - // *DEBUG: Useful for timeout handling and "[curl:bugs] #1420" tests - // xfer_timeout = 3L; + // *DEBUG: Enable following override for timeout handling and "[curl:bugs] #1420" tests + // xfer_timeout = 1L; code = curl_easy_setopt(mCurlHandle, CURLOPT_TIMEOUT, xfer_timeout); check_curl_easy_code(code, CURLOPT_TIMEOUT); code = curl_easy_setopt(mCurlHandle, CURLOPT_CONNECTTIMEOUT, timeout); diff --git a/indra/llcorehttp/_httppolicy.cpp b/indra/llcorehttp/_httppolicy.cpp index 09b9206f63..e5d6321401 100755 --- a/indra/llcorehttp/_httppolicy.cpp +++ b/indra/llcorehttp/_httppolicy.cpp @@ -414,6 +414,18 @@ bool HttpPolicy::stageAfterCompletion(HttpOpRequest * op) // Retry or finalize if (! op->mStatus) { + // *DEBUG: For "[curl:bugs] #1420" tests. This will interfere + // with unit tests due to allocation retention by logging code. + // But you won't be checking this in enabled. +#if 0 + if (op->mStatus == HttpStatus(HttpStatus::EXT_CURL_EASY, CURLE_OPERATION_TIMEDOUT)) + { + LL_WARNS(LOG_CORE) << "HTTP request " << static_cast(op) + << " timed out." + << LL_ENDL; + } +#endif + // If this failed, we might want to retry. if (op->mPolicyRetries < op->mPolicyRetryLimit && op->mStatus.isRetryable()) { diff --git a/indra/llcorehttp/httpcommon.cpp b/indra/llcorehttp/httpcommon.cpp index 8714915fa2..7907e958a4 100755 --- a/indra/llcorehttp/httpcommon.cpp +++ b/indra/llcorehttp/httpcommon.cpp @@ -254,8 +254,9 @@ bool HttpStatus::isRetryable() const *this == op_timedout || // Timer expired *this == post_error || // Transport problem *this == partial_file || // Data inconsistency in response - *this == inv_cont_range || // Short data read disagrees with content-range - *this == inv_status); // Inv status can reflect internal state problem in libcurl + // *DEBUG: Comment out 'inv_status' test for [curl:bugs] #1420 testing. + *this == inv_status || // Inv status can reflect internal state problem in libcurl + *this == inv_cont_range); // Short data read disagrees with content-range } } // end namespace LLCore -- cgit v1.3