From 5611cb6d476540e6a1c654c1f9acdce2787b3505 Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Mon, 23 Apr 2012 16:19:39 -0400 Subject: Okay, imported the core-http library and got it compiling suspiciously easily. The unit/integration tests don't work yet as I'm still battling cmake/autobuild as usual but first milestone passed. --- indra/llcorehttp/_thread.h | 106 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 indra/llcorehttp/_thread.h (limited to 'indra/llcorehttp/_thread.h') diff --git a/indra/llcorehttp/_thread.h b/indra/llcorehttp/_thread.h new file mode 100644 index 0000000000..5960f0dcdb --- /dev/null +++ b/indra/llcorehttp/_thread.h @@ -0,0 +1,106 @@ +/** + * @file _thread.h + * @brief thread type abstraction + * + * $LicenseInfo:firstyear=2012&license=viewerlgpl$ + * Second Life Viewer Source Code + * Copyright (C) 2012, 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 + * License as published by the Free Software Foundation; + * version 2.1 of the License only. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * Linden Research, Inc., 945 Battery Street, San Francisco, CA 94111 USA + * $/LicenseInfo$ + */ + +#ifndef LLCOREINT_THREAD_H_ +#define LLCOREINT_THREAD_H_ + + +#include "_refcounted.h" + + +namespace LLCoreInt +{ + +class HttpThread : public RefCounted +{ +private: + HttpThread(); // Not defined + void operator=(const HttpThread &); // Not defined + + void at_exit() + { + // the thread function has exited so we need to release our reference + // to ourself so that we will be automagically cleaned up. + release(); + } + + void run() + { // 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. + boost::this_thread::at_thread_exit(boost::bind(&HttpThread::at_exit, this)); + + // run the thread function + mThreadFunc(this); + + } // THREAD CONTEXT + +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. + /// + explicit HttpThread(boost::function threadFunc) + : RefCounted(true), // implicit reference + mThreadFunc(threadFunc) + { + // this creates a boost thread that will call HttpThread::run on this instance + // and pass it the threadfunc callable... + boost::function f = boost::bind(&HttpThread::run, this); + + mThread = new boost::thread(f); + } + + virtual ~HttpThread() + { + delete mThread; + } + + inline void join() + { + mThread->join(); + } + + inline bool joinable() const + { + mThread->joinable(); + } + +private: + boost::function mThreadFunc; + boost::thread * mThread; +}; // end class HttpThread + +} // end namespace LLCoreInt + +#endif // LLCOREINT_THREAD_H_ + + -- cgit v1.3 From 88bc54192fa274bb1617801e4aeddbbf4e9e7944 Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Wed, 25 Apr 2012 13:18:27 -0400 Subject: Get Mac building. Unused variable in boost and missing return value which wasn't caught in other environments. --- indra/llcorehttp/CMakeLists.txt | 5 +++++ indra/llcorehttp/_thread.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) (limited to 'indra/llcorehttp/_thread.h') diff --git a/indra/llcorehttp/CMakeLists.txt b/indra/llcorehttp/CMakeLists.txt index 8d21007ca0..b3f2d8234c 100644 --- a/indra/llcorehttp/CMakeLists.txt +++ b/indra/llcorehttp/CMakeLists.txt @@ -58,6 +58,11 @@ set(llcorehttp_HEADER_FILES set_source_files_properties(${llcorehttp_HEADER_FILES} PROPERTIES HEADER_FILE_ONLY TRUE) +if (APPLE) + # Boost headers define unused members so... + set_source_files_properties(${llcorehttp_SOURCE_FILES} + PROPERTIES COMPILE_FLAGS -Wno-unused-variable) +endif (APPLE) list(APPEND llcorehttp_SOURCE_FILES ${llcorehttp_HEADER_FILES}) diff --git a/indra/llcorehttp/_thread.h b/indra/llcorehttp/_thread.h index 5960f0dcdb..0937d698c7 100644 --- a/indra/llcorehttp/_thread.h +++ b/indra/llcorehttp/_thread.h @@ -91,7 +91,7 @@ public: inline bool joinable() const { - mThread->joinable(); + return mThread->joinable(); } private: -- cgit v1.3 From 74d59e7128bb02a4b49af99e44f437a736a3f62b Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Mon, 7 May 2012 15:16:31 -0400 Subject: Build llcorehttp as part of a viewer dependency with unit tests. This required boost::thread and the easiest path to that was to go with the 1.48 Boost release in the 3P tree (eliminating a fork for a modified 1.45 packaging). One unit test, the most important one, is failing in test_httprequest but that can be attended to later. This test issues a GET to http://localhost:2/ and that is hitting the wire but the libcurl plumbing isn't delivering the failure, only the eventual timeout. An unexpected change in behavior. --- autobuild.xml | 12 ++--- indra/cmake/Boost.cmake | 23 ++++++---- indra/cmake/LLCoreHttp.cmake | 2 + indra/cmake/LLPrimitive.cmake | 8 ++-- indra/llcommon/linden_common.h | 4 ++ indra/llcorehttp/CMakeLists.txt | 30 ++++++------ indra/llcorehttp/_httpoprequest.cpp | 2 + indra/llcorehttp/_thread.h | 4 +- indra/llcorehttp/httpcommon.h | 2 + indra/llcorehttp/tests/all_test.cpp | 64 -------------------------- indra/llcorehttp/tests/llcorehttp_test.cpp | 66 +++++++++++++++++++++++++++ indra/llcorehttp/tests/test_bufferarray.hpp | 3 +- indra/llcorehttp/tests/test_httpheaders.hpp | 2 +- indra/llcorehttp/tests/test_httpoperation.hpp | 2 +- indra/llcorehttp/tests/test_httprequest.hpp | 12 ++--- indra/llcorehttp/tests/test_httpstatus.hpp | 2 +- indra/llcorehttp/tests/test_refcounted.hpp | 2 +- 17 files changed, 131 insertions(+), 109 deletions(-) delete mode 100644 indra/llcorehttp/tests/all_test.cpp create mode 100644 indra/llcorehttp/tests/llcorehttp_test.cpp (limited to 'indra/llcorehttp/_thread.h') diff --git a/autobuild.xml b/autobuild.xml index 9914be6867..5c9af29b99 100644 --- a/autobuild.xml +++ b/autobuild.xml @@ -186,9 +186,9 @@ archive hash - d98078791ce345bf6168ce9ba53ca2d7 + 0c4678ac85395f5f5294b63da1d79007 url - http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-boost/rev/222752/arch/Darwin/installer/boost-1.45.0-darwin-20110304.tar.bz2 + http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-boost/rev/249117/arch/Darwin/installer/boost-1.48.0-darwin-20120208.tar.bz2 name darwin @@ -198,9 +198,9 @@ archive hash - a34e7fffdb94a6a4d8a2966b1f216da3 + 848766eac189e0fa785f4a025532acd9 url - http://s3.amazonaws.com/viewer-source-downloads/install_pkgs/boost-1.45.0-linux-20110310.tar.bz2 + http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-boost/rev/249117/arch/Linux/installer/boost-1.48.0-linux-20120208.tar.bz2 name linux @@ -210,9 +210,9 @@ archive hash - 98be22c8833aa2bca184b9fa09fbb82b + cd1e60a00d40f4475ae5e0aca86f74c1 url - http://s3.amazonaws.com/viewer-source-downloads/install_pkgs/boost-1.45.0-windows-20110124.tar.bz2 + http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-boost/rev/249117/arch/CYGWIN/installer/boost-1.48.0-windows-20120208.tar.bz2 name windows diff --git a/indra/cmake/Boost.cmake b/indra/cmake/Boost.cmake index 2135f0584c..24de1fc0c2 100644 --- a/indra/cmake/Boost.cmake +++ b/indra/cmake/Boost.cmake @@ -12,12 +12,13 @@ if (STANDALONE) set(BOOST_SIGNALS_LIBRARY boost_signals-mt) set(BOOST_SYSTEM_LIBRARY boost_system-mt) set(BOOST_FILESYSTEM_LIBRARY boost_filesystem-mt) + set(BOOST_THREAD_LIBRARY boost_thread-mt) else (STANDALONE) use_prebuilt_binary(boost) set(Boost_INCLUDE_DIRS ${LIBS_PREBUILT_DIR}/include) if (WINDOWS) - set(BOOST_VERSION 1_45) + set(BOOST_VERSION 1_48) if(MSVC80) set(BOOST_PROGRAM_OPTIONS_LIBRARY optimized libboost_program_options-vc80-mt-${BOOST_VERSION} @@ -37,22 +38,26 @@ else (STANDALONE) else(MSVC80) # MSVC 10.0 config set(BOOST_PROGRAM_OPTIONS_LIBRARY - optimized libboost_program_options-vc100-mt-${BOOST_VERSION} - debug libboost_program_options-vc100-mt-gd-${BOOST_VERSION}) + optimized libboost_program_options-mt + debug libboost_program_options-mt-gd) set(BOOST_REGEX_LIBRARY - optimized libboost_regex-vc100-mt-${BOOST_VERSION} - debug libboost_regex-vc100-mt-gd-${BOOST_VERSION}) + optimized libboost_regex-mt + debug libboost_regex-mt-gd) set(BOOST_SYSTEM_LIBRARY - optimized libboost_system-vc100-mt-${BOOST_VERSION} - debug libboost_system-vc100-mt-gd-${BOOST_VERSION}) + optimized libboost_system-mt + debug libboost_system-mt-gd) set(BOOST_FILESYSTEM_LIBRARY - optimized libboost_filesystem-vc100-mt-${BOOST_VERSION} - debug libboost_filesystem-vc100-mt-gd-${BOOST_VERSION}) + optimized libboost_filesystem-mt + debug libboost_filesystem-mt-gd) + set(BOOST_THREAD_LIBRARY + optimized libboost_thread-mt + debug libboost_thread-mt-gd) endif (MSVC80) elseif (DARWIN OR LINUX) set(BOOST_PROGRAM_OPTIONS_LIBRARY boost_program_options) set(BOOST_REGEX_LIBRARY boost_regex) set(BOOST_SYSTEM_LIBRARY boost_system) set(BOOST_FILESYSTEM_LIBRARY boost_filesystem) + set(BOOST_THREAD_LIBRARY boost_thread) endif (WINDOWS) endif (STANDALONE) diff --git a/indra/cmake/LLCoreHttp.cmake b/indra/cmake/LLCoreHttp.cmake index 280189da3d..61e4b23d98 100644 --- a/indra/cmake/LLCoreHttp.cmake +++ b/indra/cmake/LLCoreHttp.cmake @@ -3,12 +3,14 @@ include(CARes) include(CURL) include(OpenSSL) +include(Boost) set(LLCOREHTTP_INCLUDE_DIRS ${LIBS_OPEN_DIR}/llcorehttp ${CARES_INCLUDE_DIRS} ${CURL_INCLUDE_DIRS} ${OPENSSL_INCLUDE_DIRS} + ${BOOST_INCLUDE_DIRS} ) set(LLCOREHTTP_LIBRARIES llcorehttp) diff --git a/indra/cmake/LLPrimitive.cmake b/indra/cmake/LLPrimitive.cmake index f15a2c2649..ab39cbb6be 100644 --- a/indra/cmake/LLPrimitive.cmake +++ b/indra/cmake/LLPrimitive.cmake @@ -15,10 +15,10 @@ if (WINDOWS) optimized llprimitive debug libcollada14dom22-d optimized libcollada14dom22 - debug libboost_filesystem-vc100-mt-gd-1_45 - optimized libboost_filesystem-vc100-mt-1_45 - debug libboost_system-vc100-mt-gd-1_45 - optimized libboost_system-vc100-mt-1_45 + debug libboost_filesystem-mt-gd + optimized libboost_filesystem-mt + debug libboost_system-mt-gd + optimized libboost_system-mt ) else (WINDOWS) set(LLPRIMITIVE_LIBRARIES diff --git a/indra/llcommon/linden_common.h b/indra/llcommon/linden_common.h index bdcc98e402..50db03cb6a 100644 --- a/indra/llcommon/linden_common.h +++ b/indra/llcommon/linden_common.h @@ -82,4 +82,8 @@ #include "llfile.h" #include "llformat.h" +// Boost 1.45 had version 2 as the default for the filesystem library, +// 1.48 has version 3 as the default. Keep compatibility for now. +#define BOOST_FILESYSTEM_VERSION 2 + #endif diff --git a/indra/llcorehttp/CMakeLists.txt b/indra/llcorehttp/CMakeLists.txt index e0d1817781..a950e9180c 100644 --- a/indra/llcorehttp/CMakeLists.txt +++ b/indra/llcorehttp/CMakeLists.txt @@ -73,23 +73,24 @@ target_link_libraries( ${CARES_LIBRARIES} ${OPENSSL_LIBRARIES} ${CRYPTO_LIBRARIES} + ${BOOST_THREAD_LIBRARY} ) # tests -#if (LL_TESTS) -if (LL_TESTS AND 0) +if (LL_TESTS) +#if (LL_TESTS AND 0) SET(llcorehttp_TEST_SOURCE_FILES - test_allocator.cpp + tests/test_allocator.cpp ) set(llcorehttp_TEST_HEADER_FILS - test_httpstatus.hpp - test_refcounted.hpp - test_httpoperation.hpp - test_httprequest.hpp - test_httprequestqueue.hpp - test_httpheaders.hpp - test_bufferarray.hpp + tests/test_httpstatus.hpp + tests/test_refcounted.hpp + tests/test_httpoperation.hpp + tests/test_httprequest.hpp + tests/test_httprequestqueue.hpp + tests/test_httpheaders.hpp + tests/test_bufferarray.hpp ) set_source_files_properties(${llcorehttp_TEST_HEADER_FILES} @@ -105,13 +106,16 @@ if (LL_TESTS AND 0) ${WINDOWS_LIBRARIES} ${LLCOMMON_LIBRARIES} ${GOOGLEMOCK_LIBRARIES} + ${CURL_LIBRARIES} + ${OPENSSL_LIBRARIES} + ${CRYPTO_LIBRARIES} ) - LL_ADD_INTEGRATION_TEST(all + LL_ADD_INTEGRATION_TEST(llcorehttp "${llcorehttp_TEST_SOURCE_FILES}" "${test_libs}" ) -#endif (LL_TESTS) -endif (LL_TESTS AND 0) +endif (LL_TESTS) +#endif (LL_TESTS AND 0) diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp index 316df8bd49..3c9eb71b9a 100644 --- a/indra/llcorehttp/_httpoprequest.cpp +++ b/indra/llcorehttp/_httpoprequest.cpp @@ -259,6 +259,8 @@ HttpStatus HttpOpRequest::prepareForGet(HttpService * service) HttpStatus status; mCurlHandle = curl_easy_init(); + curl_easy_setopt(mCurlHandle, CURLOPT_TIMEOUT, 30); + curl_easy_setopt(mCurlHandle, CURLOPT_CONNECTTIMEOUT, 30); curl_easy_setopt(mCurlHandle, CURLOPT_NOSIGNAL, 1); curl_easy_setopt(mCurlHandle, CURLOPT_NOPROGRESS, 1); curl_easy_setopt(mCurlHandle, CURLOPT_URL, mReqURL.c_str()); diff --git a/indra/llcorehttp/_thread.h b/indra/llcorehttp/_thread.h index 0937d698c7..46a333a749 100644 --- a/indra/llcorehttp/_thread.h +++ b/indra/llcorehttp/_thread.h @@ -27,9 +27,11 @@ #ifndef LLCOREINT_THREAD_H_ #define LLCOREINT_THREAD_H_ +#include +#include -#include "_refcounted.h" +#include "_refcounted.h" namespace LLCoreInt { diff --git a/indra/llcorehttp/httpcommon.h b/indra/llcorehttp/httpcommon.h index cd7c09f097..5fc497e720 100644 --- a/indra/llcorehttp/httpcommon.h +++ b/indra/llcorehttp/httpcommon.h @@ -93,6 +93,8 @@ /// /// +#include "linden_common.h" + #include diff --git a/indra/llcorehttp/tests/all_test.cpp b/indra/llcorehttp/tests/all_test.cpp deleted file mode 100644 index 636f6f8c05..0000000000 --- a/indra/llcorehttp/tests/all_test.cpp +++ /dev/null @@ -1,64 +0,0 @@ -/** - * @file test_all - * @brief Main test runner - * - * $LicenseInfo:firstyear=2012&license=viewerlgpl$ - * Second Life Viewer Source Code - * Copyright (C) 2012, 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 - * License as published by the Free Software Foundation; - * version 2.1 of the License only. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - * - * Linden Research, Inc., 945 Battery Street, San Francisco, CA 94111 USA - * $/LicenseInfo$ - */ - - -#include - -#include -#include - -#include - -// Pull in each of the test sets -#include "test_httpstatus.hpp" -#include "test_refcounted.hpp" -#include "test_httpoperation.hpp" -#include "test_httprequest.hpp" -#include "test_httpheaders.hpp" -#include "test_bufferarray.hpp" -#include "test_httprequestqueue.hpp" - - -namespace tut -{ - test_runner_singleton runner; -} - -int main() -{ - curl_global_init(CURL_GLOBAL_ALL); - - // *FIXME: Need threaded/SSL curl setup here. - - tut::reporter reporter; - - tut::runner.get().set_callback(&reporter); - tut::runner.get().run_tests(); - return !reporter.all_ok(); - - curl_global_cleanup(); -} - diff --git a/indra/llcorehttp/tests/llcorehttp_test.cpp b/indra/llcorehttp/tests/llcorehttp_test.cpp new file mode 100644 index 0000000000..ad596d61cc --- /dev/null +++ b/indra/llcorehttp/tests/llcorehttp_test.cpp @@ -0,0 +1,66 @@ +/** + * @file llcorehttp_test + * @brief Main test runner + * + * $LicenseInfo:firstyear=2012&license=viewerlgpl$ + * Second Life Viewer Source Code + * Copyright (C) 2012, 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 + * License as published by the Free Software Foundation; + * version 2.1 of the License only. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * Linden Research, Inc., 945 Battery Street, San Francisco, CA 94111 USA + * $/LicenseInfo$ + */ + + +#include + +#include +#include + +#include + +// Pull in each of the test sets +#include "test_httpstatus.hpp" +#include "test_refcounted.hpp" +#include "test_httpoperation.hpp" +#include "test_httprequest.hpp" +#include "test_httpheaders.hpp" +#include "test_bufferarray.hpp" +#include "test_httprequestqueue.hpp" + +#if 0 + +namespace tut +{ + test_runner_singleton runner; +} + +int main() +{ + curl_global_init(CURL_GLOBAL_ALL); + + // *FIXME: Need threaded/SSL curl setup here. + + tut::reporter reporter; + + tut::runner.get().set_callback(&reporter); + tut::runner.get().run_tests(); + return !reporter.all_ok(); + + curl_global_cleanup(); +} + +#endif diff --git a/indra/llcorehttp/tests/test_bufferarray.hpp b/indra/llcorehttp/tests/test_bufferarray.hpp index 4f5d0284a1..ecbb5ef250 100644 --- a/indra/llcorehttp/tests/test_bufferarray.hpp +++ b/indra/llcorehttp/tests/test_bufferarray.hpp @@ -26,7 +26,7 @@ #ifndef TEST_LLCORE_BUFFER_ARRAY_H_ #define TEST_LLCORE_BUFFER_ARRAY_H_ -#include +#include "bufferarray.h" #include @@ -183,7 +183,6 @@ void BufferArrayTestObjectType::test<4>() char str1[] = "abcdefghij"; size_t str1_len(strlen(str1)); char str2[] = "ABCDEFGHIJ"; - size_t str2_len(strlen(str2)); char buffer[256]; size_t len = ba->write(str1, str1_len); diff --git a/indra/llcorehttp/tests/test_httpheaders.hpp b/indra/llcorehttp/tests/test_httpheaders.hpp index d22b516691..ce0d19b058 100644 --- a/indra/llcorehttp/tests/test_httpheaders.hpp +++ b/indra/llcorehttp/tests/test_httpheaders.hpp @@ -26,7 +26,7 @@ #ifndef TEST_LLCORE_HTTP_HEADERS_H_ #define TEST_LLCORE_HTTP_HEADERS_H_ -#include +#include "httpheaders.h" #include diff --git a/indra/llcorehttp/tests/test_httpoperation.hpp b/indra/llcorehttp/tests/test_httpoperation.hpp index c9feaddb2a..6c3df1e9e3 100644 --- a/indra/llcorehttp/tests/test_httpoperation.hpp +++ b/indra/llcorehttp/tests/test_httpoperation.hpp @@ -27,7 +27,7 @@ #define TEST_LLCORE_HTTP_OPERATION_H_ #include "_httpoperation.h" -#include +#include "httphandler.h" #include diff --git a/indra/llcorehttp/tests/test_httprequest.hpp b/indra/llcorehttp/tests/test_httprequest.hpp index df5640859f..ab25a2eb1a 100644 --- a/indra/llcorehttp/tests/test_httprequest.hpp +++ b/indra/llcorehttp/tests/test_httprequest.hpp @@ -26,11 +26,11 @@ #ifndef TEST_LLCORE_HTTP_REQUEST_H_ #define TEST_LLCORE_HTTP_REQUEST_H_ -#include -#include -#include -#include -#include +#include "httprequest.h" +#include "httphandler.h" +#include "httpresponse.h" +#include "_httpservice.h" +#include "_httprequestqueue.h" #include @@ -360,7 +360,7 @@ void HttpRequestTestObjectType::test<5>() mStatus = HttpStatus(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_CONNECT); HttpHandle handle = req->requestGetByteRange(HttpRequest::DEFAULT_POLICY_ID, 0.0f, - "http://localhost:2/nothing/here", + "http://127.0.0.1:2/nothing/here", 0, 0, NULL, diff --git a/indra/llcorehttp/tests/test_httpstatus.hpp b/indra/llcorehttp/tests/test_httpstatus.hpp index fa5803a17c..38bf494dec 100644 --- a/indra/llcorehttp/tests/test_httpstatus.hpp +++ b/indra/llcorehttp/tests/test_httpstatus.hpp @@ -27,7 +27,7 @@ #ifndef TEST_HTTP_STATUS_H_ #define TEST_HTTP_STATUS_H_ -#include +#include "httpcommon.h" #include #include diff --git a/indra/llcorehttp/tests/test_refcounted.hpp b/indra/llcorehttp/tests/test_refcounted.hpp index 6a17947288..cb4b50287a 100644 --- a/indra/llcorehttp/tests/test_refcounted.hpp +++ b/indra/llcorehttp/tests/test_refcounted.hpp @@ -26,7 +26,7 @@ #ifndef TEST_LLCOREINT_REF_COUNTED_H_ #define TEST_LLCOREINT_REF_COUNTED_H_ -#include <_refcounted.h> +#include "_refcounted.h" #include "test_allocator.h" -- cgit v1.3 From e172ec84fa217aae8d1e51c1e0673322c30891fe Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Sat, 23 Jun 2012 23:33:50 -0400 Subject: SH-3184/SH-3221 Improve cleanup, destructor, thread termination, etc. logic in library. With this commit, the cleanup paths should be production quality. Unit tests have been expanded to include cases requiring thread termination and cleanup by the worker thread. Special operation/request added to support the unit tests. Thread interface expanded to include a very aggressive cancel() method that does not do cleanup but prevents the thread from accessing objects that will be destroyed. --- indra/llcorehttp/_httplibcurl.cpp | 97 +++++++++++++----- indra/llcorehttp/_httplibcurl.h | 16 +-- indra/llcorehttp/_httpoperation.cpp | 36 +++++++ indra/llcorehttp/_httpoperation.h | 25 +++++ indra/llcorehttp/_httppolicy.cpp | 15 ++- indra/llcorehttp/_httppolicy.h | 15 ++- indra/llcorehttp/_httprequestqueue.h | 6 +- indra/llcorehttp/_httpservice.cpp | 75 ++++++++++---- indra/llcorehttp/_httpservice.h | 11 +- indra/llcorehttp/_thread.h | 20 +++- indra/llcorehttp/httprequest.cpp | 43 ++++++-- indra/llcorehttp/httprequest.h | 9 ++ indra/llcorehttp/tests/test_httprequest.hpp | 154 +++++++++++++++++++++++++++- 13 files changed, 447 insertions(+), 75 deletions(-) (limited to 'indra/llcorehttp/_thread.h') diff --git a/indra/llcorehttp/_httplibcurl.cpp b/indra/llcorehttp/_httplibcurl.cpp index 45e16d420e..bc8b3cc9be 100644 --- a/indra/llcorehttp/_httplibcurl.cpp +++ b/indra/llcorehttp/_httplibcurl.cpp @@ -47,12 +47,19 @@ HttpLibcurl::HttpLibcurl(HttpService * service) HttpLibcurl::~HttpLibcurl() { - // *FIXME: need to cancel requests in this class, not in op class. + shutdown(); + + mService = NULL; +} + + +void HttpLibcurl::shutdown() +{ while (! mActiveOps.empty()) { active_set_t::iterator item(mActiveOps.begin()); - (*item)->cancel(); + cancelRequest(*item); (*item)->release(); mActiveOps.erase(item); } @@ -63,8 +70,6 @@ HttpLibcurl::~HttpLibcurl() { if (mMultiHandles[policy_class]) { - // *FIXME: Do some multi cleanup here first - curl_multi_cleanup(mMultiHandles[policy_class]); mMultiHandles[policy_class] = 0; } @@ -73,20 +78,12 @@ HttpLibcurl::~HttpLibcurl() delete [] mMultiHandles; mMultiHandles = NULL; } - - mService = NULL; -} - -void HttpLibcurl::init() -{} - - -void HttpLibcurl::term() -{} + mPolicyCount = 0; +} -void HttpLibcurl::setPolicyCount(int policy_count) +void HttpLibcurl::start(int policy_count) { llassert_always(policy_count <= POLICY_CLASS_LIMIT); llassert_always(! mMultiHandles); // One-time call only @@ -143,8 +140,9 @@ HttpService::ELoopSpeed HttpLibcurl::processTransport() } else { - // *FIXME: Issue a logging event for this. - ; + LL_WARNS_ONCE("CoreHttp") << "Unexpected message from libcurl. Msg code: " + << msg->msg + << LL_ENDL; } msgs_in_queue = 0; } @@ -191,30 +189,61 @@ void HttpLibcurl::addOp(HttpOpRequest * op) } +// *NOTE: cancelRequest logic parallels completeRequest logic. +// Keep them synchronized as necessary. Caller is expected to +// remove to op from the active list and release the op *after* +// calling this method. It must be called first to deliver the +// op to the reply queue with refcount intact. +void HttpLibcurl::cancelRequest(HttpOpRequest * op) +{ + // Deactivate request + op->mCurlActive = false; + + // Detach from multi and recycle handle + curl_multi_remove_handle(mMultiHandles[op->mReqPolicy], op->mCurlHandle); + curl_easy_cleanup(op->mCurlHandle); + op->mCurlHandle = NULL; + + // Tracing + if (op->mTracing > TRACE_OFF) + { + LL_INFOS("CoreHttp") << "TRACE, RequestCanceled, Handle: " + << static_cast(op) + << ", Status: " << op->mStatus.toHex() + << LL_ENDL; + } + + // Cancel op and deliver for notification + op->cancel(); +} + + +// *NOTE: cancelRequest logic parallels completeRequest logic. +// Keep them synchronized as necessary. bool HttpLibcurl::completeRequest(CURLM * multi_handle, CURL * handle, CURLcode status) { HttpOpRequest * op(NULL); curl_easy_getinfo(handle, CURLINFO_PRIVATE, &op); - // *FIXME: check the pointer if (handle != op->mCurlHandle || ! op->mCurlActive) { - // *FIXME: This is a sanity check that needs validation/termination. - ; + LL_WARNS("CoreHttp") << "libcurl handle and HttpOpRequest handle in disagreement or inactive request." + << " Handle: " << static_cast(handle) + << LL_ENDL; + return false; } active_set_t::iterator it(mActiveOps.find(op)); if (mActiveOps.end() == it) { - // *FIXME: Fatal condition. This must be here. - ; - } - else - { - mActiveOps.erase(it); + LL_WARNS("CoreHttp") << "libcurl completion for request not on active list. Continuing." + << " Handle: " << static_cast(handle) + << LL_ENDL; + return false; } // Deactivate request + mActiveOps.erase(it); op->mCurlActive = false; // Set final status of request if it hasn't failed by other mechanisms yet @@ -258,9 +287,21 @@ int HttpLibcurl::getActiveCount() const } -int HttpLibcurl::getActiveCountInClass(int /* policy_class */) const +int HttpLibcurl::getActiveCountInClass(int policy_class) const { - return getActiveCount(); + int count(0); + + for (active_set_t::const_iterator iter(mActiveOps.begin()); + mActiveOps.end() != iter; + ++iter) + { + if ((*iter)->mReqPolicy == policy_class) + { + ++count; + } + } + + return count; } diff --git a/indra/llcorehttp/_httplibcurl.h b/indra/llcorehttp/_httplibcurl.h index 5e1dd1bfbf..69f7bb2b6d 100644 --- a/indra/llcorehttp/_httplibcurl.h +++ b/indra/llcorehttp/_httplibcurl.h @@ -65,9 +65,6 @@ private: void operator=(const HttpLibcurl &); // Not defined public: - static void init(); - static void term(); - /// Give cycles to libcurl to run active requests. Completed /// operations (successful or failed) will be retried or handed /// over to the reply queue as final responses. @@ -79,16 +76,23 @@ public: void addOp(HttpOpRequest * op); /// One-time call to set the number of policy classes to be - /// serviced and to create the resources for each. - void setPolicyCount(int policy_count); + /// serviced and to create the resources for each. Value + /// must agree with HttpPolicy::setPolicies() call. + void start(int policy_count); + + void shutdown(); int getActiveCount() const; int getActiveCountInClass(int policy_class) const; - + protected: /// Invoked when libcurl has indicated a request has been processed /// to completion and we need to move the request to a new state. bool completeRequest(CURLM * multi_handle, CURL * handle, CURLcode status); + + /// Invoked to cancel an active request, mainly during shutdown + /// and destroy. + void cancelRequest(HttpOpRequest * op); protected: typedef std::set active_set_t; diff --git a/indra/llcorehttp/_httpoperation.cpp b/indra/llcorehttp/_httpoperation.cpp index 0d9553434e..b35ab79d65 100644 --- a/indra/llcorehttp/_httpoperation.cpp +++ b/indra/llcorehttp/_httpoperation.cpp @@ -208,4 +208,40 @@ void HttpOpNull::stageFromRequest(HttpService * service) } +// ================================== +// HttpOpSpin +// ================================== + + +HttpOpSpin::HttpOpSpin(int mode) + : HttpOperation(), + mMode(mode) +{} + + +HttpOpSpin::~HttpOpSpin() +{} + + +void HttpOpSpin::stageFromRequest(HttpService * service) +{ + if (0 == mMode) + { + // Spin forever + while (true) + { + ms_sleep(100); + } + } + else + { + this->addRef(); + if (! HttpRequestQueue::instanceOf()->addOp(this)) + { + this->release(); + } + } +} + + } // end namespace LLCore diff --git a/indra/llcorehttp/_httpoperation.h b/indra/llcorehttp/_httpoperation.h index 5823c08c7b..717a9b0d72 100644 --- a/indra/llcorehttp/_httpoperation.h +++ b/indra/llcorehttp/_httpoperation.h @@ -170,6 +170,31 @@ public: }; // end class HttpOpNull +/// HttpOpSpin is a test-only request that puts the worker +/// thread into a cpu spin. Used for unit tests and cleanup +/// evaluation. You do not want to use this. +class HttpOpSpin : public HttpOperation +{ +public: + // 0 does a hard spin in the operation + // 1 does a soft spin continuously requeuing itself + HttpOpSpin(int mode); + +protected: + virtual ~HttpOpSpin(); + +private: + HttpOpSpin(const HttpOpSpin &); // Not defined + void operator=(const HttpOpSpin &); // Not defined + +public: + virtual void stageFromRequest(HttpService *); + +protected: + int mMode; +}; // end class HttpOpSpin + + } // end namespace LLCore #endif // _LLCORE_HTTP_OPERATION_H_ diff --git a/indra/llcorehttp/_httppolicy.cpp b/indra/llcorehttp/_httppolicy.cpp index 1dae20add6..93e295537c 100644 --- a/indra/llcorehttp/_httppolicy.cpp +++ b/indra/llcorehttp/_httppolicy.cpp @@ -75,6 +75,14 @@ HttpPolicy::HttpPolicy(HttpService * service) HttpPolicy::~HttpPolicy() +{ + shutdown(); + + mService = NULL; +} + + +void HttpPolicy::shutdown() { for (int policy_class(0); policy_class < mActiveClasses; ++policy_class) { @@ -100,12 +108,12 @@ HttpPolicy::~HttpPolicy() } delete [] mState; mState = NULL; - mService = NULL; + mActiveClasses = 0; } -void HttpPolicy::setPolicies(const HttpPolicyGlobal & global, - const std::vector & classes) +void HttpPolicy::start(const HttpPolicyGlobal & global, + const std::vector & classes) { llassert_always(! mState); @@ -244,6 +252,7 @@ bool HttpPolicy::changePriority(HttpHandle handle, HttpRequest::priority_t prior return false; } + bool HttpPolicy::stageAfterCompletion(HttpOpRequest * op) { static const HttpStatus cant_connect(HttpStatus::EXT_CURL_EASY, CURLE_COULDNT_CONNECT); diff --git a/indra/llcorehttp/_httppolicy.h b/indra/llcorehttp/_httppolicy.h index c93279bc83..90bb3b571d 100644 --- a/indra/llcorehttp/_httppolicy.h +++ b/indra/llcorehttp/_httppolicy.h @@ -60,6 +60,17 @@ private: void operator=(const HttpPolicy &); // Not defined public: + /// Cancel all ready and retry requests sending them to + /// their notification queues. Release state resources + /// making further request handling impossible. + void shutdown(); + + /// Deliver policy definitions and enable handling of + /// requests. One-time call invoked before starting + /// the worker thread. + void start(const HttpPolicyGlobal & global, + const std::vector & classes); + /// Give the policy layer some cycles to scan the ready /// queue promoting higher-priority requests to active /// as permited. @@ -98,10 +109,6 @@ public: return mGlobalOptions; } - void setPolicies(const HttpPolicyGlobal & global, - const std::vector & classes); - - // Get ready counts for a particular class int getReadyCount(HttpRequest::policy_t policy_class); diff --git a/indra/llcorehttp/_httprequestqueue.h b/indra/llcorehttp/_httprequestqueue.h index 6e8f00c4da..e11fd17c90 100644 --- a/indra/llcorehttp/_httprequestqueue.h +++ b/indra/llcorehttp/_httprequestqueue.h @@ -73,11 +73,15 @@ public: public: typedef std::vector OpContainer; - /// Insert an object at the back of the reply queue. + /// Insert an object at the back of the request queue. /// /// Caller must provide one refcount to the queue which takes /// possession of the count. /// + /// @return Standard status. On failure, caller + /// must dispose of the operation with + /// an explicit release() call. + /// /// Threading: callable by any thread. HttpStatus addOp(HttpOperation * op); diff --git a/indra/llcorehttp/_httpservice.cpp b/indra/llcorehttp/_httpservice.cpp index 9c5c7bf9b4..afbab2ab71 100644 --- a/indra/llcorehttp/_httpservice.cpp +++ b/indra/llcorehttp/_httpservice.cpp @@ -53,35 +53,43 @@ HttpService::HttpService() mPolicy(NULL), mTransport(NULL) { + // Create the default policy class HttpPolicyClass pol_class; pol_class.set(HttpRequest::CP_CONNECTION_LIMIT, DEFAULT_CONNECTIONS); pol_class.set(HttpRequest::CP_PER_HOST_CONNECTION_LIMIT, DEFAULT_CONNECTIONS); pol_class.set(HttpRequest::CP_ENABLE_PIPELINING, 0L); - mPolicyClasses.push_back(pol_class); } HttpService::~HttpService() { + mExitRequested = true; + if (RUNNING == sState) + { + // Trying to kill the service object with a running thread + // is a bit tricky. + if (mThread) + { + mThread->cancel(); + + if (! mThread->timedJoin(2000)) + { + // Failed to join, expect problems ahead... + LL_WARNS("CoreHttp") << "Destroying HttpService with running thread. Expect problems." + << LL_ENDL; + } + } + } + if (mRequestQueue) { mRequestQueue->release(); mRequestQueue = NULL; } - if (mPolicy) - { - // *TODO: need a finalization here - ; - } - - if (mTransport) - { - // *TODO: need a finalization here - delete mTransport; - mTransport = NULL; - } + delete mTransport; + mTransport = NULL; delete mPolicy; mPolicy = NULL; @@ -110,9 +118,22 @@ void HttpService::init(HttpRequestQueue * queue) void HttpService::term() { - llassert_always(RUNNING != sState); if (sInstance) { + if (RUNNING == sState) + { + // 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; + + // And a little sleep + ms_sleep(1000); + + // Dtor will make some additional efforts and issue any final + // warnings... + } + delete sInstance; sInstance = NULL; } @@ -159,9 +180,9 @@ void HttpService::startThread() mThread->release(); } - // Push current policy definitions - mPolicy->setPolicies(mPolicyGlobal, mPolicyClasses); - mTransport->setPolicyCount(mPolicyClasses.size()); + // 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 @@ -174,6 +195,7 @@ void HttpService::stopRequested() mExitRequested = true; } + bool HttpService::changePriority(HttpHandle handle, HttpRequest::priority_t priority) { bool found(false); @@ -191,9 +213,26 @@ bool HttpService::changePriority(HttpHandle handle, HttpRequest::priority_t prio void HttpService::shutdown() { + // Disallow future enqueue of requests mRequestQueue->stopQueue(); - // *FIXME: Run down everything.... + // Cancel requests alread on the request queue + HttpRequestQueue::OpContainer ops; + mRequestQueue->fetchAll(false, ops); + while (! ops.empty()) + { + HttpOperation * op(ops.front()); + ops.erase(ops.begin()); + + op->cancel(); + op->release(); + } + + // Shutdown transport canceling requests, freeing resources + mTransport->shutdown(); + + // And now policy + mPolicy->shutdown(); } diff --git a/indra/llcorehttp/_httpservice.h b/indra/llcorehttp/_httpservice.h index 43044d97c0..a74235c475 100644 --- a/indra/llcorehttp/_httpservice.h +++ b/indra/llcorehttp/_httpservice.h @@ -134,7 +134,7 @@ public: /// acquires its weaknesses. static bool isStopped(); - /// Threading: callable by application thread *once*. + /// Threading: callable by consumer thread *once*. void startThread(); /// Threading: callable by worker thread. @@ -152,23 +152,28 @@ public: /// Threading: callable by worker thread. bool changePriority(HttpHandle handle, HttpRequest::priority_t priority); + /// Threading: callable by worker thread. HttpPolicy & getPolicy() { return *mPolicy; } + /// Threading: callable by worker thread. HttpLibcurl & getTransport() { return *mTransport; } + /// Threading: callable by consumer thread. HttpPolicyGlobal & getGlobalOptions() { return mPolicyGlobal; } + /// Threading: callable by consumer thread. HttpRequest::policy_t createPolicyClass(); + /// Threading: callable by consumer thread. HttpPolicyClass & getClassOptions(HttpRequest::policy_t policy_class) { llassert(policy_class >= 0 && policy_class < mPolicyClasses.size()); @@ -187,9 +192,9 @@ protected: static volatile EState sState; HttpRequestQueue * mRequestQueue; volatile bool mExitRequested; - - // === calling-thread-only data === LLCoreInt::HttpThread * mThread; + + // === consumer-thread-only data === HttpPolicyGlobal mPolicyGlobal; std::vector mPolicyClasses; diff --git a/indra/llcorehttp/_thread.h b/indra/llcorehttp/_thread.h index 46a333a749..4cf35055e9 100644 --- a/indra/llcorehttp/_thread.h +++ b/indra/llcorehttp/_thread.h @@ -27,9 +27,11 @@ #ifndef LLCOREINT_THREAD_H_ #define LLCOREINT_THREAD_H_ +#include "linden_common.h" + #include #include - +#include #include "_refcounted.h" @@ -91,11 +93,27 @@ public: mThread->join(); } + inline bool timedJoin(S32 millis) + { + return mThread->timed_join(boost::posix_time::milliseconds(millis)); + } + inline bool joinable() const { return mThread->joinable(); } + // A very hostile method to force a thread to quit + inline void cancel() + { + boost::thread::native_handle_type thread(mThread->native_handle()); +#if LL_WINDOWS + TerminateThread(thread, 0); +#else + pthread_cancel(thread); +#endif + } + private: boost::function mThreadFunc; boost::thread * mThread; diff --git a/indra/llcorehttp/httprequest.cpp b/indra/llcorehttp/httprequest.cpp index a525d8f9ea..3a55a849b9 100644 --- a/indra/llcorehttp/httprequest.cpp +++ b/indra/llcorehttp/httprequest.cpp @@ -370,11 +370,13 @@ HttpStatus HttpRequest::createService() { HttpStatus status; - llassert_always(! has_inited); - HttpRequestQueue::init(); - HttpRequestQueue * rq = HttpRequestQueue::instanceOf(); - HttpService::init(rq); - has_inited = true; + if (! has_inited) + { + HttpRequestQueue::init(); + HttpRequestQueue * rq = HttpRequestQueue::instanceOf(); + HttpService::init(rq); + has_inited = true; + } return status; } @@ -384,10 +386,12 @@ HttpStatus HttpRequest::destroyService() { HttpStatus status; - llassert_always(has_inited); - HttpService::term(); - HttpRequestQueue::term(); - has_inited = false; + if (has_inited) + { + HttpService::term(); + HttpRequestQueue::term(); + has_inited = false; + } return status; } @@ -423,6 +427,27 @@ HttpHandle HttpRequest::requestStopThread(HttpHandler * user_handler) return handle; } + +HttpHandle HttpRequest::requestSpin(int mode) +{ + HttpStatus status; + HttpHandle handle(LLCORE_HTTP_HANDLE_INVALID); + + HttpOpSpin * op = new HttpOpSpin(mode); + op->setReplyPath(mReplyQueue, NULL); + if (! (status = mRequestQueue->addOp(op))) // transfers refcount + { + op->release(); + mLastReqStatus = status; + return handle; + } + + mLastReqStatus = status; + handle = static_cast(op); + + return handle; +} + // ==================================== // Dynamic Policy Methods // ==================================== diff --git a/indra/llcorehttp/httprequest.h b/indra/llcorehttp/httprequest.h index 24fff24b83..134a61b618 100644 --- a/indra/llcorehttp/httprequest.h +++ b/indra/llcorehttp/httprequest.h @@ -409,6 +409,15 @@ public: /// HttpHandle requestStopThread(HttpHandler * handler); + /// Queue a Spin request. + /// DEBUG/TESTING ONLY. This puts the worker into a CPU spin for + /// test purposes. + /// + /// @param mode 0 for hard spin, 1 for soft spin + /// @return Standard handle return cases. + /// + HttpHandle requestSpin(int mode); + /// @} /// @name DynamicPolicyMethods diff --git a/indra/llcorehttp/tests/test_httprequest.hpp b/indra/llcorehttp/tests/test_httprequest.hpp index b09db6db28..ed049aa09c 100644 --- a/indra/llcorehttp/tests/test_httprequest.hpp +++ b/indra/llcorehttp/tests/test_httprequest.hpp @@ -1230,10 +1230,158 @@ 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. template <> template <> -void HttpRequestTestObjectType::test<12>() +void HttpRequestTestObjectType::test<14>() { ScopedCurlInit ready; @@ -1352,7 +1500,9 @@ void HttpRequestTestObjectType::test<12>() throw; } } - +// *NOTE: This test ^^^^^^^^ must be the last one in the set. It uses a +// sleeping service that interferes with other HTTP tests. Keep it +// last until that little HTTP server can get some attention... } // end namespace tut -- cgit v1.3 From e8b0088d1a0c02bfa1f9768dc91fc3df4322adae Mon Sep 17 00:00:00 2001 From: Monty Brandenberg Date: Tue, 26 Jun 2012 12:28:58 -0400 Subject: 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(-) (limited to 'indra/llcorehttp/_thread.h') 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 +#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 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 @@ -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. -- cgit v1.3