From 66981fab0b3c8dcc3a031d50710a2b24ec6b0603 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 10 May 2018 21:46:07 -0400 Subject: SL-793: Use Boost.Fiber instead of the "dcoroutine" library. Longtime fans will remember that the "dcoroutine" library is a Google Summer of Code project by Giovanni P. Deretta. He originally called it "Boost.Coroutine," and we originally added it to our 3p-boost autobuild package as such. But when the official Boost.Coroutine library came along (with a very different API), and we still needed the API of the GSoC project, we renamed the unofficial one "dcoroutine" to allow coexistence. The "dcoroutine" library had an internal low-level API more or less analogous to Boost.Context. We later introduced an implementation of that internal API based on Boost.Context, a step towards eliminating the GSoC code in favor of official, supported Boost code. However, recent versions of Boost.Context no longer support the API on which we built the shim for "dcoroutine." We started down the path of reimplementing that shim using the current Boost.Context API -- then realized that it's time to bite the bullet and replace the "dcoroutine" API with the Boost.Fiber API, which we've been itching to do for literally years now. Naturally, most of the heavy lifting is in llcoros.{h,cpp} and lleventcoro.{h,cpp} -- which is good: the LLCoros layer abstracts away most of the differences between "dcoroutine" and Boost.Fiber. The one feature Boost.Fiber does not provide is the ability to forcibly terminate some other fiber. Accordingly, disable LLCoros::kill() and LLCoprocedureManager::shutdown(). The only known shutdown() call was in LLCoprocedurePool's destructor. We also took the opportunity to remove postAndSuspend2() and its associated machinery: FutureListener2, LLErrorEvent, errorException(), errorLog(), LLCoroEventPumps. All that dual-LLEventPump stuff was introduced at a time when the Responder pattern was king, and we assumed we'd want to listen on one LLEventPump with the success handler and on another with the error handler. We have never actually used that in practice. Remove associated tests, of course. There is one other semantic difference that necessitates patching a number of tests: with "dcoroutine," fulfilling a future IMMEDIATELY resumes the waiting coroutine. With Boost.Fiber, fulfilling a future merely marks the fiber as ready to resume next time the scheduler gets around to it. To observe the test side effects, we've inserted a number of llcoro::suspend() calls -- also in the main loop. For a long time we retained a single unit test exercising the raw "dcoroutine" API. Remove that. Eliminate llcoro_get_id.{h,cpp}, which provided llcoro::get_id(), which was a hack to emulate fiber-local variables. Since Boost.Fiber has an actual API for that, remove the hack. In fact, use (new alias) LLCoros::local_ptr for LLSingleton's dependency tracking in place of llcoro::get_id(). In CMake land, replace BOOST_COROUTINE_LIBRARY with BOOST_FIBER_LIBRARY. We don't actually use the Boost.Coroutine for anything (though there exist plausible use cases). --- indra/test/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/test') diff --git a/indra/test/CMakeLists.txt b/indra/test/CMakeLists.txt index 8344cead57..4187076030 100644 --- a/indra/test/CMakeLists.txt +++ b/indra/test/CMakeLists.txt @@ -98,7 +98,7 @@ target_link_libraries(lltest ${WINDOWS_LIBRARIES} ${BOOST_PROGRAM_OPTIONS_LIBRARY} ${BOOST_REGEX_LIBRARY} - ${BOOST_COROUTINE_LIBRARY} + ${BOOST_FIBER_LIBRARY} ${BOOST_CONTEXT_LIBRARY} ${BOOST_SYSTEM_LIBRARY} ${DL_LIBRARY} -- cgit v1.3 From 939d35054881d150fe5d191d6965f6a09c5d0223 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 29 Dec 2018 10:19:01 -0500 Subject: SL-793: Add LL_PRETTY_FUNCTION macro wrapping __PRETTY_FUNCTION__ which is, of course, different in Visual Studio (__FUNCSIG__). Use LL_PRETTY_FUNCTION in DEBUG output instead of plain __FUNCTION__. --- indra/llcommon/llpreprocessor.h | 7 +++++++ indra/test/debug.h | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) (limited to 'indra/test') diff --git a/indra/llcommon/llpreprocessor.h b/indra/llcommon/llpreprocessor.h index e8f9981437..bae402110a 100644 --- a/indra/llcommon/llpreprocessor.h +++ b/indra/llcommon/llpreprocessor.h @@ -232,4 +232,11 @@ #define LL_COMPILE_TIME_MESSAGE(msg) #endif +// __FUNCTION__ works on all the platforms we care about, but... +#if LL_WINDOWS +#define LL_PRETTY_FUNCTION __FUNCSIG__ +#else +#define LL_PRETTY_FUNCTION __PRETTY_FUNCTION__ +#endif + #endif // not LL_LINDEN_PREPROCESSOR_H diff --git a/indra/test/debug.h b/indra/test/debug.h index d61eba651b..33c3ea2d27 100644 --- a/indra/test/debug.h +++ b/indra/test/debug.h @@ -64,7 +64,7 @@ private: // It's often convenient to use the name of the enclosing function as the name // of the Debug block. -#define DEBUG Debug debug(__FUNCTION__) +#define DEBUG Debug debug(LL_PRETTY_FUNCTION) // These BEGIN/END macros are specifically for debugging output -- please // don't assume you must use such for coroutines in general! They only help to -- cgit v1.3 From b5bb0794f0022517c7aeff9a7775864a56488da6 Mon Sep 17 00:00:00 2001 From: Anchor Date: Wed, 8 May 2019 18:57:33 -0600 Subject: [DRTVWR-476] - fix linking --- indra/CMakeLists.txt | 5 +++++ indra/cmake/LLAddBuildTest.cmake | 5 +---- indra/integration_tests/llimage_libtest/CMakeLists.txt | 1 + indra/integration_tests/llui_libtest/CMakeLists.txt | 1 + indra/llcorehttp/CMakeLists.txt | 1 + indra/llplugin/slplugin/CMakeLists.txt | 1 + indra/newview/CMakeLists.txt | 5 +---- indra/test/CMakeLists.txt | 1 + indra/win_crash_logger/CMakeLists.txt | 2 ++ 9 files changed, 14 insertions(+), 8 deletions(-) (limited to 'indra/test') diff --git a/indra/CMakeLists.txt b/indra/CMakeLists.txt index 62a8f3f003..53e5d7b6a5 100644 --- a/indra/CMakeLists.txt +++ b/indra/CMakeLists.txt @@ -15,6 +15,11 @@ set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake") include(Variables) include(BuildVersion) +set(LEGACY_STDIO_LIBS) +if (WINDOWS) + set(LEGACY_STDIO_LIBS legacy_stdio_definitions) +endif (WINDOWS) + if (NOT CMAKE_BUILD_TYPE) set(CMAKE_BUILD_TYPE RelWithDebInfo CACHE STRING "Build type. One of: Debug Release RelWithDebInfo" FORCE) diff --git a/indra/cmake/LLAddBuildTest.cmake b/indra/cmake/LLAddBuildTest.cmake index 0a20a2c068..4932e9044f 100644 --- a/indra/cmake/LLAddBuildTest.cmake +++ b/indra/cmake/LLAddBuildTest.cmake @@ -200,6 +200,7 @@ FUNCTION(LL_ADD_INTEGRATION_TEST ) SET(libraries + ${LEGACY_STDIO_LIBS} ${library_dependencies} ${BOOST_FIBER_LIBRARY} ${BOOST_CONTEXT_LIBRARY} @@ -208,10 +209,6 @@ FUNCTION(LL_ADD_INTEGRATION_TEST ${PTHREAD_LIBRARY} ) - if (WINDOWS) - list(INSERT libraries 0 legacy_stdio_definitions) - endif (WINDOWS) - # Add test executable build target if(TEST_DEBUG) message(STATUS "ADD_EXECUTABLE(INTEGRATION_TEST_${testname} ${source_files})") diff --git a/indra/integration_tests/llimage_libtest/CMakeLists.txt b/indra/integration_tests/llimage_libtest/CMakeLists.txt index d9353f904c..5787d4d600 100644 --- a/indra/integration_tests/llimage_libtest/CMakeLists.txt +++ b/indra/integration_tests/llimage_libtest/CMakeLists.txt @@ -64,6 +64,7 @@ endif (DARWIN) # Libraries on which this application depends on # Sort by high-level to low-level target_link_libraries(llimage_libtest + ${LEGACY_STDIO_LIBS} ${LLCOMMON_LIBRARIES} ${LLVFS_LIBRARIES} ${LLMATH_LIBRARIES} diff --git a/indra/integration_tests/llui_libtest/CMakeLists.txt b/indra/integration_tests/llui_libtest/CMakeLists.txt index 34e34c7e47..1cec660eb0 100644 --- a/indra/integration_tests/llui_libtest/CMakeLists.txt +++ b/indra/integration_tests/llui_libtest/CMakeLists.txt @@ -75,6 +75,7 @@ endif (DARWIN) # Libraries on which this library depends, needed for Linux builds # Sort by high-level to low-level target_link_libraries(llui_libtest + ${LEGACY_STDIO_LIBS} llui llinventory llmessage diff --git a/indra/llcorehttp/CMakeLists.txt b/indra/llcorehttp/CMakeLists.txt index 9dbc6f447e..9e9685f1cb 100644 --- a/indra/llcorehttp/CMakeLists.txt +++ b/indra/llcorehttp/CMakeLists.txt @@ -198,6 +198,7 @@ endif (DARWIN) ) set(example_libs + ${LEGACY_STDIO_LIBS} ${LLCOREHTTP_LIBRARIES} ${WINDOWS_LIBRARIES} ${LLMESSAGE_LIBRARIES} diff --git a/indra/llplugin/slplugin/CMakeLists.txt b/indra/llplugin/slplugin/CMakeLists.txt index 33520ad64c..e4f64448c5 100644 --- a/indra/llplugin/slplugin/CMakeLists.txt +++ b/indra/llplugin/slplugin/CMakeLists.txt @@ -63,6 +63,7 @@ set_target_properties(SLPlugin endif () target_link_libraries(SLPlugin + ${LEGACY_STDIO_LIBS} ${LLPLUGIN_LIBRARIES} ${LLMESSAGE_LIBRARIES} ${LLCOMMON_LIBRARIES} diff --git a/indra/newview/CMakeLists.txt b/indra/newview/CMakeLists.txt index 2ad969a705..0ba3b07566 100644 --- a/indra/newview/CMakeLists.txt +++ b/indra/newview/CMakeLists.txt @@ -1977,11 +1977,8 @@ endif (WINDOWS) # dumped into the target binary and runtime lookup will find the most # modern version. -if (WINDOWS) - list(INSERT PNG_PRELOAD_ARCHIVES 0 legacy_stdio_definitions) -endif (WINDOWS) - target_link_libraries(${VIEWER_BINARY_NAME} + ${LEGACY_STDIO_LIBS} ${PNG_PRELOAD_ARCHIVES} ${ZLIB_PRELOAD_ARCHIVES} ${URIPARSER_PRELOAD_ARCHIVES} diff --git a/indra/test/CMakeLists.txt b/indra/test/CMakeLists.txt index 4187076030..0f14862cba 100644 --- a/indra/test/CMakeLists.txt +++ b/indra/test/CMakeLists.txt @@ -83,6 +83,7 @@ list(APPEND test_SOURCE_FILES ${test_HEADER_FILES}) add_executable(lltest ${test_SOURCE_FILES}) target_link_libraries(lltest + ${LEGACY_STDIO_LIBS} ${LLDATABASE_LIBRARIES} ${LLINVENTORY_LIBRARIES} ${LLMESSAGE_LIBRARIES} diff --git a/indra/win_crash_logger/CMakeLists.txt b/indra/win_crash_logger/CMakeLists.txt index 1c3479bf69..1b038c26b5 100644 --- a/indra/win_crash_logger/CMakeLists.txt +++ b/indra/win_crash_logger/CMakeLists.txt @@ -72,7 +72,9 @@ find_library(DXGUID_LIBRARY dxguid ${DIRECTX_LIBRARY_DIR}) add_executable(windows-crash-logger WIN32 ${win_crash_logger_SOURCE_FILES}) + target_link_libraries(windows-crash-logger + ${LEGACY_STDIO_LIBS} ${BREAKPAD_EXCEPTION_HANDLER_LIBRARIES} ${LLCRASHLOGGER_LIBRARIES} ${LLWINDOW_LIBRARIES} -- cgit v1.3 From d7c2e4a77bed665d7ab626d9955c35db8c318e95 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 11 Sep 2019 09:33:07 -0400 Subject: DRTVWR-476: Add Sync class to help with stepwise coroutine tests. Sync is specifically intended for test programs. It is based on an LLScalarCond. The idea is that each of two coroutines can watch for the other to get a chance to run, indicated by incrementing the wrapped int and notifying the wrapped condition_variable. This is less hand-wavy than calling llcoro::suspend() and hoping that the other routine will have had a chance to run. Use Sync in lleventcoro_test.cpp. Also refactor lleventcoro_test.cpp so that instead of a collection of static data requiring a clear() call at start of each individual test function, the relevant data is all part of the test_data struct common to all test functions. Make the helper coroutine functions members of test_data too. Introduce llcoro::logname(), a convenience function to log the name of the currently executing coroutine or "main" if in the thread's main coroutine. --- indra/llcommon/llcoros.h | 13 ++++ indra/llcommon/tests/lleventcoro_test.cpp | 102 ++++++++++++++---------------- indra/test/CMakeLists.txt | 1 + indra/test/sync.h | 85 +++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 53 deletions(-) create mode 100644 indra/test/sync.h (limited to 'indra/test') diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h index 678633497d..dedb6c8eca 100644 --- a/indra/llcommon/llcoros.h +++ b/indra/llcommon/llcoros.h @@ -239,4 +239,17 @@ private: static void delete_CoroData(CoroData* cdptr); }; +namespace llcoro +{ + +inline +std::string logname() +{ + static std::string main("main"); + std::string name(LLCoros::instance().getName()); + return name.empty()? main : name; +} + +} // llcoro + #endif /* ! defined(LL_LLCOROS_H) */ diff --git a/indra/llcommon/tests/lleventcoro_test.cpp b/indra/llcommon/tests/lleventcoro_test.cpp index 2e4b6ba823..4e774b27d9 100644 --- a/indra/llcommon/tests/lleventcoro_test.cpp +++ b/indra/llcommon/tests/lleventcoro_test.cpp @@ -45,6 +45,7 @@ #include "llcoros.h" #include "lleventcoro.h" #include "../test/debug.h" +#include "../test/sync.h" using namespace llcoro; @@ -58,8 +59,9 @@ using namespace llcoro; class ImmediateAPI { public: - ImmediateAPI(): - mPump("immediate", true) + ImmediateAPI(Sync& sync): + mPump("immediate", true), + mSync(sync) { mPump.listen("API", boost::bind(&ImmediateAPI::operator(), this, _1)); } @@ -68,22 +70,18 @@ public: // Invoke this with an LLSD map containing: // ["value"]: Integer value. We will reply with ["value"] + 1. - // ["reply"]: Name of LLEventPump on which to send success response. - // ["error"]: Name of LLEventPump on which to send error response. - // ["fail"]: Presence of this key selects ["error"], else ["success"] as - // the name of the pump on which to send the response. + // ["reply"]: Name of LLEventPump on which to send response. bool operator()(const LLSD& event) const { + mSync.bump(); LLSD::Integer value(event["value"]); - LLSD::String replyPumpName(event.has("fail")? "error" : "reply"); - LLEventPumps::instance().obtain(event[replyPumpName]).post(value + 1); - // give listener a chance to process - llcoro::suspend(); + LLEventPumps::instance().obtain(event["reply"]).post(value + 1); return false; } private: LLEventStream mPump; + Sync& mSync; }; /***************************************************************************** @@ -91,34 +89,29 @@ private: *****************************************************************************/ namespace tut { - struct coroutine_data {}; - typedef test_group coroutine_group; + struct test_data + { + Sync mSync; + ImmediateAPI immediateAPI{mSync}; + std::string replyName, errorName, threw, stringdata; + LLSD result, errordata; + int which; + + void explicit_wait(boost::shared_ptr>& cbp); + void waitForEventOn1(); + void coroPump(); + void postAndWait1(); + void coroPumpPost(); + }; + typedef test_group coroutine_group; typedef coroutine_group::object object; coroutine_group coroutinegrp("coroutine"); - // use static data so we can intersperse coroutine functions with the - // tests that engage them - ImmediateAPI immediateAPI; - std::string replyName, errorName, threw, stringdata; - LLSD result, errordata; - int which; - - // reinit vars at the start of each test - void clear() - { - replyName.clear(); - errorName.clear(); - threw.clear(); - stringdata.clear(); - result = LLSD(); - errordata = LLSD(); - which = 0; - } - - void explicit_wait(boost::shared_ptr>& cbp) + void test_data::explicit_wait(boost::shared_ptr>& cbp) { BEGIN { + mSync.bump(); // The point of this test is to verify / illustrate suspending a // coroutine for something other than an LLEventPump. In other // words, this shows how to adapt to any async operation that @@ -136,6 +129,7 @@ namespace tut // calling get() on the future causes us to suspend debug("about to suspend"); stringdata = future.get(); + mSync.bump(); ensure_equals("Got it", stringdata, "received"); } END @@ -144,30 +138,32 @@ namespace tut template<> template<> void object::test<1>() { - clear(); set_test_name("explicit_wait"); DEBUG; // Construct the coroutine instance that will run explicit_wait. boost::shared_ptr> respond; LLCoros::instance().launch("test<1>", - boost::bind(explicit_wait, boost::ref(respond))); + [this, &respond](){ explicit_wait(respond); }); + mSync.bump(); // When the coroutine waits for the future, it returns here. debug("about to respond"); // Now we're the I/O subsystem delivering a result. This should make // the coroutine ready. respond->set_value("received"); // but give it a chance to wake up - llcoro::suspend(); + mSync.yield(); // ensure the coroutine ran and woke up again with the intended result ensure_equals(stringdata, "received"); } - void waitForEventOn1() + void test_data::waitForEventOn1() { BEGIN { + mSync.bump(); result = suspendUntilEventOn("source"); + mSync.bump(); } END } @@ -175,25 +171,27 @@ namespace tut template<> template<> void object::test<2>() { - clear(); set_test_name("waitForEventOn1"); DEBUG; - LLCoros::instance().launch("test<2>", waitForEventOn1); + LLCoros::instance().launch("test<2>", [this](){ waitForEventOn1(); }); + mSync.bump(); debug("about to send"); LLEventPumps::instance().obtain("source").post("received"); // give waitForEventOn1() a chance to run - llcoro::suspend(); + mSync.yield(); debug("back from send"); ensure_equals(result.asString(), "received"); } - void coroPump() + void test_data::coroPump() { BEGIN { + mSync.bump(); LLCoroEventPump waiter; replyName = waiter.getName(); result = waiter.suspend(); + mSync.bump(); } END } @@ -201,26 +199,28 @@ namespace tut template<> template<> void object::test<3>() { - clear(); set_test_name("coroPump"); DEBUG; - LLCoros::instance().launch("test<3>", coroPump); + LLCoros::instance().launch("test<3>", [this](){ coroPump(); }); + mSync.bump(); debug("about to send"); LLEventPumps::instance().obtain(replyName).post("received"); // give coroPump() a chance to run - llcoro::suspend(); + mSync.yield(); debug("back from send"); ensure_equals(result.asString(), "received"); } - void postAndWait1() + void test_data::postAndWait1() { BEGIN { + mSync.bump(); result = postAndSuspend(LLSDMap("value", 17), // request event immediateAPI.getPump(), // requestPump "reply1", // replyPump "reply"); // request["reply"] = name + mSync.bump(); } END } @@ -228,22 +228,21 @@ namespace tut template<> template<> void object::test<4>() { - clear(); set_test_name("postAndWait1"); DEBUG; - LLCoros::instance().launch("test<4>", postAndWait1); - // give postAndWait1() a chance to run - llcoro::suspend(); + LLCoros::instance().launch("test<4>", [this](){ postAndWait1(); }); ensure_equals(result.asInteger(), 18); } - void coroPumpPost() + void test_data::coroPumpPost() { BEGIN { + mSync.bump(); LLCoroEventPump waiter; result = waiter.postAndSuspend(LLSDMap("value", 17), immediateAPI.getPump(), "reply"); + mSync.bump(); } END } @@ -251,12 +250,9 @@ namespace tut template<> template<> void object::test<5>() { - clear(); set_test_name("coroPumpPost"); DEBUG; - LLCoros::instance().launch("test<5>", coroPumpPost); - // give coroPumpPost() a chance to run - llcoro::suspend(); + LLCoros::instance().launch("test<5>", [this](){ coroPumpPost(); }); ensure_equals(result.asInteger(), 18); } } diff --git a/indra/test/CMakeLists.txt b/indra/test/CMakeLists.txt index 0f14862cba..87536e146b 100644 --- a/indra/test/CMakeLists.txt +++ b/indra/test/CMakeLists.txt @@ -67,6 +67,7 @@ set(test_HEADER_FILES llpipeutil.h llsdtraits.h lltut.h + sync.h ) if (NOT WINDOWS) diff --git a/indra/test/sync.h b/indra/test/sync.h new file mode 100644 index 0000000000..cafbc034b4 --- /dev/null +++ b/indra/test/sync.h @@ -0,0 +1,85 @@ +/** + * @file sync.h + * @author Nat Goodspeed + * @date 2019-03-13 + * @brief Synchronize coroutines within a test program so we can observe side + * effects. Certain test programs test coroutine synchronization + * mechanisms. Such tests usually want to interleave coroutine + * executions in strictly stepwise fashion. This class supports that + * paradigm. + * + * $LicenseInfo:firstyear=2019&license=viewerlgpl$ + * Copyright (c) 2019, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_SYNC_H) +#define LL_SYNC_H + +#include "llcond.h" +#include "lltut.h" +#include "stringize.h" +#include "llerror.h" +#include "llcoros.h" + +/** + * Instantiate Sync in any test in which we need to suspend one coroutine + * until we're sure that another has had a chance to run. Simply calling + * llcoro::suspend() isn't necessarily enough; that provides a chance for the + * other to run, but doesn't guarantee that it has. If each coroutine is + * consistent about calling Sync::bump() every time it wakes from any + * suspension, Sync::yield() and yield_until() should at least ensure that + * somebody else has had a chance to run. + */ +class Sync +{ + LLScalarCond mCond{0}; + F32Milliseconds mTimeout; + +public: + Sync(F32Milliseconds timeout=F32Milliseconds(10.0f)): + mTimeout(timeout) + {} + + /// Bump mCond by n steps -- ideally, do this every time a participating + /// coroutine wakes up from any suspension. The choice to bump() after + /// resumption rather than just before suspending is worth calling out: + /// this practice relies on the fact that condition_variable::notify_all() + /// merely marks a suspended coroutine ready to run, rather than + /// immediately resuming it. This way, though, even if a coroutine exits + /// before reaching its next suspend point, the other coroutine isn't + /// left waiting forever. + void bump(int n=1) + { + LL_DEBUGS() << llcoro::logname() << " bump(" << n << ") -> " << (mCond.get() + n) << LL_ENDL; + mCond.set_all(mCond.get() + n); + } + + /// suspend until "somebody else" has bumped mCond by n steps + void yield(int n=1) + { + return yield_until(STRINGIZE("Sync::yield_for(" << n << ") timed out after " + << int(mTimeout.value()) << "ms"), + mCond.get() + n); + } + + /// suspend until "somebody else" has bumped mCond to a specific value + void yield_until(int until) + { + return yield_until(STRINGIZE("Sync::yield_until(" << until << ") timed out after " + << int(mTimeout.value()) << "ms"), + until); + } + +private: + void yield_until(const std::string& desc, int until) + { + std::string name(llcoro::logname()); + LL_DEBUGS() << name << " yield_until(" << until << ") suspending" << LL_ENDL; + tut::ensure(name + ' ' + desc, mCond.wait_for_equal(mTimeout, until)); + // each time we wake up, bump mCond + bump(); + } +}; + +#endif /* ! defined(LL_SYNC_H) */ -- cgit v1.3 From 6b70493ddb1b95a2d3527e2189f5b94f5a2b606f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 14 Oct 2019 15:41:09 -0400 Subject: DRTVWR-476: Make test program --debug switch work like LOGTEST=DEBUG. The comments within indra/test/test.cpp promise that --debug is, in fact, like LOGTEST=DEBUG. Until now, that was a lie. LOGTEST=level displayed log output on stderr as well as in testprogram.log, while --debug did not. Add LLError::logToStderr() function, and make initForApplication() (i.e. commonInit()) call that instead of instantiating RecordToStderr inline. Also call it when test.cpp recognizes --debug switch. Remove the mFileRecorder, mFixedBufferRecorder and mFileRecorderFileName members from SettingsConfig. That tactic doesn't scale. Instead, add findRecorder() and removeRecorder() template functions to locate (or remove) a RecorderPtr to an object of the specified subclass. Both are based on an underlying findRecorderPos() template function. Since we never expect to manage more than a handful of RecorderPtrs, and since access to the deleted members is very much application setup rather than any kind of ongoing access, a search loop suffices. logToFile() uses removeRecorder() rather than removing mFileRecorder (the only use of mFileRecorder). logToFixedBuffer() uses removeRecorder() rather than removing mFixedBufferRecorder (the only use of mFixedBufferRecorder). Make RecordToFile store the filename with which it was instantiated. Add a getFilename() method to retrieve it. logFileName() is now based on findRecorder() instead of mFileRecorderFileName (the only use of mFileRecorderFileName). Make RecordToStderr::mUseANSI a simple bool rather than a three-state enum, and set it immediately on construction. Apparently the reason it was set lazily was because it consults its own checkANSI() method, and of course 'this' doesn't acquire the leaf class type until the constructor has completed successfully. But since nothing in checkANSI() depends on anything else in RecordToStderr, making it static solves that problem. --- indra/llcommon/llerror.cpp | 196 +++++++++++++++++++++++++--------------- indra/llcommon/llerrorcontrol.h | 1 + indra/test/test.cpp | 3 + 3 files changed, 127 insertions(+), 73 deletions(-) (limited to 'indra/test') diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp index 2d4898f7be..acd863a316 100644 --- a/indra/llcommon/llerror.cpp +++ b/indra/llcommon/llerror.cpp @@ -118,27 +118,28 @@ namespace { class RecordToFile : public LLError::Recorder { public: - RecordToFile(const std::string& filename) + RecordToFile(const std::string& filename): + mName(filename) { mFile.open(filename.c_str(), std::ios_base::out | std::ios_base::app); if (!mFile) { LL_INFOS() << "Error setting log file to " << filename << LL_ENDL; } - else - { - if (!LLError::getAlwaysFlush()) - { - mFile.sync_with_stdio(false); - } - } + else + { + if (!LLError::getAlwaysFlush()) + { + mFile.sync_with_stdio(false); + } + } } - + ~RecordToFile() { mFile.close(); } - + virtual bool enabled() override { #ifdef LL_RELEASE_FOR_DOWNLOAD @@ -148,11 +149,13 @@ namespace { #endif } - bool okay() { return mFile.good(); } - - virtual void recordMessage(LLError::ELevel level, - const std::string& message) override - { + bool okay() const { return mFile.good(); } + + std::string getFilename() const { return mName; } + + virtual void recordMessage(LLError::ELevel level, + const std::string& message) override + { if (LLError::getAlwaysFlush()) { mFile << message << std::endl; @@ -161,9 +164,10 @@ namespace { { mFile << message << "\n"; } - } - + } + private: + const std::string mName; llofstream mFile; }; @@ -171,7 +175,7 @@ namespace { class RecordToStderr : public LLError::Recorder { public: - RecordToStderr(bool timestamp) : mUseANSI(ANSI_PROBE) + RecordToStderr(bool timestamp) : mUseANSI(checkANSI()) { this->showMultiline(true); } @@ -184,10 +188,7 @@ namespace { virtual void recordMessage(LLError::ELevel level, const std::string& message) override { - if (ANSI_PROBE == mUseANSI) - mUseANSI = (checkANSI() ? ANSI_YES : ANSI_NO); - - if (ANSI_YES == mUseANSI) + if (mUseANSI) { // Default all message levels to bold so we can distinguish our own messages from those dumped by subprocesses and libraries. colorANSI("1"); // bold @@ -206,16 +207,11 @@ namespace { } } fprintf(stderr, "%s\n", message.c_str()); - if (ANSI_YES == mUseANSI) colorANSI("0"); // reset + if (mUseANSI) colorANSI("0"); // reset } private: - enum ANSIState - { - ANSI_PROBE, - ANSI_YES, - ANSI_NO - } mUseANSI; + bool mUseANSI; void colorANSI(const std::string color) { @@ -223,7 +219,7 @@ namespace { fprintf(stderr, "\033[%sm", color.c_str() ); }; - bool checkANSI(void) + static bool checkANSI(void) { #if LL_LINUX || LL_DARWIN // Check whether it's okay to use ANSI; if stderr is @@ -491,14 +487,11 @@ namespace LLError LLError::FatalFunction mCrashFunction; LLError::TimeFunction mTimeFunction; - + Recorders mRecorders; - RecorderPtr mFileRecorder; - RecorderPtr mFixedBufferRecorder; - std::string mFileRecorderFileName; - - int mShouldLogCallCounter; - + + int mShouldLogCallCounter; + private: SettingsConfig(); }; @@ -532,9 +525,6 @@ namespace LLError mCrashFunction(NULL), mTimeFunction(NULL), mRecorders(), - mFileRecorder(), - mFixedBufferRecorder(), - mFileRecorderFileName(), mShouldLogCallCounter(0) { } @@ -686,20 +676,19 @@ namespace void commonInit(const std::string& user_dir, const std::string& app_dir, bool log_to_stderr = true) { LLError::Settings::getInstance()->reset(); - + LLError::setDefaultLevel(LLError::LEVEL_INFO); - LLError::setAlwaysFlush(true); - LLError::setEnabledLogTypesMask(0xFFFFFFFF); + LLError::setAlwaysFlush(true); + LLError::setEnabledLogTypesMask(0xFFFFFFFF); LLError::setFatalFunction(LLError::crashAndLoop); LLError::setTimeFunction(LLError::utcTime); // log_to_stderr is only false in the unit and integration tests to keep builds quieter if (log_to_stderr && shouldLogToStderr()) { - LLError::RecorderPtr recordToStdErr(new RecordToStderr(stderrLogWantsTime())); - LLError::addRecorder(recordToStdErr); + LLError::logToStderr(); } - + #if LL_WINDOWS LLError::RecorderPtr recordToWinDebug(new RecordToWinDebug()); LLError::addRecorder(recordToWinDebug); @@ -997,49 +986,110 @@ namespace LLError s->mRecorders.erase(std::remove(s->mRecorders.begin(), s->mRecorders.end(), recorder), s->mRecorders.end()); } + + // Find an entry in SettingsConfig::mRecorders whose RecorderPtr points to + // a Recorder subclass of type RECORDER. Return, not a RecorderPtr (which + // points to the Recorder base class), but a shared_ptr which + // specifically points to the concrete RECORDER subclass instance, along + // with a Recorders::iterator indicating the position of that entry in + // mRecorders. The shared_ptr might be empty (operator!() returns true) if + // there was no such RECORDER subclass instance in mRecorders. + template + std::pair, Recorders::iterator> + findRecorderPos() + { + SettingsConfigPtr s = Settings::instance().getSettingsConfig(); + // Since we promise to return an iterator, use a classic iterator + // loop. + auto end{s->mRecorders.end()}; + for (Recorders::iterator it{s->mRecorders.begin()}; it != end; ++it) + { + // *it is a RecorderPtr, a shared_ptr. Use a + // dynamic_pointer_cast to try to downcast to test if it's also a + // shared_ptr. + auto ptr = boost::dynamic_pointer_cast(*it); + if (ptr) + { + // found the entry we want + return { ptr, it }; + } + } + // dropped out of the loop without finding any such entry -- instead + // of default-constructing Recorders::iterator (which might or might + // not be valid), return a value that is valid but not dereferenceable. + return { {}, end }; + } + + // Find an entry in SettingsConfig::mRecorders whose RecorderPtr points to + // a Recorder subclass of type RECORDER. Return, not a RecorderPtr (which + // points to the Recorder base class), but a shared_ptr which + // specifically points to the concrete RECORDER subclass instance. The + // shared_ptr might be empty (operator!() returns true) if there was no + // such RECORDER subclass instance in mRecorders. + template + boost::shared_ptr findRecorder() + { + return findRecorderPos().first; + } + + // Remove an entry from SettingsConfig::mRecorders whose RecorderPtr + // points to a Recorder subclass of type RECORDER. Return true if there + // was one and we removed it, false if there wasn't one to start with. + template + bool removeRecorder() + { + auto found = findRecorderPos(); + if (found.first) + { + SettingsConfigPtr s = Settings::instance().getSettingsConfig(); + s->mRecorders.erase(found.second); + } + return bool(found.first); + } } namespace LLError { void logToFile(const std::string& file_name) { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + // remove any previous Recorder filling this role + removeRecorder(); - removeRecorder(s->mFileRecorder); - s->mFileRecorder.reset(); - s->mFileRecorderFileName.clear(); - if (!file_name.empty()) { - RecorderPtr recordToFile(new RecordToFile(file_name)); - if (boost::dynamic_pointer_cast(recordToFile)->okay()) - { - s->mFileRecorderFileName = file_name; - s->mFileRecorder = recordToFile; - addRecorder(recordToFile); - } + boost::shared_ptr recordToFile(new RecordToFile(file_name)); + if (recordToFile->okay()) + { + addRecorder(recordToFile); + } } } - - void logToFixedBuffer(LLLineBuffer* fixedBuffer) + + std::string logFileName() { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); + auto found = findRecorder(); + return found? found->getFilename() : std::string(); + } - removeRecorder(s->mFixedBufferRecorder); - s->mFixedBufferRecorder.reset(); - - if (fixedBuffer) - { - RecorderPtr recordToFixedBuffer(new RecordToFixedBuffer(fixedBuffer)); - s->mFixedBufferRecorder = recordToFixedBuffer; - addRecorder(recordToFixedBuffer); + void logToStderr() + { + if (! findRecorder()) + { + RecorderPtr recordToStdErr(new RecordToStderr(stderrLogWantsTime())); + addRecorder(recordToStdErr); } - } + } - std::string logFileName() + void logToFixedBuffer(LLLineBuffer* fixedBuffer) { - SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig(); - return s->mFileRecorderFileName; + // remove any previous Recorder filling this role + removeRecorder(); + + if (fixedBuffer) + { + RecorderPtr recordToFixedBuffer(new RecordToFixedBuffer(fixedBuffer)); + addRecorder(recordToFixedBuffer); + } } } diff --git a/indra/llcommon/llerrorcontrol.h b/indra/llcommon/llerrorcontrol.h index 276d22fc36..bfa2269025 100644 --- a/indra/llcommon/llerrorcontrol.h +++ b/indra/llcommon/llerrorcontrol.h @@ -183,6 +183,7 @@ namespace LLError // each error message is passed to each recorder via recordMessage() LL_COMMON_API void logToFile(const std::string& filename); + LL_COMMON_API void logToStderr(); LL_COMMON_API void logToFixedBuffer(LLLineBuffer*); // Utilities to add recorders for logging to a file or a fixed buffer // A second call to the same function will remove the logger added diff --git a/indra/test/test.cpp b/indra/test/test.cpp index b14c2eb255..51f0e80043 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -611,6 +611,9 @@ int main(int argc, char **argv) wait_at_exit = true; break; case 'd': + // this is what LLError::initForApplication() does internally + // when you pass log_to_stderr=true + LLError::logToStderr(); LLError::setDefaultLevel(LLError::LEVEL_DEBUG); break; case 'x': -- cgit v1.3 From 18e2b9ca8b5d4be0f1b92b464421693678cca0a0 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 15 Oct 2019 16:16:55 -0400 Subject: DRTVWR-476: Remove llwrap(), LLListenerWrapper[Base] and support. The only usage of any of this was in test code. --- indra/llcommon/CMakeLists.txt | 2 - indra/llcommon/llevents.h | 70 +------------ indra/llcommon/lllistenerwrapper.h | 198 ------------------------------------- indra/test/llevents_tut.cpp | 30 +----- 4 files changed, 2 insertions(+), 298 deletions(-) delete mode 100644 indra/llcommon/lllistenerwrapper.h (limited to 'indra/test') diff --git a/indra/llcommon/CMakeLists.txt b/indra/llcommon/CMakeLists.txt index d237985b04..035b379246 100644 --- a/indra/llcommon/CMakeLists.txt +++ b/indra/llcommon/CMakeLists.txt @@ -1,4 +1,3 @@ - # -*- cmake -*- project(llcommon) @@ -185,7 +184,6 @@ set(llcommon_HEADER_FILES llkeythrottle.h llleap.h llleaplistener.h - lllistenerwrapper.h llliveappconfig.h lllivefile.h llmainthreadtask.h diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h index 18525a8fa5..253592256d 100644 --- a/indra/llcommon/llevents.h +++ b/indra/llcommon/llevents.h @@ -814,62 +814,6 @@ private: LL_COMMON_API bool sendReply(const LLSD& reply, const LLSD& request, const std::string& replyKey="reply"); -/** - * Base class for LLListenerWrapper. See visit_and_connect() and llwrap(). We - * provide virtual @c accept_xxx() methods, customization points allowing a - * subclass access to certain data visible at LLEventPump::listen() time. - * Example subclass usage: - * - * @code - * myEventPump.listen("somename", - * llwrap(boost::bind(&MyClass::method, instance, _1))); - * @endcode - * - * Because of the anticipated usage (note the anonymous temporary - * MyListenerWrapper instance in the example above), the @c accept_xxx() - * methods must be @c const. - */ -class LL_COMMON_API LLListenerWrapperBase -{ -public: - /// New instance. The accept_xxx() machinery makes it important to use - /// shared_ptrs for our data. Many copies of this object are made before - /// the instance that actually ends up in the signal, yet accept_xxx() - /// will later be called on the @em original instance. All copies of the - /// same original instance must share the same data. - LLListenerWrapperBase(): - mName(new std::string), - mConnection(new LLBoundListener) - { - } - - /// Copy constructor. Copy shared_ptrs to original instance data. - LLListenerWrapperBase(const LLListenerWrapperBase& that): - mName(that.mName), - mConnection(that.mConnection) - { - } - virtual ~LLListenerWrapperBase() {} - - /// Ask LLEventPump::listen() for the listener name - virtual void accept_name(const std::string& name) const - { - *mName = name; - } - - /// Ask LLEventPump::listen() for the new connection - virtual void accept_connection(const LLBoundListener& connection) const - { - *mConnection = connection; - } - -protected: - /// Listener name. - boost::shared_ptr mName; - /// Connection. - boost::shared_ptr mConnection; -}; - /***************************************************************************** * Underpinnings *****************************************************************************/ @@ -1121,19 +1065,7 @@ namespace LLEventDetail // Boost.Signals, in case we were passed a boost::ref(). visit_each(visitor, LLEventDetail::unwrap(raw_listener)); // Make the connection using passed function. - LLBoundListener connection(connect_func(listener)); - // If the LISTENER is an LLListenerWrapperBase subclass, pass it the - // desired information. It's important that we pass the raw_listener - // so the compiler can make decisions based on its original type. - const LLListenerWrapperBase* lwb = - ll_template_cast(&raw_listener); - if (lwb) - { - lwb->accept_name(name); - lwb->accept_connection(connection); - } - // In any case, show new connection to caller. - return connection; + return connect_func(listener); } } // namespace LLEventDetail diff --git a/indra/llcommon/lllistenerwrapper.h b/indra/llcommon/lllistenerwrapper.h deleted file mode 100644 index 09d074abca..0000000000 --- a/indra/llcommon/lllistenerwrapper.h +++ /dev/null @@ -1,198 +0,0 @@ -/** - * @file lllistenerwrapper.h - * @author Nat Goodspeed - * @date 2009-11-30 - * @brief Introduce LLListenerWrapper template - * - * $LicenseInfo:firstyear=2009&license=viewerlgpl$ - * Second Life Viewer Source Code - * Copyright (C) 2010, 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$ - */ - -#if ! defined(LL_LLLISTENERWRAPPER_H) -#define LL_LLLISTENERWRAPPER_H - -#include "llevents.h" // LLListenerWrapperBase -#include - -/** - * Template base class for coding wrappers for LLEventPump listeners. - * - * Derive your listener wrapper from LLListenerWrapper. You must use - * LLLISTENER_WRAPPER_SUBCLASS() so your subclass will play nicely with - * boost::visit_each (q.v.). That way boost::signals2 can still detect - * derivation from LLEventTrackable, and so forth. - */ -template -class LLListenerWrapper: public LLListenerWrapperBase -{ -public: - /// Wrap an arbitrary listener object - LLListenerWrapper(const LISTENER& listener): - mListener(listener) - {} - - /// call - virtual bool operator()(const LLSD& event) - { - return mListener(event); - } - - /// Allow boost::visit_each() to peek at our mListener. - template - void accept_visitor(V& visitor) const - { - using boost::visit_each; - visit_each(visitor, mListener, 0); - } - -private: - LISTENER mListener; -}; - -/** - * Specialize boost::visit_each() (leveraging ADL) to peek inside an - * LLListenerWrapper to traverse its LISTENER. We borrow the - * accept_visitor() pattern from boost::bind(), avoiding the need to make - * mListener public. - */ -template -void visit_each(V& visitor, const LLListenerWrapper& wrapper, int) -{ - wrapper.accept_visitor(visitor); -} - -/// use this (sigh!) for each subclass of LLListenerWrapper you write -#define LLLISTENER_WRAPPER_SUBCLASS(CLASS) \ -template \ -void visit_each(V& visitor, const CLASS& wrapper, int) \ -{ \ - visit_each(visitor, static_cast&>(wrapper), 0); \ -} \ - \ -/* Have to state this explicitly, rather than using LL_TEMPLATE_CONVERTIBLE, */ \ -/* because the source type is itself a template. */ \ -template \ -struct ll_template_cast_impl*> \ -{ \ - const LLListenerWrapperBase* operator()(const CLASS* wrapper) \ - { \ - return wrapper; \ - } \ -} - -/** - * Make an instance of a listener wrapper. Every wrapper class must be a - * template accepting a listener object of arbitrary type. In particular, the - * type of a boost::bind() expression is deliberately undocumented. So we - * can't just write Wrapper(boost::bind(...)). Instead we must - * write llwrap(boost::bind(...)). - */ -template class WRAPPER, typename T> -WRAPPER llwrap(const T& listener) -{ - return WRAPPER(listener); -} - -/** - * This LLListenerWrapper template subclass is used to report entry/exit to an - * event listener, by changing this: - * @code - * someEventPump.listen("MyClass", - * boost::bind(&MyClass::method, ptr, _1)); - * @endcode - * to this: - * @code - * someEventPump.listen("MyClass", - * llwrap( - * boost::bind(&MyClass::method, ptr, _1))); - * @endcode - */ -template -class LLCoutListener: public LLListenerWrapper -{ - typedef LLListenerWrapper super; - -public: - /// Wrap an arbitrary listener object - LLCoutListener(const LISTENER& listener): - super(listener) - {} - - /// call - virtual bool operator()(const LLSD& event) - { - std::cout << "Entering listener " << *super::mName << " with " << event << std::endl; - bool handled = super::operator()(event); - std::cout << "Leaving listener " << *super::mName; - if (handled) - { - std::cout << " (handled)"; - } - std::cout << std::endl; - return handled; - } -}; - -LLLISTENER_WRAPPER_SUBCLASS(LLCoutListener); - -/** - * This LLListenerWrapper template subclass is used to log entry/exit to an - * event listener, by changing this: - * @code - * someEventPump.listen("MyClass", - * boost::bind(&MyClass::method, ptr, _1)); - * @endcode - * to this: - * @code - * someEventPump.listen("MyClass", - * llwrap( - * boost::bind(&MyClass::method, ptr, _1))); - * @endcode - */ -template -class LLLogListener: public LLListenerWrapper -{ - typedef LLListenerWrapper super; - -public: - /// Wrap an arbitrary listener object - LLLogListener(const LISTENER& listener): - super(listener) - {} - - /// call - virtual bool operator()(const LLSD& event) - { - LL_DEBUGS("LLLogListener") << "Entering listener " << *super::mName << " with " << event << LL_ENDL; - bool handled = super::operator()(event); - LL_DEBUGS("LLLogListener") << "Leaving listener " << *super::mName; - if (handled) - { - LL_CONT << " (handled)"; - } - LL_CONT << LL_ENDL; - return handled; - } -}; - -LLLISTENER_WRAPPER_SUBCLASS(LLLogListener); - -#endif /* ! defined(LL_LLLISTENERWRAPPER_H) */ diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp index 3abae3e43e..9ff0f5086d 100644 --- a/indra/test/llevents_tut.cpp +++ b/indra/test/llevents_tut.cpp @@ -38,7 +38,6 @@ #define testable public #include "llevents.h" #undef testable -#include "lllistenerwrapper.h" // STL headers // std headers #include @@ -633,33 +632,6 @@ ensure("implicit disconnect", ! connection.connected()); heaptest.post(2); } -template<> template<> -void events_object::test<15>() -{ -// This test ensures that using an LLListenerWrapper subclass doesn't -// block Boost.Signals2 from recognizing a bound LLEventTrackable -// subclass. -set_test_name("listen(llwrap(boost::bind(...TempTrackableListener ref...)))"); -bool live = false; -LLEventPump& heaptest(pumps.obtain("heaptest")); -LLBoundListener connection; -{ - TempTrackableListener tempListener("temp", live); - ensure("TempTrackableListener constructed", live); - connection = heaptest.listen(tempListener.getName(), - llwrap( - boost::bind(&TempTrackableListener::call, - boost::ref(tempListener), _1))); - heaptest.post(1); - check_listener("received", tempListener, 1); -} // presumably this will make tempListener go away? -// verify that -ensure("TempTrackableListener destroyed", ! live); -ensure("implicit disconnect", ! connection.connected()); -// now just make sure we don't blow up trying to access a freed object! -heaptest.post(2); -} - class TempSharedListener: public TempListener, public boost::enable_shared_from_this { @@ -670,7 +642,7 @@ TempSharedListener(const std::string& name, bool& liveFlag): }; template<> template<> -void events_object::test<16>() +void events_object::test<15>() { set_test_name("listen(boost::bind(...TempSharedListener ref...))"); #if 0 -- cgit v1.3 From afaad3cef749e926cb63b6aed0c14f25d3495d55 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 15 Oct 2019 23:06:04 -0400 Subject: DRTVWR-476: Remove special case for listen(boost::bind(weak_ptr)). LLEventDetail::visit_and_connect() promised special treatment for the specific case when an LLEventPump::listen() listener was composed of (possibly nested) boost::bind() objects storing boost::weak_ptr values -- specifically boost::bind() rather than std::bind or lambdas, specifically boost::weak_ptr rather than std::weak_ptr. Outside of self-tests, it does not appear that anyone actually uses that support. There is good reason not to: it's a silent side effect of a complicated compile-time inspection that could be silently derailed by use of std::bind() or a lambda or a std::weak_ptr. Can you be sure you've engaged that promise? How? A more robust guarantee can be achieved by storing an LLTempBoundConnection in the transient object itself. When the object is destroyed, the listener is disconnected. Normal C++ rules around object destruction guarantee it. This idiom is widely used. There are a couple good reasons to remove the visit_and_connect() machinery: * boost::bind() and boost::weak_ptr do not constitute the wave of the future. Preferring those constructs to lambdas and std::weak_ptr penalizes new code, whether by silently failing or by discouraging use of modern idioms. * The visit_and_connect() machinery was always complicated, and apparently never very robust. Most of its promised features have been commented out over the years. Making the code base simpler, clearer and more maintainable is always a useful effect. LLEventDetail::visit_and_connect() was also used by the four LLNotificationChannelBase::connectMumble() methods. Streamline those as well. Of course, remove related test code. --- indra/llcommon/llevents.h | 343 +------------------------------------------ indra/llui/llnotifications.h | 34 +---- indra/test/llevents_tut.cpp | 222 ++++++---------------------- 3 files changed, 54 insertions(+), 545 deletions(-) (limited to 'indra/test') diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h index 253592256d..3c388bf176 100644 --- a/indra/llcommon/llevents.h +++ b/indra/llcommon/llevents.h @@ -309,37 +309,6 @@ testable: PumpNames mQueueNames; }; -/***************************************************************************** -* details -*****************************************************************************/ -namespace LLEventDetail -{ - /// Any callable capable of connecting an LLEventListener to an - /// LLStandardSignal to produce an LLBoundListener can be mapped to this - /// signature. - typedef boost::function ConnectFunc; - - /// overload of visit_and_connect() when we have a string identifier available - template - LLBoundListener visit_and_connect(const std::string& name, - const LISTENER& listener, - const ConnectFunc& connect_func); - /** - * Utility template function to use Visitor appropriately - * - * @param listener Callable to connect, typically a boost::bind() - * expression. This will be visited by Visitor using boost::visit_each(). - * @param connect_func Callable that will connect() @a listener to an - * LLStandardSignal, returning LLBoundListener. - */ - template - LLBoundListener visit_and_connect(const LISTENER& listener, - const ConnectFunc& connect_func) - { - return visit_and_connect("", listener, connect_func); - } -} // namespace LLEventDetail - /***************************************************************************** * LLEventTrackable *****************************************************************************/ @@ -374,11 +343,6 @@ namespace LLEventDetail * instance, it attempts to dereference the Foo* pointer that was * deleted but not zeroed.) * - Undefined behavior results. - * If you suspect you may encounter any such scenario, you're better off - * managing the lifespan of your object with boost::shared_ptr. - * Passing LLEventPump::listen() a boost::bind() expression - * involving a boost::weak_ptr is recognized specially, engaging - * thread-safe Boost.Signals2 machinery. */ typedef boost::signals2::trackable LLEventTrackable; @@ -517,44 +481,13 @@ public: * the result be assigned to a LLTempBoundListener or the listener is * manually disconnected when no longer needed since there will be no * way to later find and disconnect this listener manually. - * - * If (as is typical) you pass a boost::bind() expression as @a - * listener, listen() will inspect the components of that expression. If a - * bound object matches any of several cases, the connection will - * automatically be disconnected when that object is destroyed. - * - * * You bind a boost::weak_ptr. - * * Binding a boost::shared_ptr that way would ensure that the - * referenced object would @em never be destroyed, since the @c - * shared_ptr stored in the LLEventPump would remain an outstanding - * reference. Use the weaken() function to convert your @c shared_ptr to - * @c weak_ptr. Because this is easy to forget, binding a @c shared_ptr - * will produce a compile error (@c BOOST_STATIC_ASSERT failure). - * * You bind a simple pointer or reference to an object derived from - * boost::enable_shared_from_this. (UNDER CONSTRUCTION) - * * You bind a simple pointer or reference to an object derived from - * LLEventTrackable. Unlike the cases described above, though, this is - * vulnerable to a couple of cross-thread race conditions, as described - * in the LLEventTrackable documentation. */ - template - LLBoundListener listen(const std::string& name, const LISTENER& listener, + LLBoundListener listen(const std::string& name, + const LLEventListener& listener, const NameList& after=NameList(), const NameList& before=NameList()) { - // Examine listener, using our listen_impl() method to make the - // actual connection. - // This is why listen() is a template. Conversion from boost::bind() - // to LLEventListener performs type erasure, so it's important to look - // at the boost::bind object itself before that happens. - return LLEventDetail::visit_and_connect(name, - listener, - boost::bind(&LLEventPump::listen_invoke, - this, - name, - _1, - after, - before)); + return listen_impl(name, listener, after, before); } /// Get the LLBoundListener associated with the passed name (dummy @@ -598,13 +531,6 @@ private: private: - LLBoundListener listen_invoke(const std::string& name, const LLEventListener& listener, - const NameList& after, - const NameList& before) - { - return this->listen_impl(name, listener, after, before); - } - // must precede mName; see LLEventPump::LLEventPump() LLHandle mRegistry; @@ -814,261 +740,6 @@ private: LL_COMMON_API bool sendReply(const LLSD& reply, const LLSD& request, const std::string& replyKey="reply"); -/***************************************************************************** -* Underpinnings -*****************************************************************************/ -/** - * We originally provided a suite of overloaded - * LLEventTrackable::listenTo(LLEventPump&, ...) methods that would call - * LLEventPump::listen(...) and then pass the returned LLBoundListener to - * LLEventTrackable::track(). This was workable but error-prone: the coder - * must remember to call listenTo() rather than the more straightforward - * listen() method. - * - * Now we publish only the single canonical listen() method, so there's a - * uniform mechanism. Having a single way to do this is good, in that there's - * no question in the coder's mind which of several alternatives to choose. - * - * To support automatic connection management, we use boost::visit_each - * (http://www.boost.org/doc/libs/1_37_0/doc/html/boost/visit_each.html) to - * inspect each argument of a boost::bind expression. (Although the visit_each - * mechanism was first introduced with the original Boost.Signals library, it - * was only later documented.) - * - * Cases: - * * At least one of the function's arguments is a boost::weak_ptr. Pass - * the corresponding shared_ptr to slot_type::track(). Ideally that would be - * the object whose method we want to call, but in fact we do the same for - * any weak_ptr we might find among the bound arguments. If we're passing - * our bound method a weak_ptr to some object, wouldn't the destruction of - * that object invalidate the call? So we disconnect automatically when any - * such object is destroyed. This is the mechanism preferred by boost:: - * signals2. - * * One of the functions's arguments is a boost::shared_ptr. This produces - * a compile error: the bound copy of the shared_ptr stored in the - * boost_bind object stored in the signal object would make the referenced - * T object immortal. We provide a weaken() function. Pass - * weaken(your_shared_ptr) instead. (We can inspect, but not modify, the - * boost::bind object. Otherwise we'd replace the shared_ptr with weak_ptr - * implicitly and just proceed.) - * * One of the function's arguments is a plain pointer/reference to an object - * derived from boost::enable_shared_from_this. We assume that this object - * is managed using boost::shared_ptr, so we implicitly extract a shared_ptr - * and track that. (UNDER CONSTRUCTION) - * * One of the function's arguments is derived from LLEventTrackable. Pass - * the LLBoundListener to its LLEventTrackable::track(). This is vulnerable - * to a couple different race conditions, as described in LLEventTrackable - * documentation. (NOTE: Now that LLEventTrackable is a typedef for - * boost::signals2::trackable, the Signals2 library handles this itself, so - * our visitor needs no special logic for this case.) - * * Any other argument type is irrelevant to automatic connection management. - */ - -namespace LLEventDetail -{ - template - const F& unwrap(const F& f) { return f; } - - template - const F& unwrap(const boost::reference_wrapper& f) { return f.get(); } - - // Most of the following is lifted from the Boost.Signals use of - // visit_each. - template struct truth {}; - - /** - * boost::visit_each() Visitor, used on a template argument const F& - * f as follows (see visit_and_connect()): - * @code - * LLEventListener listener(f); - * Visitor visitor(listener); // bind listener so it can track() shared_ptrs - * using boost::visit_each; // allow unqualified visit_each() call for ADL - * visit_each(visitor, unwrap(f)); - * @endcode - */ - class Visitor - { - public: - /** - * Visitor binds a reference to LLEventListener so we can track() any - * shared_ptrs we find in the argument list. - */ - Visitor(LLEventListener& listener): - mListener(listener) - { - } - - /** - * boost::visit_each() calls this method for each component of a - * boost::bind() expression. - */ - template - void operator()(const T& t) const - { - decode(t, 0); - } - - private: - // decode() decides between a reference wrapper and anything else - // boost::ref() variant - template - void decode(const boost::reference_wrapper& t, int) const - { -// add_if_trackable(t.get_pointer()); - } - - // decode() anything else - template - void decode(const T& t, long) const - { - typedef truth<(boost::is_pointer::value)> is_a_pointer; - maybe_get_pointer(t, is_a_pointer()); - } - - // maybe_get_pointer() decides between a pointer and a non-pointer - // plain pointer variant - template - void maybe_get_pointer(const T& t, truth) const - { -// add_if_trackable(t); - } - - // shared_ptr variant - template - void maybe_get_pointer(const boost::shared_ptr& t, truth) const - { - // If we have a shared_ptr to this object, it doesn't matter - // whether the object is derived from LLEventTrackable, so no - // further analysis of T is needed. -// mListener.track(t); - - // Make this case illegal. Passing a bound shared_ptr to - // slot_type::track() is useless, since the bound shared_ptr will - // keep the object alive anyway! Force the coder to cast to weak_ptr. - - // Trivial as it is, make the BOOST_STATIC_ASSERT() condition - // dependent on template param so the macro is only evaluated if - // this method is in fact instantiated, as described here: - // http://www.boost.org/doc/libs/1_34_1/doc/html/boost_staticassert.html - - // ATTENTION: Don't bind a shared_ptr using - // LLEventPump::listen(boost::bind()). Doing so captures a copy of - // the shared_ptr, making the referenced object effectively - // immortal. Use the weaken() function, e.g.: - // somepump.listen(boost::bind(...weaken(my_shared_ptr)...)); - // This lets us automatically disconnect when the referenced - // object is destroyed. - BOOST_STATIC_ASSERT(sizeof(T) == 0); - } - - // weak_ptr variant - template - void maybe_get_pointer(const boost::weak_ptr& t, truth) const - { - // If we have a weak_ptr to this object, it doesn't matter - // whether the object is derived from LLEventTrackable, so no - // further analysis of T is needed. - mListener.track(t); -// std::cout << "Found weak_ptr<" << typeid(T).name() << ">!\n"; - } - -#if 0 - // reference to anything derived from boost::enable_shared_from_this - template - inline void maybe_get_pointer(const boost::enable_shared_from_this& ct, - truth) const - { - // Use the slot_type::track(shared_ptr) mechanism. Cast away - // const-ness because (in our code base anyway) it's unusual - // to find shared_ptr. - boost::enable_shared_from_this& - t(const_cast&>(ct)); - std::cout << "Capturing shared_from_this()" << std::endl; - boost::shared_ptr sp(t.shared_from_this()); -/*==========================================================================*| - std::cout << "Capturing weak_ptr" << std::endl; - boost::weak_ptr wp(sp); -|*==========================================================================*/ - std::cout << "Tracking shared__ptr" << std::endl; - mListener.track(sp); - } -#endif - - // non-pointer variant - template - void maybe_get_pointer(const T& t, truth) const - { - // Take the address of this object, because the object itself may be - // trackable -// add_if_trackable(boost::addressof(t)); - } - -/*==========================================================================*| - // add_if_trackable() adds LLEventTrackable objects to mTrackables - inline void add_if_trackable(const LLEventTrackable* t) const - { - if (t) - { - } - } - - // pointer to anything not an LLEventTrackable subclass - inline void add_if_trackable(const void*) const - { - } - - // pointer to free function - // The following construct uses the preprocessor to generate - // add_if_trackable() overloads accepting pointer-to-function taking - // 0, 1, ..., LLEVENTS_LISTENER_ARITY parameters of arbitrary type. -#define BOOST_PP_LOCAL_MACRO(n) \ - template \ - inline void \ - add_if_trackable(R (*)(BOOST_PP_ENUM_PARAMS(n, T))) const \ - { \ - } -#define BOOST_PP_LOCAL_LIMITS (0, LLEVENTS_LISTENER_ARITY) -#include BOOST_PP_LOCAL_ITERATE() -#undef BOOST_PP_LOCAL_MACRO -#undef BOOST_PP_LOCAL_LIMITS -|*==========================================================================*/ - - /// Bind a reference to the LLEventListener to call its track() method. - LLEventListener& mListener; - }; - - /** - * Utility template function to use Visitor appropriately - * - * @param raw_listener Callable to connect, typically a boost::bind() - * expression. This will be visited by Visitor using boost::visit_each(). - * @param connect_funct Callable that will connect() @a raw_listener to an - * LLStandardSignal, returning LLBoundListener. - */ - template - LLBoundListener visit_and_connect(const std::string& name, - const LISTENER& raw_listener, - const ConnectFunc& connect_func) - { - // Capture the listener - LLEventListener listener(raw_listener); - // Define our Visitor, binding the listener so we can call - // listener.track() if we discover any shared_ptr. - LLEventDetail::Visitor visitor(listener); - // Allow unqualified visit_each() call for ADL - using boost::visit_each; - // Visit each component of a boost::bind() expression. Pass - // 'raw_listener', our template argument, rather than 'listener' from - // which type details have been erased. unwrap() comes from - // Boost.Signals, in case we were passed a boost::ref(). - visit_each(visitor, LLEventDetail::unwrap(raw_listener)); - // Make the connection using passed function. - return connect_func(listener); - } -} // namespace LLEventDetail - // Somewhat to my surprise, passing boost::bind(...boost::weak_ptr...) to // listen() fails in Boost code trying to instantiate LLEventListener (i.e. // LLStandardSignal::slot_type) because the boost::get_pointer() utility function isn't @@ -1079,12 +750,4 @@ namespace boost T* get_pointer(const weak_ptr& ptr) { return shared_ptr(ptr).get(); } } -/// Since we forbid use of listen(boost::bind(...shared_ptr...)), provide an -/// easy way to cast to the corresponding weak_ptr. -template -boost::weak_ptr weaken(const boost::shared_ptr& ptr) -{ - return boost::weak_ptr(ptr); -} - #endif /* ! defined(LL_LLEVENTS_H) */ diff --git a/indra/llui/llnotifications.h b/indra/llui/llnotifications.h index 62cf41256b..cac687f53d 100644 --- a/indra/llui/llnotifications.h +++ b/indra/llui/llnotifications.h @@ -746,42 +746,24 @@ public: virtual ~LLNotificationChannelBase() {} // you can also connect to a Channel, so you can be notified of // changes to this channel - template - LLBoundListener connectChanged(const LISTENER& slot) + LLBoundListener connectChanged(const LLEventListener& slot) { - // Examine slot to see if it binds an LLEventTrackable subclass, or a - // boost::shared_ptr to something, or a boost::weak_ptr to something. // Call this->connectChangedImpl() to actually connect it. - return LLEventDetail::visit_and_connect(slot, - boost::bind(&LLNotificationChannelBase::connectChangedImpl, - this, - _1)); + return connectChangedImpl(slot); } - template - LLBoundListener connectAtFrontChanged(const LISTENER& slot) + LLBoundListener connectAtFrontChanged(const LLEventListener& slot) { - return LLEventDetail::visit_and_connect(slot, - boost::bind(&LLNotificationChannelBase::connectAtFrontChangedImpl, - this, - _1)); + return connectAtFrontChangedImpl(slot); } - template - LLBoundListener connectPassedFilter(const LISTENER& slot) + LLBoundListener connectPassedFilter(const LLEventListener& slot) { // see comments in connectChanged() - return LLEventDetail::visit_and_connect(slot, - boost::bind(&LLNotificationChannelBase::connectPassedFilterImpl, - this, - _1)); + return connectPassedFilterImpl(slot); } - template - LLBoundListener connectFailedFilter(const LISTENER& slot) + LLBoundListener connectFailedFilter(const LLEventListener& slot) { // see comments in connectChanged() - return LLEventDetail::visit_and_connect(slot, - boost::bind(&LLNotificationChannelBase::connectFailedFilterImpl, - this, - _1)); + return connectFailedFilterImpl(slot); } // use this when items change or to add a new one diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp index 9ff0f5086d..a8a3188249 100644 --- a/indra/test/llevents_tut.cpp +++ b/indra/test/llevents_tut.cpp @@ -492,196 +492,60 @@ void events_object::test<10>() heaptest.stopListening("temp"); } -template<> template<> -void events_object::test<11>() -{ - set_test_name("listen(boost::bind(...weak_ptr...))"); - // listen() detecting weak_ptr in boost::bind() object - bool live = false; - LLEventPump& heaptest(pumps.obtain("heaptest")); - LLBoundListener connection; - ensure("default state", !connection.connected()); - { - boost::shared_ptr newListener(new TempListener("heap", live)); - newListener->reset(); - ensure("TempListener constructed", live); - connection = heaptest.listen(newListener->getName(), - boost::bind(&Listener::call, - weaken(newListener), - _1)); - ensure("new connection", connection.connected()); - heaptest.post(1); - check_listener("received", *newListener, 1); - } // presumably this will make newListener go away? - // verify that - ensure("TempListener destroyed", !live); - ensure("implicit disconnect", !connection.connected()); - // now just make sure we don't blow up trying to access a freed object! - heaptest.post(2); -} - -template<> template<> -void events_object::test<12>() -{ - set_test_name("listen(boost::bind(...shared_ptr...))"); - /*==========================================================================*| - // DISABLED because I've made this case produce a compile error. - // Following the error leads the disappointed dev to a comment - // instructing her to use the weaken() function to bind a weak_ptr - // instead of binding a shared_ptr, and explaining why. I know of - // no way to use TUT to code a repeatable test in which the expected - // outcome is a compile error. The interested reader is invited to - // uncomment this block and build to see for herself. - - // listen() detecting shared_ptr in boost::bind() object - bool live = false; - LLEventPump& heaptest(pumps.obtain("heaptest")); - LLBoundListener connection; - std::string listenerName("heap"); - ensure("default state", !connection.connected()); - { - boost::shared_ptr newListener(new TempListener(listenerName, live)); - ensure_equals("use_count", newListener.use_count(), 1); - newListener->reset(); - ensure("TempListener constructed", live); - connection = heaptest.listen(newListener->getName(), - boost::bind(&Listener::call, newListener, _1)); - ensure("new connection", connection.connected()); - ensure_equals("use_count", newListener.use_count(), 2); - heaptest.post(1); - check_listener("received", *newListener, 1); - } // this should make newListener go away... - // Unfortunately, the fact that we've bound a shared_ptr by value into - // our LLEventPump means that copy will keep the referenced object alive. - ensure("TempListener still alive", live); - ensure("still connected", connection.connected()); - // disconnecting explicitly should delete the TempListener... - heaptest.stopListening(listenerName); -#if 0 // however, in my experience, it does not. I don't know why not. - // Ah: on 2009-02-19, Frank Mori Hess, author of the Boost.Signals2 - // library, stated on the boost-users mailing list: - // http://www.nabble.com/Re%3A--signals2--review--The-review-of-the-signals2-library-(formerly-thread_safe_signals)-begins-today%2C-Nov-1st-p22102367.html - // "It will get destroyed eventually. The signal cleans up its slot - // list little by little during connect/invoke. It doesn't immediately - // remove disconnected slots from the slot list since other threads - // might be using the same slot list concurrently. It might be - // possible to make it immediately reset the shared_ptr owning the - // slot though, leaving an empty shared_ptr in the slot list, since - // that wouldn't invalidate any iterators." - ensure("TempListener destroyed", ! live); - ensure("implicit disconnect", ! connection.connected()); -#endif // 0 - // now just make sure we don't blow up trying to access a freed object! - heaptest.post(2); -|*==========================================================================*/ -} - class TempTrackableListener: public TempListener, public LLEventTrackable { public: -TempTrackableListener(const std::string& name, bool& liveFlag): - TempListener(name, liveFlag) -{} + TempTrackableListener(const std::string& name, bool& liveFlag): + TempListener(name, liveFlag) + {} }; template<> template<> -void events_object::test<13>() -{ -set_test_name("listen(boost::bind(...TempTrackableListener ref...))"); -bool live = false; -LLEventPump& heaptest(pumps.obtain("heaptest")); -LLBoundListener connection; +void events_object::test<11>() { - TempTrackableListener tempListener("temp", live); - ensure("TempTrackableListener constructed", live); - connection = heaptest.listen(tempListener.getName(), - boost::bind(&TempTrackableListener::call, - boost::ref(tempListener), _1)); - heaptest.post(1); - check_listener("received", tempListener, 1); -} // presumably this will make tempListener go away? -// verify that -ensure("TempTrackableListener destroyed", ! live); -ensure("implicit disconnect", ! connection.connected()); -// now just make sure we don't blow up trying to access a freed object! -heaptest.post(2); + set_test_name("listen(boost::bind(...TempTrackableListener ref...))"); + bool live = false; + LLEventPump& heaptest(pumps.obtain("heaptest")); + LLBoundListener connection; + { + TempTrackableListener tempListener("temp", live); + ensure("TempTrackableListener constructed", live); + connection = heaptest.listen(tempListener.getName(), + boost::bind(&TempTrackableListener::call, + boost::ref(tempListener), _1)); + heaptest.post(1); + check_listener("received", tempListener, 1); + } // presumably this will make tempListener go away? + // verify that + ensure("TempTrackableListener destroyed", ! live); + ensure("implicit disconnect", ! connection.connected()); + // now just make sure we don't blow up trying to access a freed object! + heaptest.post(2); } template<> template<> -void events_object::test<14>() -{ -set_test_name("listen(boost::bind(...TempTrackableListener pointer...))"); -bool live = false; -LLEventPump& heaptest(pumps.obtain("heaptest")); -LLBoundListener connection; +void events_object::test<12>() { - TempTrackableListener* newListener(new TempTrackableListener("temp", live)); - ensure("TempTrackableListener constructed", live); - connection = heaptest.listen(newListener->getName(), - boost::bind(&TempTrackableListener::call, - newListener, _1)); - heaptest.post(1); - check_listener("received", *newListener, 1); - // explicitly destroy newListener - delete newListener; -} -// verify that -ensure("TempTrackableListener destroyed", ! live); -ensure("implicit disconnect", ! connection.connected()); -// now just make sure we don't blow up trying to access a freed object! -heaptest.post(2); + set_test_name("listen(boost::bind(...TempTrackableListener pointer...))"); + bool live = false; + LLEventPump& heaptest(pumps.obtain("heaptest")); + LLBoundListener connection; + { + TempTrackableListener* newListener(new TempTrackableListener("temp", live)); + ensure("TempTrackableListener constructed", live); + connection = heaptest.listen(newListener->getName(), + boost::bind(&TempTrackableListener::call, + newListener, _1)); + heaptest.post(1); + check_listener("received", *newListener, 1); + // explicitly destroy newListener + delete newListener; + } + // verify that + ensure("TempTrackableListener destroyed", ! live); + ensure("implicit disconnect", ! connection.connected()); + // now just make sure we don't blow up trying to access a freed object! + heaptest.post(2); } -class TempSharedListener: public TempListener, -public boost::enable_shared_from_this -{ -public: -TempSharedListener(const std::string& name, bool& liveFlag): - TempListener(name, liveFlag) -{} -}; - -template<> template<> -void events_object::test<15>() -{ - set_test_name("listen(boost::bind(...TempSharedListener ref...))"); -#if 0 -bool live = false; -LLEventPump& heaptest(pumps.obtain("heaptest")); -LLBoundListener connection; -{ - // We MUST have at least one shared_ptr to an - // enable_shared_from_this subclass object before - // shared_from_this() can work. - boost::shared_ptr - tempListener(new TempSharedListener("temp", live)); - ensure("TempSharedListener constructed", live); - // However, we're not passing either the shared_ptr or its - // corresponding weak_ptr -- instead, we're passing a reference to - // the TempSharedListener. -/*==========================================================================*| - std::cout << "Capturing const ref" << std::endl; - const boost::enable_shared_from_this& cref(*tempListener); - std::cout << "Capturing const ptr" << std::endl; - const boost::enable_shared_from_this* cp(&cref); - std::cout << "Capturing non-const ptr" << std::endl; - boost::enable_shared_from_this* p(const_cast*>(cp)); - std::cout << "Capturing shared_from_this()" << std::endl; - boost::shared_ptr sp(p->shared_from_this()); - std::cout << "Capturing weak_ptr" << std::endl; - boost::weak_ptr wp(weaken(sp)); - std::cout << "Binding weak_ptr" << std::endl; -|*==========================================================================*/ - connection = heaptest.listen(tempListener->getName(), - boost::bind(&TempSharedListener::call, *tempListener, _1)); - heaptest.post(1); - check_listener("received", *tempListener, 1); -} // presumably this will make tempListener go away? -// verify that -ensure("TempSharedListener destroyed", ! live); -ensure("implicit disconnect", ! connection.connected()); -// now just make sure we don't blow up trying to access a freed object! -heaptest.post(2); -#endif // 0 -} } // namespace tut -- cgit v1.3 From 79a3e391d3c992925230ff01551747e7edccb0ca Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 17 Oct 2019 13:26:51 -0400 Subject: DRTVWR-476: Kill LLEventQueue, per-frame LLEventPump::flush() calls. No one uses LLEventQueue to defer posted events until the next mainloop tick -- and with LLCoros moving to Boost.Fiber, cross-coroutine event posting works that way anyway, making LLEventQueue pretty unnecessary. The static RegisterFlush instance in llevents.cpp was used to call LLEventPumps::flush() once per mainloop tick, which in turn called flush() on every registered LLEventPump. But the only reason for that mechanism was to support LLEventQueue. In fact, when LLEventMailDrop overrode its flush() method for something quite different, it was startling to find that the new flush() override was being called once per frame -- which caused at least one fairly mysterious bug. Remove RegisterFlush. Both LLEventPumps::flush() and LLEventPump::flush() remain for now, though intended usage is unclear. Eliminating LLEventQueue means we must at least repurpose LLEventPumps::mQueueNames, a map intended to make LLEventPumps::obtain() instantiate an LLEventQueue rather than the default LLEventPump. Replace it with mFactories, a map from desired instance name to a callable returning LLEventPump*. New map initialization syntax plus lambda support allows us to populate that map at compile time with little lambdas returning the correct subclass instance. Similarly, LLLeapListener::newpump() used to check the ["type"] entry in the LLSD request specifically for "LLEventQueue". Introduce another such map in llleaplistener.cpp for potential future extensibility. Eliminate the LLEventQueue-specific test. --- indra/llcommon/llevents.cpp | 102 +++++--------------------------------- indra/llcommon/llevents.h | 39 +++------------ indra/llcommon/llleaplistener.cpp | 21 ++++++-- indra/test/llevents_tut.cpp | 57 ++++----------------- 4 files changed, 46 insertions(+), 173 deletions(-) (limited to 'indra/test') diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp index 186e710c43..d31f5f2d32 100644 --- a/indra/llcommon/llevents.cpp +++ b/indra/llcommon/llevents.cpp @@ -64,51 +64,16 @@ #endif /***************************************************************************** -* queue_names: specify LLEventPump names that should be instantiated as -* LLEventQueue -*****************************************************************************/ -/** - * At present, we recognize particular requested LLEventPump names as needing - * LLEventQueues. Later on we'll migrate this information to an external - * configuration file. - */ -const char* queue_names[] = -{ - "placeholder - replace with first real name string" -}; - -/***************************************************************************** -* If there's a "mainloop" pump, listen on that to flush all LLEventQueues +* LLEventPumps *****************************************************************************/ -struct RegisterFlush : public LLEventTrackable +LLEventPumps::PumpFactories LLEventPumps::mFactories { - RegisterFlush(): - pumps(LLEventPumps::instance()) - { - pumps.obtain("mainloop").listen("flushLLEventQueues", boost::bind(&RegisterFlush::flush, this, _1)); - } - bool flush(const LLSD&) - { - pumps.flush(); - return false; - } - ~RegisterFlush() - { - // LLEventTrackable handles stopListening for us. - } - LLEventPumps& pumps; + // LLEventStream is the default for obtain(), so even if somebody DOES + // call obtain("placeholder"), this sample entry won't break anything. + { "placeholder", [](const std::string& name) { return new LLEventStream(name); } } }; -static RegisterFlush registerFlush; -/***************************************************************************** -* LLEventPumps -*****************************************************************************/ -LLEventPumps::LLEventPumps(): - // Until we migrate this information to an external config file, - // initialize mQueueNames from the static queue_names array. - mQueueNames(boost::begin(queue_names), boost::end(queue_names)) -{ -} +LLEventPumps::LLEventPumps() {} LLEventPump& LLEventPumps::obtain(const std::string& name) { @@ -121,10 +86,10 @@ LLEventPump& LLEventPumps::obtain(const std::string& name) } // Here we must instantiate an LLEventPump subclass. LLEventPump* newInstance; - // Should this name be an LLEventQueue? - PumpNames::const_iterator nfound = mQueueNames.find(name); - if (nfound != mQueueNames.end()) - newInstance = new LLEventQueue(name); + // Do we have a predefined factory for this instance name? + PumpFactories::const_iterator nfound = mFactories.find(name); + if (nfound != mFactories.end()) + newInstance = (nfound->second)(name); else newInstance = new LLEventStream(name); // LLEventPump's constructor implicitly registers each new instance in @@ -144,14 +109,13 @@ bool LLEventPumps::post(const std::string&name, const LLSD&message) return (*found).second->post(message); } - void LLEventPumps::flush() { // Flush every known LLEventPump instance. Leave it up to each instance to // decide what to do with the flush() call. - for (PumpMap::iterator pmi = mPumpMap.begin(), pmend = mPumpMap.end(); pmi != pmend; ++pmi) + for (PumpMap::value_type& pair : mPumpMap) { - pmi->second->flush(); + pair.second->flush(); } } @@ -605,48 +569,6 @@ LLBoundListener LLEventMailDrop::listen_impl(const std::string& name, void LLEventMailDrop::discard() { mEventHistory.clear(); - LLEventStream::flush(); -} - -/***************************************************************************** -* LLEventQueue -*****************************************************************************/ -bool LLEventQueue::post(const LLSD& event) -{ - if (mEnabled) - { - // Defer sending this event by queueing it until flush() - mEventQueue.push_back(event); - } - // Unconditionally return false. We won't know until flush() whether a - // listener claims to have handled the event -- meanwhile, don't block - // other listeners. - return false; -} - -void LLEventQueue::flush() -{ - if(!mSignal) return; - - // Consider the case when a given listener on this LLEventQueue posts yet - // another event on the same queue. If we loop over mEventQueue directly, - // we'll end up processing all those events during the same flush() call - // -- rather like an EventStream. Instead, copy mEventQueue and clear it, - // so that any new events posted to this LLEventQueue during flush() will - // be processed in the *next* flush() call. - EventQueue queue(mEventQueue); - mEventQueue.clear(); - // NOTE NOTE NOTE: Any new access to member data beyond this point should - // cause us to move our LLStandardSignal object to a pimpl class along - // with said member data. Then the local shared_ptr will preserve both. - - // DEV-43463: capture a local copy of mSignal. See LLEventStream::post() - // for detailed comments. - boost::shared_ptr signal(mSignal); - for ( ; ! queue.empty(); queue.pop_front()) - { - (*signal)(queue.front()); - } } /***************************************************************************** diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h index ce2aa2f3c9..c55351919e 100644 --- a/indra/llcommon/llevents.h +++ b/indra/llcommon/llevents.h @@ -37,6 +37,7 @@ #include #include #include +#include #if LL_WINDOWS #pragma warning (push) #pragma warning (disable : 4263) // boost::signals2::expired_slot::what() has const mismatch @@ -55,7 +56,6 @@ #include #include // reference_wrapper #include -#include #include #include "llsd.h" #include "llsingleton.h" @@ -303,10 +303,10 @@ testable: // destroyed. typedef std::set PumpSet; PumpSet mOurPumps; - // LLEventPump names that should be instantiated as LLEventQueue rather - // than as LLEventStream - typedef std::set PumpNames; - PumpNames mQueueNames; + // LLEventPump subclasses that should be instantiated for particular + // instance names + typedef std::map> PumpFactories; + static PumpFactories mFactories; }; /***************************************************************************** @@ -351,7 +351,7 @@ typedef boost::signals2::trackable LLEventTrackable; *****************************************************************************/ /** * LLEventPump is the base class interface through which we access the - * concrete subclasses LLEventStream and LLEventQueue. + * concrete subclasses such as LLEventStream. * * @NOTE * LLEventPump derives from LLEventTrackable so that when you "chain" @@ -594,11 +594,10 @@ public: * event *must* eventually reach a listener that will consume it, else the * queue will grow to arbitrary length. * - * @NOTE: When using an LLEventMailDrop (or LLEventQueue) with a LLEventTimeout or + * @NOTE: When using an LLEventMailDrop with an LLEventTimeout or * LLEventFilter attaching the filter downstream, using Timeout's constructor will * cause the MailDrop to discharge any of its stored events. The timeout should * instead be connected upstream using its listen() method. - * See llcoro::suspendUntilEventOnWithTimeout() for an example. */ class LL_COMMON_API LLEventMailDrop : public LLEventStream { @@ -622,30 +621,6 @@ private: EventList mEventHistory; }; -/***************************************************************************** -* LLEventQueue -*****************************************************************************/ -/** - * LLEventQueue is a LLEventPump whose post() method defers calling registered - * listeners until flush() is called. - */ -class LL_COMMON_API LLEventQueue: public LLEventPump -{ -public: - LLEventQueue(const std::string& name, bool tweak=false): LLEventPump(name, tweak) {} - virtual ~LLEventQueue() {} - - /// Post an event to all listeners - virtual bool post(const LLSD& event); - - /// flush queued events - virtual void flush(); - -private: - typedef std::deque EventQueue; - EventQueue mEventQueue; -}; - /***************************************************************************** * LLReqID *****************************************************************************/ diff --git a/indra/llcommon/llleaplistener.cpp b/indra/llcommon/llleaplistener.cpp index f50bacb1e8..0d18e5fff9 100644 --- a/indra/llcommon/llleaplistener.cpp +++ b/indra/llcommon/llleaplistener.cpp @@ -14,6 +14,8 @@ // associated header #include "llleaplistener.h" // STL headers +#include +#include // std headers // external library headers #include @@ -119,6 +121,18 @@ LLLeapListener::~LLLeapListener() } } +namespace +{ + +static std::map> factories +{ + // tweak name for uniqueness + { "LLEventStream", [](const std::string& name){ return new LLEventStream(name, true); } }, + { "LLEventMailDrop", [](const std::string& name){ return new LLEventMailDrop(name, true); } } +}; + +} // anonymous namespace + void LLLeapListener::newpump(const LLSD& request) { Response reply(LLSD(), request); @@ -127,13 +141,14 @@ void LLLeapListener::newpump(const LLSD& request) LLSD const & type = request["type"]; LLEventPump * new_pump = NULL; - if (type.asString() == "LLEventQueue") + auto found = factories.find(type.asString()); + if (found != factories.end()) { - new_pump = new LLEventQueue(name, true); // tweak name for uniqueness + new_pump = (found->second)(name); } else { - if (! (type.isUndefined() || type.asString() == "LLEventStream")) + if (! type.isUndefined()) { reply.warn(STRINGIZE("unknown 'type' " << type << ", using LLEventStream")); } diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp index a8a3188249..17f64a4953 100644 --- a/indra/test/llevents_tut.cpp +++ b/indra/test/llevents_tut.cpp @@ -91,9 +91,7 @@ template<> template<> void events_object::test<1>() { set_test_name("basic operations"); - // Now there's a static constructor in llevents.cpp that registers on - // the "mainloop" pump to call LLEventPumps::flush(). - // Actually -- having to modify this to track the statically- + // Having to modify this to track the statically- // constructed pumps in other TUT modules in this giant monolithic test // executable isn't such a hot idea. // ensure_equals("initial pump", pumps.mPumpMap.size(), 1); @@ -209,43 +207,6 @@ bool chainEvents(Listener& someListener, const LLSD& event) template<> template<> void events_object::test<3>() -{ - set_test_name("LLEventQueue delayed action"); - // This access is NOT legal usage: we can do it only because we're - // hacking private for test purposes. Normally we'd either compile in - // a particular name, or (later) edit a config file. - pumps.mQueueNames.insert("login"); - LLEventPump& login(pumps.obtain("login")); - // The "mainloop" pump is special: posting on that implicitly calls - // LLEventPumps::flush(), which in turn should flush our "login" - // LLEventQueue. - LLEventPump& mainloop(pumps.obtain("mainloop")); - ensure("LLEventQueue leaf class", dynamic_cast (&login)); - listener0.listenTo(login); - listener0.reset(0); - login.post(1); - check_listener("waiting for queued event", listener0, 0); - mainloop.post(LLSD()); - check_listener("got queued event", listener0, 1); - login.stopListening(listener0.getName()); - // Verify that when an event handler posts a new event on the same - // LLEventQueue, it doesn't get processed in the same flush() call -- - // it waits until the next flush() call. - listener0.reset(17); - login.listen("chainEvents", boost::bind(chainEvents, boost::ref(listener0), _1)); - login.post(1); - check_listener("chainEvents(1) not yet called", listener0, 17); - mainloop.post(LLSD()); - check_listener("chainEvents(1) called", listener0, 1); - mainloop.post(LLSD()); - check_listener("chainEvents(0) called", listener0, 0); - mainloop.post(LLSD()); - check_listener("chainEvents(-1) not called", listener0, 0); - login.stopListening("chainEvents"); -} - -template<> template<> -void events_object::test<4>() { set_test_name("explicitly-instantiated LLEventStream"); // Explicitly instantiate an LLEventStream, and verify that it @@ -270,7 +231,7 @@ void events_object::test<4>() } template<> template<> -void events_object::test<5>() +void events_object::test<4>() { set_test_name("stopListening()"); LLEventPump& login(pumps.obtain("login")); @@ -284,7 +245,7 @@ void events_object::test<5>() } template<> template<> -void events_object::test<6>() +void events_object::test<5>() { set_test_name("chaining LLEventPump instances"); LLEventPump& upstream(pumps.obtain("upstream")); @@ -309,7 +270,7 @@ void events_object::test<6>() } template<> template<> -void events_object::test<7>() +void events_object::test<6>() { set_test_name("listener dependency order"); typedef LLEventPump::NameList NameList; @@ -391,7 +352,7 @@ void events_object::test<7>() } template<> template<> -void events_object::test<8>() +void events_object::test<7>() { set_test_name("tweaked and untweaked LLEventPump instance names"); { // nested scope @@ -423,7 +384,7 @@ void eventSource(const LLListenerOrPumpName& listener) } template<> template<> -void events_object::test<9>() +void events_object::test<8>() { set_test_name("LLListenerOrPumpName"); // Passing a boost::bind() expression to LLListenerOrPumpName @@ -464,7 +425,7 @@ private: }; template<> template<> -void events_object::test<10>() +void events_object::test<9>() { set_test_name("listen(boost::bind(...TempListener...))"); // listen() can't do anything about a plain TempListener instance: @@ -501,7 +462,7 @@ public: }; template<> template<> -void events_object::test<11>() +void events_object::test<10>() { set_test_name("listen(boost::bind(...TempTrackableListener ref...))"); bool live = false; @@ -524,7 +485,7 @@ void events_object::test<11>() } template<> template<> -void events_object::test<12>() +void events_object::test<11>() { set_test_name("listen(boost::bind(...TempTrackableListener pointer...))"); bool live = false; -- cgit v1.3 From 28a54c2f7b25b9fda763c51746701f0cd21c8018 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 22 Oct 2019 16:49:29 -0400 Subject: DRTVWR-476: Infrastructure to help manage long-lived coroutines. Introduce LLCoros::Stop exception, with subclasses Stopping, Stopped and Shutdown. Add LLCoros::checkStop(), intended to be called periodically by any coroutine with nontrivial lifespan. It checks the LLApp status and, unless isRunning(), throws one of these new exceptions. Make LLCoros::toplevel() catch Stop specially and log forcible coroutine termination. Now that LLApp status matters even in a test program, introduce a trivial LLTestApp subclass whose sole function is to make isRunning() true. (LLApp::setStatus() is protected: only a subclass can call it.) Add LLTestApp instances to lleventcoro_test.cpp and lllogin_test.cpp. Make LLCoros::toplevel() accept parameters by value rather than by const reference so we can continue using them even after context switches. Make private LLCoros::get_CoroData() static. Given that we've observed some coroutines living past LLCoros destruction, making the caller call LLCoros::instance() is more dangerous than encapsulating it within a static method -- since the encapsulated call can check LLCoros::wasDeleted() first and do something reasonable instead. This also eliminates the need for both a const and non-const overload. Defend LLCoros::delete_CoroData() (cleanup function for fiber_specific_ptr for CoroData, implicitly called after coroutine termination) against calls after ~LLCoros(). Add a status string to coroutine-local data, with LLCoro::setStatus(), getStatus() and RAII class TempStatus. Add an optional 'when' string argument to LLCoros::printActiveCoroutines(). Make ~LLCoros() print the coroutines still active at destruction. --- indra/llcommon/llcoros.cpp | 105 ++++++++++++++++----- indra/llcommon/llcoros.h | 66 ++++++++++++- indra/llcommon/tests/lleventcoro_test.cpp | 2 + indra/test/lltestapp.h | 34 +++++++ .../viewer_components/login/tests/lllogin_test.cpp | 2 + 5 files changed, 181 insertions(+), 28 deletions(-) create mode 100644 indra/test/lltestapp.h (limited to 'indra/test') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index 37bcfc3242..78a0c5d225 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -48,6 +48,7 @@ #undef BOOST_DISABLE_ASSERTS #endif // other Linden headers +#include "llapp.h" #include "lltimer.h" #include "llevents.h" #include "llerror.h" @@ -58,10 +59,15 @@ #include #endif - -const LLCoros::CoroData& LLCoros::get_CoroData(const std::string& caller) const +// static +LLCoros::CoroData& LLCoros::get_CoroData(const std::string& caller) { - CoroData* current = mCurrent.get(); + CoroData* current{ nullptr }; + // be careful about attempted accesses in the final throes of app shutdown + if (! wasDeleted()) + { + current = instance().mCurrent.get(); + } // For the main() coroutine, the one NOT explicitly launched by launch(), // we never explicitly set mCurrent. Use a static CoroData instance with // canonical values. @@ -78,12 +84,6 @@ const LLCoros::CoroData& LLCoros::get_CoroData(const std::string& caller) const return *current; } -LLCoros::CoroData& LLCoros::get_CoroData(const std::string& caller) -{ - // reuse const implementation, just cast away const-ness of result - return const_cast(const_cast(this)->get_CoroData(caller)); -} - //static LLCoros::coro::id LLCoros::get_self() { @@ -93,7 +93,7 @@ LLCoros::coro::id LLCoros::get_self() //static void LLCoros::set_consuming(bool consuming) { - CoroData& data(LLCoros::instance().get_CoroData("set_consuming()")); + CoroData& data(get_CoroData("set_consuming()")); // DO NOT call this on the main() coroutine. llassert_always(! data.mName.empty()); data.mConsuming = consuming; @@ -102,7 +102,19 @@ void LLCoros::set_consuming(bool consuming) //static bool LLCoros::get_consuming() { - return LLCoros::instance().get_CoroData("get_consuming()").mConsuming; + return get_CoroData("get_consuming()").mConsuming; +} + +// static +void LLCoros::setStatus(const std::string& status) +{ + get_CoroData("setStatus()").mStatus = status; +} + +// static +std::string LLCoros::getStatus() +{ + return get_CoroData("getStatus()").mStatus; } LLCoros::LLCoros(): @@ -118,6 +130,11 @@ LLCoros::LLCoros(): { } +LLCoros::~LLCoros() +{ + printActiveCoroutines("at LLCoros destruction"); +} + std::string LLCoros::generateDistinctName(const std::string& prefix) const { static int unique = 0; @@ -166,19 +183,19 @@ void LLCoros::setStackSize(S32 stacksize) mStackSize = stacksize; } -void LLCoros::printActiveCoroutines() +void LLCoros::printActiveCoroutines(const std::string& when) { - LL_INFOS("LLCoros") << "Number of active coroutines: " << (S32)mCoros.size() << LL_ENDL; + LL_INFOS("LLCoros") << "Number of active coroutines " << when + << ": " << (S32)mCoros.size() << LL_ENDL; if (mCoros.size() > 0) { LL_INFOS("LLCoros") << "-------------- List of active coroutines ------------"; - CoroMap::iterator iter; - CoroMap::iterator end = mCoros.end(); F64 time = LLTimer::getTotalSeconds(); - for (iter = mCoros.begin(); iter != end; iter++) + for (const auto& pair : mCoros) { - F64 life_time = time - iter->second->mCreationTime; - LL_CONT << LL_NEWLINE << "Name: " << iter->first << " life: " << life_time; + F64 life_time = time - pair.second->mCreationTime; + LL_CONT << LL_NEWLINE << pair.first << ' ' << pair.second->mStatus + << " life: " << life_time; } LL_CONT << LL_ENDL; LL_INFOS("LLCoros") << "-----------------------------------------------------" << LL_ENDL; @@ -244,11 +261,19 @@ void LLCoros::winlevel(const callable_t& callable) #endif // Top-level wrapper around caller's coroutine callable. -void LLCoros::toplevel(const std::string& name, const callable_t& callable) +// Normally we like to pass strings and such by const reference -- but in this +// case, we WANT to copy both the name and the callable to our local stack! +void LLCoros::toplevel(std::string name, callable_t callable) { CoroData* corodata = new CoroData(name); - // Store it in our pointer map. Oddly, must cast away const-ness of key. - mCoros.insert(const_cast(name), corodata); + if (corodata == NULL) + { + // Out of memory? + printActiveCoroutines(); + LL_ERRS("LLCoros") << "Failed to start coroutine: " << name << " Stacksize: " << mStackSize << " Total coroutines: " << mCoros.size() << LL_ENDL; + } + // Store it in our pointer map. + mCoros.insert(name, corodata); // also set it as current mCurrent.reset(corodata); @@ -261,18 +286,39 @@ void LLCoros::toplevel(const std::string& name, const callable_t& callable) callable(); #endif } + catch (const Stop& exc) + { + LL_INFOS("LLCoros") << "coroutine " << name << " terminating because " + << exc.what() << LL_ENDL; + } catch (const LLContinueError&) { // Any uncaught exception derived from LLContinueError will be caught // here and logged. This coroutine will terminate but the rest of the // viewer will carry on. - LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << corodata->mName)); + LOG_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << name)); } catch (...) { // Any OTHER kind of uncaught exception will cause the viewer to // crash, hopefully informatively. - CRASH_ON_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << corodata->mName)); + CRASH_ON_UNHANDLED_EXCEPTION(STRINGIZE("coroutine " << name)); + } +} + +void LLCoros::checkStop() +{ + if (wasDeleted()) + { + LLTHROW(Shutdown("LLCoros was deleted")); + } + if (LLApp::isStopped()) + { + LLTHROW(Stopped("viewer is stopped")); + } + if (! LLApp::isRunning()) + { + LLTHROW(Stopping("viewer is stopping")); } } @@ -288,6 +334,19 @@ void LLCoros::delete_CoroData(CoroData* cdptr) { // This custom cleanup function is necessarily static. Find and bind the // LLCoros instance. + // In case the LLCoros instance has already been deleted, just warn and + // scoot. We do NOT want to reinstantiate LLCoros during shutdown! + if (wasDeleted()) + { + // The LLSingletons involved in logging may have been deleted too. + // This warning may help developers track down coroutines that have + // not yet been cleaned up. + // But cdptr is very likely a dangling pointer by this time, so don't + // try to dereference mName. + logwarns("Coroutine terminating after LLCoros instance deleted"); + return; + } + LLCoros& self(LLCoros::instance()); // We set mCurrent on entry to a new fiber, expecting that the // corresponding entry has already been stored in mCoros. It is an diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h index dedb6c8eca..de7b691284 100644 --- a/indra/llcommon/llcoros.h +++ b/indra/llcommon/llcoros.h @@ -29,6 +29,7 @@ #if ! defined(LL_LLCOROS_H) #define LL_LLCOROS_H +#include "llexception.h" #include #include #include @@ -74,6 +75,7 @@ class LL_COMMON_API LLCoros: public LLSingleton { LLSINGLETON(LLCoros); + ~LLCoros(); public: /// The viewer's use of the term "coroutine" became deeply embedded before /// the industry term "fiber" emerged to distinguish userland threads from @@ -147,8 +149,8 @@ public: */ void setStackSize(S32 stacksize); - /// for delayed initialization - void printActiveCoroutines(); + /// diagnostic + void printActiveCoroutines(const std::string& when=std::string()); /// get the current coro::id for those who really really care static coro::id get_self(); @@ -176,6 +178,7 @@ public: { set_consuming(consuming); } + OverrideConsuming(const OverrideConsuming&) = delete; ~OverrideConsuming() { set_consuming(mPrevConsuming); @@ -185,6 +188,58 @@ public: bool mPrevConsuming; }; + /// set string coroutine status for diagnostic purposes + static void setStatus(const std::string& status); + static std::string getStatus(); + + /// RAII control of status + class TempStatus + { + public: + TempStatus(const std::string& status): + mOldStatus(getStatus()) + { + setStatus(status); + } + TempStatus(const TempStatus&) = delete; + ~TempStatus() + { + setStatus(mOldStatus); + } + + private: + std::string mOldStatus; + }; + + /// thrown by checkStop() + struct Stop: public LLContinueError + { + Stop(const std::string& what): LLContinueError(what) {} + }; + + /// early stages + struct Stopping: public Stop + { + Stopping(const std::string& what): Stop(what) {} + }; + + /// cleaning up + struct Stopped: public Stop + { + Stopped(const std::string& what): Stop(what) {} + }; + + /// cleaned up -- not much survives! + struct Shutdown: public Stop + { + Shutdown(const std::string& what): Stop(what) {} + }; + + /// Call this intermittently if there's a chance your coroutine might + /// continue running into application shutdown. Throws Stop if LLCoros has + /// been cleaned up. + static void checkStop(); + /** * Aliases for promise and future. An older underlying future implementation * required us to wrap future; that's no longer needed. However -- if it's @@ -204,13 +259,12 @@ public: private: std::string generateDistinctName(const std::string& prefix) const; - void toplevel(const std::string& name, const callable_t& callable); + void toplevel(std::string name, callable_t callable); struct CoroData; #if LL_WINDOWS static void winlevel(const callable_t& callable); #endif - CoroData& get_CoroData(const std::string& caller); - const CoroData& get_CoroData(const std::string& caller) const; + static CoroData& get_CoroData(const std::string& caller); S32 mStackSize; @@ -223,6 +277,8 @@ private: const std::string mName; // set_consuming() state bool mConsuming; + // setStatus() state + std::string mStatus; F64 mCreationTime; // since epoch }; typedef boost::ptr_map CoroMap; diff --git a/indra/llcommon/tests/lleventcoro_test.cpp b/indra/llcommon/tests/lleventcoro_test.cpp index c13920eefd..032923a108 100644 --- a/indra/llcommon/tests/lleventcoro_test.cpp +++ b/indra/llcommon/tests/lleventcoro_test.cpp @@ -40,6 +40,7 @@ #include #include "../test/lltut.h" +#include "../test/lltestapp.h" #include "llsd.h" #include "llsdutil.h" #include "llevents.h" @@ -98,6 +99,7 @@ namespace tut std::string replyName, errorName, threw, stringdata; LLSD result, errordata; int which; + LLTestApp testApp; void explicit_wait(boost::shared_ptr>& cbp); void waitForEventOn1(); diff --git a/indra/test/lltestapp.h b/indra/test/lltestapp.h new file mode 100644 index 0000000000..382516cd2b --- /dev/null +++ b/indra/test/lltestapp.h @@ -0,0 +1,34 @@ +/** + * @file lltestapp.h + * @author Nat Goodspeed + * @date 2019-10-21 + * @brief LLApp subclass useful for testing. + * + * $LicenseInfo:firstyear=2019&license=viewerlgpl$ + * Copyright (c) 2019, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_LLTESTAPP_H) +#define LL_LLTESTAPP_H + +#include "llapp.h" + +/** + * LLTestApp is a dummy LLApp that simply sets LLApp::isRunning() for anyone + * who cares. + */ +class LLTestApp: public LLApp +{ +public: + LLTestApp() + { + setStatus(APP_STATUS_RUNNING); + } + + bool init() { return true; } + bool cleanup() { return true; } + bool frame() { return true; } +}; + +#endif /* ! defined(LL_LLTESTAPP_H) */ diff --git a/indra/viewer_components/login/tests/lllogin_test.cpp b/indra/viewer_components/login/tests/lllogin_test.cpp index 23db8d0fe3..0255e10e53 100644 --- a/indra/viewer_components/login/tests/lllogin_test.cpp +++ b/indra/viewer_components/login/tests/lllogin_test.cpp @@ -42,6 +42,7 @@ // other Linden headers #include "llsd.h" #include "../../../test/lltut.h" +#include "../../../test/lltestapp.h" //#define DEBUG_ON #include "../../../test/debug.h" #include "llevents.h" @@ -201,6 +202,7 @@ namespace tut pumps.clear(); } LLEventPumps& pumps; + LLTestApp testApp; }; typedef test_group llviewerlogin_group; -- cgit v1.3 From 39f4acd92144546d346ecd63224945da8d64c5db Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 15 Nov 2019 08:11:32 -0500 Subject: DRTVWR-476: Conflate LOGFAIL env var empty with completely unset. Sometimes it's useful to be able to temporarily override an existing LOGFAIL setting in the current environment. It's far more convenient to prepend LOGFAIL='' to a command than to 'unset LOGFAIL' as a whole separate command -- and then remember to restore its previous value. --- indra/test/test.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'indra/test') diff --git a/indra/test/test.cpp b/indra/test/test.cpp index 51f0e80043..6b342ffe89 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -631,8 +631,9 @@ int main(int argc, char **argv) const char* LOGFAIL = getenv("LOGFAIL"); boost::shared_ptr replayer; // As described in stream_usage(), LOGFAIL overrides both --debug and - // LOGTEST. - if (LOGFAIL) + // LOGTEST. But allow user to set LOGFAIL empty to revert to LOGTEST + // and/or --debug. + if (LOGFAIL && *LOGFAIL) { LLError::ELevel level = LLError::decodeLevel(LOGFAIL); replayer.reset(new LLReplayLogReal(level, gAPRPoolp)); -- cgit v1.3 From 3cd2beb97ef0d368d47b0b7efd242b3c709d01af Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 9 Dec 2019 14:33:56 -0500 Subject: DRTVWR-476: Make Sync::bump() atomic, add set() method. Using Sync with multiple threads is trickier than with coroutines. In particular, Sync::bump() was racy (get() and set() as two different operations), and threads were proceeding when they should have waited. Fortunately LLCond, on which Sync is based, already supports atomic update operations. Use that for bump(). But to nail things down even more specifically, add set(n) to complement yield_until(n). Using those methods, there should be no ambiguity about which call in one thread synchronizes with which call in the other thread. --- indra/test/sync.h | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 10 deletions(-) (limited to 'indra/test') diff --git a/indra/test/sync.h b/indra/test/sync.h index cafbc034b4..0f0b6cece8 100644 --- a/indra/test/sync.h +++ b/indra/test/sync.h @@ -41,18 +41,49 @@ public: mTimeout(timeout) {} - /// Bump mCond by n steps -- ideally, do this every time a participating - /// coroutine wakes up from any suspension. The choice to bump() after - /// resumption rather than just before suspending is worth calling out: - /// this practice relies on the fact that condition_variable::notify_all() - /// merely marks a suspended coroutine ready to run, rather than - /// immediately resuming it. This way, though, even if a coroutine exits - /// before reaching its next suspend point, the other coroutine isn't - /// left waiting forever. + /** + * Bump mCond by n steps -- ideally, do this every time a participating + * coroutine wakes up from any suspension. The choice to bump() after + * resumption rather than just before suspending is worth calling out: + * this practice relies on the fact that condition_variable::notify_all() + * merely marks a suspended coroutine ready to run, rather than + * immediately resuming it. This way, though, even if a coroutine exits + * before reaching its next suspend point, the other coroutine isn't + * left waiting forever. + */ void bump(int n=1) { - LL_DEBUGS() << llcoro::logname() << " bump(" << n << ") -> " << (mCond.get() + n) << LL_ENDL; - mCond.set_all(mCond.get() + n); + // Calling mCond.set_all(mCond.get() + n) would be great for + // coroutines -- but not so good between kernel threads -- it would be + // racy. Make the increment atomic by calling update_all(), which runs + // the passed lambda within a mutex lock. + int updated; + mCond.update_all( + [&n, &updated](int& data) + { + data += n; + // Capture the new value for possible logging purposes. + updated = data; + }); + // In the multi-threaded case, this log message could be a bit + // misleading, as it will be emitted after waiting threads have + // already awakened. But emitting the log message within the lock + // would seem to hold the lock longer than we really ought. + LL_DEBUGS() << llcoro::logname() << " bump(" << n << ") -> " << updated << LL_ENDL; + } + + /** + * Set mCond to a specific n. Use of bump() and yield() is nicely + * maintainable, since you can insert or delete matching operations in a + * test function and have the rest of the Sync operations continue to + * line up as before. But sometimes you need to get very specific, which + * is where set() and yield_until() come in handy: less maintainable, + * more precise. + */ + void set(int n) + { + LL_DEBUGS() << llcoro::logname() << " set(" << n << ")" << LL_ENDL; + mCond.set_all(n); } /// suspend until "somebody else" has bumped mCond by n steps -- cgit v1.3 From dc07509f296661cf7a4d4bceb88a1a897757de98 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 3 Apr 2020 10:38:53 -0400 Subject: DRTVWR-476: Cherry-pick debug aids from commit 77b0c53 (fiber-mutex) --- indra/llcommon/llerror.h | 15 ++- indra/llcommon/llsingleton.h | 2 + indra/llcommon/tests/lleventdispatcher_test.cpp | 119 +++++++++++++-------- indra/test/chained_callback.h | 105 ++++++++++++++++++ indra/test/debug.h | 40 +++++-- indra/test/print.h | 42 ++++++++ indra/test/test.cpp | 105 +++++++++--------- .../viewer_components/login/tests/lllogin_test.cpp | 1 - 8 files changed, 320 insertions(+), 109 deletions(-) create mode 100644 indra/test/chained_callback.h create mode 100644 indra/test/print.h (limited to 'indra/test') diff --git a/indra/llcommon/llerror.h b/indra/llcommon/llerror.h index 48162eca9e..3cdd051ac7 100644 --- a/indra/llcommon/llerror.h +++ b/indra/llcommon/llerror.h @@ -191,9 +191,9 @@ namespace LLError The classes CallSite and Log are used by the logging macros below. They are not intended for general use. */ - + struct CallSite; - + class LL_COMMON_API Log { public: @@ -202,8 +202,17 @@ namespace LLError static void flush(std::ostringstream* out, char* message); static void flush(std::ostringstream*, const CallSite&); static std::string demangle(const char* mangled); + /// classname() + template + static std::string classname() { return demangle(typeid(T).name()); } + /// classname(some_pointer) + template + static std::string classname(const T* ptr) { return demangle(typeid(*ptr).name()); } + /// classname(some_reference) + template + static std::string classname(const T& obj) { return demangle(typeid(obj).name()); } }; - + struct LL_COMMON_API CallSite { // Represents a specific place in the code where a message is logged diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 27c2ceb3b6..ccd2e48bf2 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -120,6 +120,8 @@ protected: static void logdebugs(const char* p1, const char* p2="", const char* p3="", const char* p4=""); static std::string demangle(const char* mangled); + // these classname() declarations restate template functions declared in + // llerror.h because we avoid #including that here template static std::string classname() { return demangle(typeid(T).name()); } template diff --git a/indra/llcommon/tests/lleventdispatcher_test.cpp b/indra/llcommon/tests/lleventdispatcher_test.cpp index efb75951be..9da1ecfd67 100644 --- a/indra/llcommon/tests/lleventdispatcher_test.cpp +++ b/indra/llcommon/tests/lleventdispatcher_test.cpp @@ -23,6 +23,7 @@ #include "stringize.h" #include "tests/wrapllerrs.h" #include "../test/catch_and_store_what_in.h" +#include "../test/debug.h" #include #include @@ -45,15 +46,6 @@ using boost::lambda::var; using namespace llsd; -/***************************************************************************** -* Output control -*****************************************************************************/ -#ifdef DEBUG_ON -using std::cout; -#else -static std::ostringstream cout; -#endif - /***************************************************************************** * Example data, functions, classes *****************************************************************************/ @@ -155,13 +147,13 @@ struct Vars /*------------- no-args (non-const, const, static) methods -------------*/ void method0() { - cout << "method0()\n"; + debug()("method0()"); i = 17; } void cmethod0() const { - cout << 'c'; + debug()('c', NONL); const_cast(this)->method0(); } @@ -170,13 +162,13 @@ struct Vars /*------------ Callable (non-const, const, static) methods -------------*/ void method1(const LLSD& obj) { - cout << "method1(" << obj << ")\n"; + debug()("method1(", obj, ")"); llsd = obj; } void cmethod1(const LLSD& obj) const { - cout << 'c'; + debug()('c', NONL); const_cast(this)->method1(obj); } @@ -196,12 +188,12 @@ struct Vars else vcp = std::string("'") + cp + "'"; - cout << "methodna(" << b - << ", " << i - << ", " << f - << ", " << d - << ", " << vcp - << ")\n"; + debug()("methodna(", b, + ", ", i, + ", ", f, + ", ", d, + ", ", vcp, + ")"); this->b = b; this->i = i; @@ -218,12 +210,12 @@ struct Vars vbin << std::hex << std::setfill('0') << std::setw(2) << unsigned(byte); } - cout << "methodnb(" << "'" << s << "'" - << ", " << uuid - << ", " << date - << ", '" << uri << "'" - << ", " << vbin.str() - << ")\n"; + debug()("methodnb(", "'", s, "'", + ", ", uuid, + ", ", date, + ", '", uri, "'", + ", ", vbin.str(), + ")"); this->s = s; this->uuid = uuid; @@ -234,18 +226,30 @@ struct Vars void cmethodna(NPARAMSa) const { - cout << 'c'; + debug()('c', NONL); const_cast(this)->methodna(NARGSa); } void cmethodnb(NPARAMSb) const { - cout << 'c'; + debug()('c', NONL); const_cast(this)->methodnb(NARGSb); } static void smethodna(NPARAMSa); static void smethodnb(NPARAMSb); + + static Debug& debug() + { + // Lazily initialize this Debug instance so it can notice if main() + // has forcibly set LOGTEST. If it were simply a static member, it + // would already have examined the environment variable by the time + // main() gets around to checking command-line switches. Since we have + // a global static Vars instance, the same would be true of a plain + // non-static member. + static Debug sDebug("Vars"); + return sDebug; + } }; /*------- Global Vars instance for free functions and static methods -------*/ static Vars g; @@ -253,25 +257,25 @@ static Vars g; /*------------ Static Vars method implementations reference 'g' ------------*/ void Vars::smethod0() { - cout << "smethod0() -> "; + debug()("smethod0() -> ", NONL); g.method0(); } void Vars::smethod1(const LLSD& obj) { - cout << "smethod1(" << obj << ") -> "; + debug()("smethod1(", obj, ") -> ", NONL); g.method1(obj); } void Vars::smethodna(NPARAMSa) { - cout << "smethodna(...) -> "; + debug()("smethodna(...) -> ", NONL); g.methodna(NARGSa); } void Vars::smethodnb(NPARAMSb) { - cout << "smethodnb(...) -> "; + debug()("smethodnb(...) -> ", NONL); g.methodnb(NARGSb); } @@ -284,25 +288,25 @@ void clear() /*------------------- Free functions also reference 'g' --------------------*/ void free0() { - cout << "free0() -> "; + g.debug()("free0() -> ", NONL); g.method0(); } void free1(const LLSD& obj) { - cout << "free1(" << obj << ") -> "; + g.debug()("free1(", obj, ") -> ", NONL); g.method1(obj); } void freena(NPARAMSa) { - cout << "freena(...) -> "; + g.debug()("freena(...) -> ", NONL); g.methodna(NARGSa); } void freenb(NPARAMSb) { - cout << "freenb(...) -> "; + g.debug()("freenb(...) -> ", NONL); g.methodnb(NARGSb); } @@ -313,6 +317,7 @@ namespace tut { struct lleventdispatcher_data { + Debug debug{"test"}; WrapLLErrs redirect; Dispatcher work; Vars v; @@ -431,7 +436,12 @@ namespace tut // Same for freenb() et al. params = LLSDMap("a", LLSDArray("b")("i")("f")("d")("cp")) ("b", LLSDArray("s")("uuid")("date")("uri")("bin")); - cout << "params:\n" << params << "\nparams[\"a\"]:\n" << params["a"] << "\nparams[\"b\"]:\n" << params["b"] << std::endl; + debug("params:\n", + params, "\n" + "params[\"a\"]:\n", + params["a"], "\n" + "params[\"b\"]:\n", + params["b"]); // default LLSD::Binary value std::vector binary; for (size_t ix = 0, h = 0xaa; ix < 6; ++ix, h += 0x11) @@ -448,7 +458,8 @@ namespace tut (LLDate::now()) (LLURI("http://www.ietf.org/rfc/rfc3986.txt")) (binary)); - cout << "dft_array_full:\n" << dft_array_full << std::endl; + debug("dft_array_full:\n", + dft_array_full); // Partial defaults arrays. foreach(LLSD::String a, ab) { @@ -457,7 +468,8 @@ namespace tut llsd_copy_array(dft_array_full[a].beginArray() + partition, dft_array_full[a].endArray()); } - cout << "dft_array_partial:\n" << dft_array_partial << std::endl; + debug("dft_array_partial:\n", + dft_array_partial); foreach(LLSD::String a, ab) { @@ -473,7 +485,10 @@ namespace tut dft_map_partial[a][params[a][ix].asString()] = dft_array_full[a][ix]; } } - cout << "dft_map_full:\n" << dft_map_full << "\ndft_map_partial:\n" << dft_map_partial << '\n'; + debug("dft_map_full:\n", + dft_map_full, "\n" + "dft_map_partial:\n", + dft_map_partial); // (Free function | static method) with (no | arbitrary) params, // map style, no (empty array) defaults @@ -918,7 +933,12 @@ namespace tut params[a].endArray()), dft_array_partial[a]); } - cout << "allreq:\n" << allreq << "\nleftreq:\n" << leftreq << "\nrightdft:\n" << rightdft << std::endl; + debug("allreq:\n", + allreq, "\n" + "leftreq:\n", + leftreq, "\n" + "rightdft:\n", + rightdft); // Generate maps containing parameter names not provided by the // dft_map_partial maps. @@ -930,7 +950,8 @@ namespace tut skipreq[a].erase(me.first); } } - cout << "skipreq:\n" << skipreq << std::endl; + debug("skipreq:\n", + skipreq); LLSD groups(LLSDArray // array of groups @@ -975,7 +996,11 @@ namespace tut LLSD names(grp[0]); LLSD required(grp[1][0]); LLSD optional(grp[1][1]); - cout << "For " << names << ",\n" << "required:\n" << required << "\noptional:\n" << optional << std::endl; + debug("For ", names, ",\n", + "required:\n", + required, "\n" + "optional:\n", + optional); // Loop through 'names' foreach(LLSD nm, inArray(names)) @@ -1163,7 +1188,7 @@ namespace tut } // Adjust expect["a"]["cp"] for special Vars::cp treatment. expect["a"]["cp"] = std::string("'") + expect["a"]["cp"].asString() + "'"; - cout << "expect: " << expect << '\n'; + debug("expect: ", expect); // Use substantially the same logic for args and argsplus LLSD argsarrays(LLSDArray(args)(argsplus)); @@ -1218,7 +1243,8 @@ namespace tut { array_overfull[a].append("bogus"); } - cout << "array_full: " << array_full << "\narray_overfull: " << array_overfull << std::endl; + debug("array_full: ", array_full, "\n" + "array_overfull: ", array_overfull); // We rather hope that LLDate::now() will generate a timestamp // distinct from the one it generated in the constructor, moments ago. ensure_not_equals("Timestamps too close", @@ -1233,7 +1259,8 @@ namespace tut map_overfull[a] = map_full[a]; map_overfull[a]["extra"] = "ignore"; } - cout << "map_full: " << map_full << "\nmap_overfull: " << map_overfull << std::endl; + debug("map_full: ", map_full, "\n" + "map_overfull: ", map_overfull); LLSD expect(map_full); // Twiddle the const char* param. expect["a"]["cp"] = std::string("'") + expect["a"]["cp"].asString() + "'"; @@ -1248,7 +1275,7 @@ namespace tut // so won't bother returning it. Predict that behavior to match the // LLSD values. expect["a"].erase("b"); - cout << "expect: " << expect << std::endl; + debug("expect: ", expect); // For this test, calling functions registered with different sets of // parameter defaults should make NO DIFFERENCE WHATSOEVER. Every call // should pass all params. diff --git a/indra/test/chained_callback.h b/indra/test/chained_callback.h new file mode 100644 index 0000000000..41a7f7c9fa --- /dev/null +++ b/indra/test/chained_callback.h @@ -0,0 +1,105 @@ +/** + * @file chained_callback.h + * @author Nat Goodspeed + * @date 2020-01-03 + * @brief Subclass of tut::callback used for chaining callbacks. + * + * $LicenseInfo:firstyear=2020&license=viewerlgpl$ + * Copyright (c) 2020, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_CHAINED_CALLBACK_H) +#define LL_CHAINED_CALLBACK_H + +/** + * Derive your TUT callback from chained_callback instead of tut::callback to + * ensure that multiple such callbacks can coexist in a given test executable. + * The relevant callback method will be called for each callback instance in + * reverse order of the instance's link() methods being called: the most + * recently link()ed callback will be called first, then the previous, and so + * forth. + * + * Obviously, for this to work, all relevant callbacks must be derived from + * chained_callback instead of tut::callback. Given that, control should reach + * each of them regardless of their construction order. The chain is + * guaranteed to stop because the first link() call will link to test_runner's + * default_callback, which is simply an instance of the callback() base class. + * + * The rule for deriving from chained_callback is that you may override any of + * its virtual methods, but your override must at some point call the + * corresponding chained_callback method. + */ +class chained_callback: public tut::callback +{ +public: + /** + * Instead of calling tut::test_runner::set_callback(&your_callback), call + * your_callback.link(); + * This uses the canonical instance of tut::test_runner. + */ + void link() + { + link(tut::runner.get()); + } + + /** + * If for some reason you have a different instance of test_runner... + */ + void link(tut::test_runner& runner) + { + // Since test_runner's constructor sets a default callback, + // get_callback() will always return a reference to a valid callback + // instance. + mPrev = &runner.get_callback(); + runner.set_callback(this); + } + + /** + * Called when new test run started. + */ + virtual void run_started() + { + mPrev->run_started(); + } + + /** + * Called when a group started + * @param name Name of the group + */ + virtual void group_started(const std::string& name) + { + mPrev->group_started(name); + } + + /** + * Called when a test finished. + * @param tr Test results. + */ + virtual void test_completed(const tut::test_result& tr) + { + mPrev->test_completed(tr); + } + + /** + * Called when a group is completed + * @param name Name of the group + */ + virtual void group_completed(const std::string& name) + { + mPrev->group_completed(name); + } + + /** + * Called when all tests in run completed. + */ + virtual void run_completed() + { + mPrev->run_completed(); + } + +private: + tut::callback* mPrev; +}; + +#endif /* ! defined(LL_CHAINED_CALLBACK_H) */ diff --git a/indra/test/debug.h b/indra/test/debug.h index 33c3ea2d27..76dbb973b2 100644 --- a/indra/test/debug.h +++ b/indra/test/debug.h @@ -29,37 +29,59 @@ #if ! defined(LL_DEBUG_H) #define LL_DEBUG_H -#include +#include "print.h" /***************************************************************************** * Debugging stuff *****************************************************************************/ -// This class is intended to illuminate entry to a given block, exit from the -// same block and checkpoints along the way. It also provides a convenient -// place to turn std::cout output on and off. +/** + * This class is intended to illuminate entry to a given block, exit from the + * same block and checkpoints along the way. It also provides a convenient + * place to turn std::cerr output on and off. + * + * If the environment variable LOGTEST is non-empty, each Debug instance will + * announce its construction and destruction, presumably at entry and exit to + * the block in which it's declared. Moreover, any arguments passed to its + * operator()() will be streamed to std::cerr, prefixed by the block + * description. + * + * The variable LOGTEST is used because that's the environment variable + * checked by test.cpp, our TUT main() program, to turn on LLError logging. It + * is expected that Debug is solely for use in test programs. + */ class Debug { public: Debug(const std::string& block): - mBlock(block) + mBlock(block), + mLOGTEST(getenv("LOGTEST")), + // debug output enabled when LOGTEST is set AND non-empty + mEnabled(mLOGTEST && *mLOGTEST) { (*this)("entry"); } + // non-copyable + Debug(const Debug&) = delete; + ~Debug() { (*this)("exit"); } - void operator()(const std::string& status) + template + void operator()(ARGS&&... args) { -#if defined(DEBUG_ON) - std::cout << mBlock << ' ' << status << std::endl; -#endif + if (mEnabled) + { + print(mBlock, ' ', std::forward(args)...); + } } private: const std::string mBlock; + const char* mLOGTEST; + bool mEnabled; }; // It's often convenient to use the name of the enclosing function as the name diff --git a/indra/test/print.h b/indra/test/print.h new file mode 100644 index 0000000000..08e36caddf --- /dev/null +++ b/indra/test/print.h @@ -0,0 +1,42 @@ +/** + * @file print.h + * @author Nat Goodspeed + * @date 2020-01-02 + * @brief print() function for debugging + * + * $LicenseInfo:firstyear=2020&license=viewerlgpl$ + * Copyright (c) 2020, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_PRINT_H) +#define LL_PRINT_H + +#include + +// print(..., NONL); +// leaves the output dangling, suppressing the normally appended std::endl +struct NONL_t {}; +#define NONL (NONL_t()) + +// normal recursion end +inline +void print() +{ + std::cerr << std::endl; +} + +// print(NONL) is a no-op +inline +void print(NONL_t) +{ +} + +template +void print(T&& first, ARGS&&... rest) +{ + std::cerr << first; + print(std::forward(rest)...); +} + +#endif /* ! defined(LL_PRINT_H) */ diff --git a/indra/test/test.cpp b/indra/test/test.cpp index 6b342ffe89..ea54ba658e 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -37,6 +37,7 @@ #include "linden_common.h" #include "llerrorcontrol.h" #include "lltut.h" +#include "chained_callback.h" #include "stringize.h" #include "namedtempfile.h" #include "lltrace.h" @@ -71,7 +72,6 @@ #include #include #include -#include #include @@ -172,8 +172,10 @@ private: LLError::RecorderPtr mRecorder; }; -class LLTestCallback : public tut::callback +class LLTestCallback : public chained_callback { + typedef chained_callback super; + public: LLTestCallback(bool verbose_mode, std::ostream *stream, boost::shared_ptr replayer) : @@ -184,7 +186,7 @@ public: mSkippedTests(0), // By default, capture a shared_ptr to std::cout, with a no-op "deleter" // so that destroying the shared_ptr makes no attempt to delete std::cout. - mStream(boost::shared_ptr(&std::cout, boost::lambda::_1)), + mStream(boost::shared_ptr(&std::cout, [](std::ostream*){})), mReplayer(replayer) { if (stream) @@ -205,22 +207,25 @@ public: ~LLTestCallback() { - } + } virtual void run_started() { //std::cout << "run_started" << std::endl; LL_INFOS("TestRunner")<<"Test Started"<< LL_ENDL; + super::run_started(); } virtual void group_started(const std::string& name) { LL_INFOS("TestRunner")<<"Unit test group_started name=" << name << LL_ENDL; *mStream << "Unit test group_started name=" << name << std::endl; + super::group_started(name); } virtual void group_completed(const std::string& name) { LL_INFOS("TestRunner")<<"Unit test group_completed name=" << name << LL_ENDL; *mStream << "Unit test group_completed name=" << name << std::endl; + super::group_completed(name); } virtual void test_completed(const tut::test_result& tr) @@ -282,6 +287,7 @@ public: *mStream << std::endl; } LL_INFOS("TestRunner")< replayer; - // As described in stream_usage(), LOGFAIL overrides both --debug and - // LOGTEST. But allow user to set LOGFAIL empty to revert to LOGTEST - // and/or --debug. - if (LOGFAIL && *LOGFAIL) + boost::shared_ptr replayer{boost::make_shared()}; + + // Testing environment variables for both 'set' and 'not empty' allows a + // user to suppress a pre-existing environment variable by forcing empty. + if (LOGTEST && *LOGTEST) { - LLError::ELevel level = LLError::decodeLevel(LOGFAIL); - replayer.reset(new LLReplayLogReal(level, gAPRPoolp)); + LLError::initForApplication(".", ".", true /* log to stderr */); + LLError::setDefaultLevel(LLError::decodeLevel(LOGTEST)); } else { - replayer.reset(new LLReplayLog()); + LLError::initForApplication(".", ".", false /* do not log to stderr */); + LLError::setDefaultLevel(LLError::LEVEL_DEBUG); + if (LOGFAIL && *LOGFAIL) + { + LLError::ELevel level = LLError::decodeLevel(LOGFAIL); + replayer.reset(new LLReplayLogReal(level, gAPRPoolp)); + } } + LLError::setFatalFunction(wouldHaveCrashed); + std::string test_app_name(argv[0]); + std::string test_log = test_app_name + ".log"; + LLFile::remove(test_log); + LLError::logToFile(test_log); + +#ifdef CTYPE_WORKAROUND + ctype_workaround(); +#endif + + if (!sMasterThreadRecorder) + { + sMasterThreadRecorder = new LLTrace::ThreadRecorder(); + LLTrace::set_master_thread_recorder(sMasterThreadRecorder); + } + + // run the tests LLTestCallback* mycallback; if (getenv("TEAMCITY_PROJECT_NAME")) @@ -653,7 +657,8 @@ int main(int argc, char **argv) mycallback = new LLTestCallback(verbose_mode, output.get(), replayer); } - tut::runner.get().set_callback(mycallback); + // a chained_callback subclass must be linked with previous + mycallback->link(); if(test_group.empty()) { diff --git a/indra/viewer_components/login/tests/lllogin_test.cpp b/indra/viewer_components/login/tests/lllogin_test.cpp index 0255e10e53..0d933a3d64 100644 --- a/indra/viewer_components/login/tests/lllogin_test.cpp +++ b/indra/viewer_components/login/tests/lllogin_test.cpp @@ -43,7 +43,6 @@ #include "llsd.h" #include "../../../test/lltut.h" #include "../../../test/lltestapp.h" -//#define DEBUG_ON #include "../../../test/debug.h" #include "llevents.h" #include "lleventcoro.h" -- cgit v1.3 From 962ccb4f01f220850fea35e32c3d92a718d35631 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 2 Apr 2020 13:14:31 -0400 Subject: DRTVWR-476: Facilitate debugging test programs with logging. On Mac, even if you run a test program with --debug or set LOGTEST=DEBUG, it won't log to stderr if you're filtering build output or running the build in an emacs compile buffer. This is because, on Mac, a viewer launched by mouse rather than from the command line is passed a stderr stream that ultimately gets logged to the system Console. The shouldLogToStderr() function is intended to avoid spamming the Console with the (voluminous) viewer log output. It tests whether stderr isatty() and, if not, suppresses calling LLError::logToStderr(). This makes debugging test programs using log output trickier than necessary. Change shouldLogToStderr() to permit logging when either stderr isatty() or is a pipe. The original intention is preserved in that empirically, a viewer launched by mouse is passed a stderr stream identified as a character device rather than as a pipe. Also introduce SetEnv, a class that facilitates setting (e.g.) LOGTEST=DEBUG for specific test programs without setting it for all test programs in the build. Using the constructor for a static object means you can set environment variables before main() is entered, which is important because it's the main() function in test.cpp that acts on the LOGTEST and LOGFAIL environment variables. These changes make it unnecessary to retain the temporary change in test.cpp to force LOGTEST to DEBUG. --- indra/llcommon/llerror.cpp | 43 +++++++++++++++++++++--------- indra/test/setenv.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++ indra/test/test.cpp | 3 --- 3 files changed, 96 insertions(+), 16 deletions(-) create mode 100644 indra/test/setenv.h (limited to 'indra/test') diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp index 1cb93d27f7..0daae96cca 100644 --- a/indra/llcommon/llerror.cpp +++ b/indra/llcommon/llerror.cpp @@ -39,6 +39,7 @@ #if !LL_WINDOWS # include # include +# include #endif // !LL_WINDOWS #include #include "string.h" @@ -654,22 +655,38 @@ namespace LLError namespace { - bool shouldLogToStderr() - { + bool shouldLogToStderr() + { #if LL_DARWIN - // On Mac OS X, stderr from apps launched from the Finder goes to the - // console log. It's generally considered bad form to spam too much - // there. - - // If stderr is a tty, assume the user launched from the command line or - // debugger and therefore wants to see stderr. Otherwise, assume we've - // been launched from the finder and shouldn't spam stderr. - return isatty(STDERR_FILENO); + // On Mac OS X, stderr from apps launched from the Finder goes to the + // console log. It's generally considered bad form to spam too much + // there. That scenario can be detected by noticing that stderr is a + // character device (S_IFCHR). + + // If stderr is a tty or a pipe, assume the user launched from the + // command line or debugger and therefore wants to see stderr. + if (isatty(STDERR_FILENO)) + return true; + // not a tty, but might still be a pipe -- check + struct stat st; + if (fstat(STDERR_FILENO, &st) < 0) + { + // capture errno right away, before engaging any other operations + auto errno_save = errno; + // this gets called during log-system setup -- can't log yet! + std::cerr << "shouldLogToStderr: fstat(" << STDERR_FILENO << ") failed, errno " + << errno_save << std::endl; + // if we can't tell, err on the safe side and don't write stderr + return false; + } + + // fstat() worked: return true only if stderr is a pipe + return ((st.st_mode & S_IFMT) == S_IFIFO); #else - return true; + return true; #endif - } - + } + bool stderrLogWantsTime() { #if LL_WINDOWS diff --git a/indra/test/setenv.h b/indra/test/setenv.h new file mode 100644 index 0000000000..ed2de9ccca --- /dev/null +++ b/indra/test/setenv.h @@ -0,0 +1,66 @@ +/** + * @file setenv.h + * @author Nat Goodspeed + * @date 2020-04-01 + * @brief Provide a way for a particular test program to alter the + * environment before entry to main(). + * + * $LicenseInfo:firstyear=2020&license=viewerlgpl$ + * Copyright (c) 2020, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_SETENV_H) +#define LL_SETENV_H + +#include // setenv() + +/** + * Our test.cpp main program responds to environment variables LOGTEST and + * LOGFAIL. But if you set (e.g.) LOGTEST=DEBUG before a viewer build, @em + * every test program in the build emits debug log output. This can be so + * voluminous as to slow down the build. + * + * With an integration test program, you can specifically build (e.g.) the + * INTEGRATION_TEST_llstring target, and set any environment variables you + * want for that. But with a unit test program, since executing the program is + * a side effect rather than an explicit target, specifically building (e.g.) + * PROJECT_lllogin_TEST_lllogin only builds the executable without running it. + * + * To set an environment variable for a particular test program, declare a + * static instance of SetEnv in its .cpp file. SetEnv's constructor takes + * pairs of strings, e.g. + * + * @code + * static SetEnv sLOGGING("LOGTEST", "INFO"); + * @endcode + * + * Declaring a static instance of SetEnv is important because that ensures + * that the environment variables are set before main() is entered, since it + * is main() that examines LOGTEST and LOGFAIL. + */ +struct SetEnv +{ + // degenerate constructor, terminate recursion + SetEnv() {} + + /** + * SetEnv() accepts an arbitrary number of pairs of strings: variable + * name, value, variable name, value ... Entering the constructor sets + * those variables in the process environment using Posix setenv(), + * overriding any previous value. If static SetEnv declarations in + * different translation units specify overlapping sets of variable names, + * it is indeterminate which instance will "win." + */ + template + SetEnv(VAR&& var, VAL&& val, ARGS&&... rest): + // constructor forwarding handles the tail of the list + SetEnv(std::forward(rest)...) + { + // set just the first (variable, value) pair + // 1 means override previous value if any + setenv(std::forward(var), std::forward(val), 1); + } +}; + +#endif /* ! defined(LL_SETENV_H) */ diff --git a/indra/test/test.cpp b/indra/test/test.cpp index ea54ba658e..87c4a8d8a3 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -544,9 +544,6 @@ int main(int argc, char **argv) // LOGTEST overrides default, but can be overridden by --debug. const char* LOGTEST = getenv("LOGTEST"); - // DELETE - LOGTEST = "DEBUG"; - // values used for options parsing apr_status_t apr_err; const char* opt_arg = NULL; -- cgit v1.3 From 5ced20b1a9b76f361e92088d1464615fba1b1697 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 2 Apr 2020 14:51:48 -0400 Subject: DRTVWR-476: chained_callback.h depends on lltut.h. #include it. --- indra/test/chained_callback.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'indra/test') diff --git a/indra/test/chained_callback.h b/indra/test/chained_callback.h index 41a7f7c9fa..05929e33ad 100644 --- a/indra/test/chained_callback.h +++ b/indra/test/chained_callback.h @@ -12,6 +12,8 @@ #if ! defined(LL_CHAINED_CALLBACK_H) #define LL_CHAINED_CALLBACK_H +#include "lltut.h" + /** * Derive your TUT callback from chained_callback instead of tut::callback to * ensure that multiple such callbacks can coexist in a given test executable. -- cgit v1.3 From d979ba68ee91404d5d3037701e49eb26286109f5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 3 Apr 2020 10:54:37 -0400 Subject: DRTVWR-476: Use a longer default timeout for Sync class. The timeout is meant to prevent a deadlocked test program from hanging a build. It's not intended to ensure some sort of SLA for the operations under test. Empirically, using a longer timeout helps some test programs. The only downside of increasing the timeout is that if some test does hang, it takes longer to notice. But changes on the order of a few seconds are negligible. --- indra/test/sync.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/test') diff --git a/indra/test/sync.h b/indra/test/sync.h index 0f0b6cece8..ca8b7262d6 100644 --- a/indra/test/sync.h +++ b/indra/test/sync.h @@ -37,7 +37,7 @@ class Sync F32Milliseconds mTimeout; public: - Sync(F32Milliseconds timeout=F32Milliseconds(10.0f)): + Sync(F32Milliseconds timeout=F32Milliseconds(10000.0f)): mTimeout(timeout) {} -- cgit v1.3