From 5e74293b0cfe562e8e3184217f27b3e86afe5277 Mon Sep 17 00:00:00 2001 From: Xiaohong Bao Date: Fri, 3 Feb 2012 22:01:46 -0700 Subject: fix for SH-2941: crash at LLCurl::Easy::~Easy() --- indra/llmessage/llcurl.cpp | 9 +++++++-- indra/llmessage/llcurl.h | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) (limited to 'indra/llmessage') diff --git a/indra/llmessage/llcurl.cpp b/indra/llmessage/llcurl.cpp index 3bcaffc275..2ef43d9c00 100644 --- a/indra/llmessage/llcurl.cpp +++ b/indra/llmessage/llcurl.cpp @@ -583,10 +583,10 @@ LLCurl::Multi::Multi(F32 idle_time_out) LLCurl::Multi::~Multi() { - cleanup() ; + cleanup(true) ; } -void LLCurl::Multi::cleanup() +void LLCurl::Multi::cleanup(bool deleted) { if(!mCurlMultiHandle) { @@ -599,6 +599,11 @@ void LLCurl::Multi::cleanup() { Easy* easy = *iter; check_curl_multi_code(curl_multi_remove_handle(mCurlMultiHandle, easy->getCurlHandle())); + + if(deleted) + { + easy->mResponder = NULL ; //avoid triggering mResponder. + } delete easy; } mEasyActiveList.clear(); diff --git a/indra/llmessage/llcurl.h b/indra/llmessage/llcurl.h index fd664c0fa1..579ba0b1ab 100644 --- a/indra/llmessage/llcurl.h +++ b/indra/llmessage/llcurl.h @@ -318,7 +318,7 @@ public: private: void easyFree(LLCurl::Easy*); - void cleanup() ; + void cleanup(bool deleted = false) ; CURLM* mCurlMultiHandle; -- cgit v1.3 From 88edb8da4e0c2610a85777c824bff67d645f716c Mon Sep 17 00:00:00 2001 From: Xiaohong Bao Date: Thu, 8 Mar 2012 17:58:25 -0700 Subject: fix for MAINT-701: crash at LLCurl::Multi::markDead() reviewed by davep --- indra/llmessage/llcurl.cpp | 48 ++++++++++++++++++++++++++++++---------------- indra/llmessage/llcurl.h | 3 ++- 2 files changed, 34 insertions(+), 17 deletions(-) (limited to 'indra/llmessage') diff --git a/indra/llmessage/llcurl.cpp b/indra/llmessage/llcurl.cpp index 2ef43d9c00..bac3e6c426 100644 --- a/indra/llmessage/llcurl.cpp +++ b/indra/llmessage/llcurl.cpp @@ -548,6 +548,7 @@ LLCurl::Multi::Multi(F32 idle_time_out) mErrorCount(0), mState(STATE_READY), mDead(FALSE), + mValid(TRUE), mMutexp(NULL), mDeletionMutexp(NULL), mEasyMutexp(NULL) @@ -584,6 +585,9 @@ LLCurl::Multi::Multi(F32 idle_time_out) LLCurl::Multi::~Multi() { cleanup(true) ; + + delete mDeletionMutexp ; + mDeletionMutexp = NULL ; } void LLCurl::Multi::cleanup(bool deleted) @@ -592,7 +596,10 @@ void LLCurl::Multi::cleanup(bool deleted) { return ; //nothing to clean. } + llassert_always(deleted || !mValid) ; + LLMutexLock lock(mDeletionMutexp); + // Clean up active for(easy_active_list_t::iterator iter = mEasyActiveList.begin(); iter != mEasyActiveList.end(); ++iter) @@ -615,14 +622,12 @@ void LLCurl::Multi::cleanup(bool deleted) check_curl_multi_code(LLCurl::deleteMultiHandle(mCurlMultiHandle)); mCurlMultiHandle = NULL ; - + delete mMutexp ; mMutexp = NULL ; - delete mDeletionMutexp ; - mDeletionMutexp = NULL ; delete mEasyMutexp ; mEasyMutexp = NULL ; - + mQueued = 0 ; mState = STATE_COMPLETED; @@ -649,10 +654,20 @@ void LLCurl::Multi::unlock() void LLCurl::Multi::markDead() { - LLMutexLock lock(mDeletionMutexp) ; + { + LLMutexLock lock(mDeletionMutexp) ; + + if(mCurlMultiHandle != NULL) + { + mDead = TRUE ; + LLCurl::getCurlThread()->setPriority(mHandle, LLQueuedThread::PRIORITY_URGENT) ; + + return; + } + } - mDead = TRUE ; - LLCurl::getCurlThread()->setPriority(mHandle, LLQueuedThread::PRIORITY_URGENT) ; + //not valid, delete it. + delete this; } void LLCurl::Multi::setState(LLCurl::Multi::ePerformState state) @@ -746,10 +761,14 @@ bool LLCurl::Multi::doPerform() setState(STATE_COMPLETED) ; mIdleTimer.reset() ; } - else if(mIdleTimer.getElapsedTimeF32() > mIdleTimeOut) //idle for too long, remove it. + else if(!mValid && mIdleTimer.getElapsedTimeF32() > mIdleTimeOut) //idle for too long, remove it. { dead = true ; } + else if(mValid && mIdleTimer.getElapsedTimeF32() > mIdleTimeOut - 1.f) //idle for too long, mark it invalid. + { + mValid = FALSE ; + } return dead ; } @@ -971,14 +990,7 @@ void LLCurlThread::killMulti(LLCurl::Multi* multi) return ; } - if(multi->isValid()) - { - multi->markDead() ; - } - else - { - deleteMulti(multi) ; - } + multi->markDead() ; } //private @@ -997,6 +1009,10 @@ void LLCurlThread::deleteMulti(LLCurl::Multi* multi) void LLCurlThread::cleanupMulti(LLCurl::Multi* multi) { multi->cleanup() ; + if(multi->isDead()) //check if marked dead during cleaning up. + { + deleteMulti(multi) ; + } } //------------------------------------------------------------ diff --git a/indra/llmessage/llcurl.h b/indra/llmessage/llcurl.h index 579ba0b1ab..d6a7714d4c 100644 --- a/indra/llmessage/llcurl.h +++ b/indra/llmessage/llcurl.h @@ -304,7 +304,7 @@ public: ePerformState getState() ; bool isCompleted() ; - bool isValid() {return mCurlMultiHandle != NULL ;} + bool isValid() {return mCurlMultiHandle != NULL && mValid;} bool isDead() {return mDead;} bool waitToComplete() ; @@ -333,6 +333,7 @@ private: ePerformState mState; BOOL mDead ; + BOOL mValid ; LLMutex* mMutexp ; LLMutex* mDeletionMutexp ; LLMutex* mEasyMutexp ; -- cgit v1.3 From 10ab4adc86207f86df30ab23d8858c23e7f550ea Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Feb 2012 13:44:43 -0500 Subject: Fix llprocess_test.cpp's exception catching for Linux. In the course of re-enabling the indra/test tests last year, Log generalized a workaround I'd introduced in llsdmessage_test.cpp. In Linux viewer land, a test program trying to catch an expected exception can't seem to catch it by its specific class (across the libllcommon.so boundary), but must instead catch std::runtime_error and validate the typeid().name() string. Log added a macro for this idiom in llevents_tut.cpp. Generalize that macro further for normal-case processing as well, move it to a header file of its own and use it in all known places -- plus the new exception-catching tests in llprocess_test.cpp. --- indra/llcommon/tests/llprocess_test.cpp | 6 +-- indra/llmessage/tests/llsdmessage_test.cpp | 36 ++----------- indra/test/catch_and_store_what_in.h | 86 ++++++++++++++++++++++++++++++ indra/test/llevents_tut.cpp | 69 +++--------------------- 4 files changed, 98 insertions(+), 99 deletions(-) create mode 100644 indra/test/catch_and_store_what_in.h (limited to 'indra/llmessage') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 06f9f3827f..a901c577d6 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -31,6 +31,7 @@ #include "../test/lltut.h" #include "../test/manageapr.h" #include "../test/namedtempfile.h" +#include "../test/catch_and_store_what_in.h" #include "stringize.h" #include "llsdutil.h" #include "llevents.h" @@ -952,10 +953,7 @@ namespace tut { \ CODE; \ } \ - catch (const EXCEPTION& e) \ - { \ - (THREW) = e.what(); \ - } \ + CATCH_AND_STORE_WHAT_IN(THREW, EXCEPTION) \ ensure("failed to throw " #EXCEPTION ": " #CODE, ! (THREW).empty()); \ } while (0) diff --git a/indra/llmessage/tests/llsdmessage_test.cpp b/indra/llmessage/tests/llsdmessage_test.cpp index 0f2c069303..6871ac0d52 100644 --- a/indra/llmessage/tests/llsdmessage_test.cpp +++ b/indra/llmessage/tests/llsdmessage_test.cpp @@ -42,6 +42,7 @@ // external library headers // other Linden headers #include "../test/lltut.h" +#include "../test/catch_and_store_what_in.h" #include "llsdserialize.h" #include "llevents.h" #include "stringize.h" @@ -72,43 +73,14 @@ namespace tut template<> template<> void llsdmessage_object::test<1>() { - bool threw = false; + std::string threw; // This should fail... try { LLSDMessage localListener; } - catch (const LLEventPump::DupPumpName&) - { - threw = true; - } - catch (const std::runtime_error& ex) - { - // This clause is because on Linux, on the viewer side, for this - // one test program (though not others!), the - // LLEventPump::DupPumpName exception isn't caught by the clause - // above. Warn the user... - std::cerr << "Failed to catch " << typeid(ex).name() << std::endl; - // But if the expected exception was thrown, allow the test to - // succeed anyway. Not sure how else to handle this odd case. - if (std::string(typeid(ex).name()) == typeid(LLEventPump::DupPumpName).name()) - { - threw = true; - } - else - { - // We don't even recognize this exception. Let it propagate - // out to TUT to fail the test. - throw; - } - } - catch (...) - { - std::cerr << "Utterly failed to catch expected exception!" << std::endl; - // This case is full of fail. We HAVE to address it. - throw; - } - ensure("second LLSDMessage should throw", threw); + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::DupPumpName) + ensure("second LLSDMessage should throw", ! threw.empty()); } template<> template<> diff --git a/indra/test/catch_and_store_what_in.h b/indra/test/catch_and_store_what_in.h new file mode 100644 index 0000000000..59f8cc0085 --- /dev/null +++ b/indra/test/catch_and_store_what_in.h @@ -0,0 +1,86 @@ +/** + * @file catch_and_store_what_in.h + * @author Nat Goodspeed + * @date 2012-02-15 + * @brief CATCH_AND_STORE_WHAT_IN() macro + * + * $LicenseInfo:firstyear=2012&license=viewerlgpl$ + * Copyright (c) 2012, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_CATCH_AND_STORE_WHAT_IN_H) +#define LL_CATCH_AND_STORE_WHAT_IN_H + +/** + * Idiom useful for test programs: catch an expected exception, store its + * what() string in a specified std::string variable. From there the caller + * can do things like: + * @code + * ensure("expected exception not thrown", ! string.empty()); + * @endcode + * or + * @code + * ensure_contains("exception doesn't mention blah", string, "blah"); + * @endcode + * etc. + * + * The trouble is that when linking to a dynamic libllcommon.so on Linux, we + * generally fail to catch the specific exception. Oddly, we can catch it as + * std::runtime_error and validate its typeid().name(), so we do -- but that's + * a lot of boilerplate per test. Encapsulate with this macro. Usage: + * + * @code + * std::string threw; + * try + * { + * some_call_that_should_throw_Foo(); + * } + * CATCH_AND_STORE_WHAT_IN(threw, Foo) + * ensure("some_call_that_should_throw_Foo() didn't throw", ! threw.empty()); + * @endcode + */ +#define CATCH_AND_STORE_WHAT_IN(THREW, EXCEPTION) \ +catch (const EXCEPTION& ex) \ +{ \ + (THREW) = ex.what(); \ +} \ +CATCH_MISSED_LINUX_EXCEPTION(THREW, EXCEPTION) + +#ifndef LL_LINUX +#define CATCH_MISSED_LINUX_EXCEPTION(THREW, EXCEPTION) \ + /* only needed on Linux */ +#else // LL_LINUX + +#define CATCH_MISSED_LINUX_EXCEPTION(THREW, EXCEPTION) \ +catch (const std::runtime_error& ex) \ +{ \ + /* This clause is needed on Linux, on the viewer side, because */ \ + /* the exception isn't caught by catch (const EXCEPTION&). */ \ + /* But if the expected exception was thrown, allow the test to */ \ + /* succeed anyway. Not sure how else to handle this odd case. */ \ + if (std::string(typeid(ex).name()) == typeid(EXCEPTION).name()) \ + { \ + /* std::cerr << "Caught " << typeid(ex).name() */ \ + /* << " with Linux workaround" << std::endl; */ \ + (THREW) = ex.what(); \ + /*std::cout << ex.what() << std::endl;*/ \ + } \ + else \ + { \ + /* We don't even recognize this exception. Let it propagate */ \ + /* out to TUT to fail the test. */ \ + throw; \ + } \ +} \ +catch (...) \ +{ \ + std::cerr << "Failed to catch expected exception " \ + << #EXCEPTION << "!" << std::endl; \ + /* This indicates a problem in the test that should be addressed. */ \ + throw; \ +} + +#endif // LL_LINUX + +#endif /* ! defined(LL_CATCH_AND_STORE_WHAT_IN_H) */ diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp index 4699bb1827..ca4c74099f 100644 --- a/indra/test/llevents_tut.cpp +++ b/indra/test/llevents_tut.cpp @@ -49,46 +49,12 @@ #include // other Linden headers #include "lltut.h" +#include "catch_and_store_what_in.h" #include "stringize.h" #include "tests/listener.h" using boost::assign::list_of; -#ifdef LL_LINUX -#define CATCH_MISSED_LINUX_EXCEPTION(exception, threw) \ -catch (const std::runtime_error& ex) \ -{ \ - /* This clause is needed on Linux, on the viewer side, because the */ \ - /* exception isn't caught by the clause above. Warn the user... */ \ - std::cerr << "Failed to catch " << typeid(ex).name() << std::endl; \ - /* But if the expected exception was thrown, allow the test to */ \ - /* succeed anyway. Not sure how else to handle this odd case. */ \ - /* This approach is also used in llsdmessage_test.cpp. */ \ - if (std::string(typeid(ex).name()) == typeid(exception).name()) \ - { \ - threw = ex.what(); \ - /*std::cout << ex.what() << std::endl;*/ \ - } \ - else \ - { \ - /* We don't even recognize this exception. Let it propagate */ \ - /* out to TUT to fail the test. */ \ - throw; \ - } \ -} \ -catch (...) \ -{ \ - std::cerr << "Utterly failed to catch expected exception " << #exception << "!" << \ - std::endl; \ - /* This indicates a problem in the test that should be addressed. */ \ - throw; \ -} - -#else // LL_LINUX -#define CATCH_MISSED_LINUX_EXCEPTION(exception, threw) \ - /* Not needed on other platforms */ -#endif // LL_LINUX - template T make(const T& value) { @@ -178,11 +144,7 @@ void events_object::test<1>() per_frame.listen(listener0.getName(), // note bug, dup name boost::bind(&Listener::call, boost::ref(listener1), _1)); } - catch (const LLEventPump::DupListenerName& e) - { - threw = e.what(); - } - CATCH_MISSED_LINUX_EXCEPTION(LLEventPump::DupListenerName, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::DupListenerName) ensure_equals(threw, std::string("DupListenerName: " "Attempt to register duplicate listener name '") + @@ -388,12 +350,7 @@ void events_object::test<7>() // after "Mary" and "checked" -- whoops! make(list_of("Mary")("checked"))); } - catch (const LLEventPump::Cycle& e) - { - threw = e.what(); - // std::cout << "Caught: " << e.what() << '\n'; - } - CATCH_MISSED_LINUX_EXCEPTION(LLEventPump::Cycle, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::Cycle) // Obviously the specific wording of the exception text can // change; go ahead and change the test to match. // Establish that it contains: @@ -426,12 +383,7 @@ void events_object::test<7>() make(list_of("shoelaces")), make(list_of("yellow"))); } - catch (const LLEventPump::OrderChange& e) - { - threw = e.what(); - // std::cout << "Caught: " << e.what() << '\n'; - } - CATCH_MISSED_LINUX_EXCEPTION(LLEventPump::OrderChange, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::OrderChange) // Same remarks about the specific wording of the exception. Just // ensure that it contains enough information to clarify the // problem and what must be done to resolve it. @@ -459,12 +411,7 @@ void events_object::test<8>() // then another with a duplicate name. LLEventStream bob2("bob"); } - catch (const LLEventPump::DupPumpName& e) - { - threw = e.what(); - // std::cout << "Caught: " << e.what() << '\n'; - } - CATCH_MISSED_LINUX_EXCEPTION(LLEventPump::DupPumpName, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::DupPumpName) ensure("Caught DupPumpName", !threw.empty()); } // delete first 'bob' LLEventStream bob("bob"); // should work, previous one unregistered @@ -505,11 +452,7 @@ void events_object::test<9>() LLListenerOrPumpName empty; empty(17); } - catch (const LLListenerOrPumpName::Empty& e) - { - threw = e.what(); - } - CATCH_MISSED_LINUX_EXCEPTION(LLListenerOrPumpName::Empty, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLListenerOrPumpName::Empty) ensure("threw Empty", !threw.empty()); } -- cgit v1.3 From 4ccae31320a7bc321a4d1df105cdf3b70b60d503 Mon Sep 17 00:00:00 2001 From: callum Date: Mon, 23 Apr 2012 11:28:52 -0700 Subject: SH-3115 FIX Crash on exit, curl handles not empty Reviewed by Ted. --- indra/llmessage/llcurl.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'indra/llmessage') diff --git a/indra/llmessage/llcurl.cpp b/indra/llmessage/llcurl.cpp index 3bcaffc275..4d92cfd32f 100644 --- a/indra/llmessage/llcurl.cpp +++ b/indra/llmessage/llcurl.cpp @@ -1506,7 +1506,8 @@ void LLCurl::cleanupClass() delete sHandleMutexp ; sHandleMutexp = NULL ; - llassert(Easy::sActiveHandles.empty()); + // removed as per https://jira.secondlife.com/browse/SH-3115 + //llassert(Easy::sActiveHandles.empty()); } //static -- cgit v1.3