From a57690123ae3ce5bdd7e970690f1380e88e4eaf6 Mon Sep 17 00:00:00 2001 From: Benoit Germain Date: Mon, 17 Mar 2025 12:34:08 +0100 Subject: Raise a regular Lua error instead of throwing a C++ std::logic_error exception in Universe::UniverseGC --- CHANGES | 2 +- docs/index.html | 6 +- src/lane.cpp | 6 ++ src/lane.hpp | 4 ++ src/universe.cpp | 46 +++++++++--- src/universe.hpp | 3 +- unit_tests/_pch.cpp | 14 ---- unit_tests/lane_tests.cpp | 10 +-- unit_tests/scripts/lane/uncooperative_shutdown.lua | 4 +- unit_tests/shared.cpp | 82 +++++++++++++++------- unit_tests/shared.h | 5 +- 11 files changed, 119 insertions(+), 63 deletions(-) diff --git a/CHANGES b/CHANGES index 713249c..7cf28b2 100644 --- a/CHANGES +++ b/CHANGES @@ -14,8 +14,8 @@ CHANGE 2: BGe 27-Nov-24 - lanes.sleep() accepts a new argument "indefinitely" to block forever (until hard cancellation is received). - function set_debug_threadname() available inside a Lane is renamed lane_threadname(); can now both read and write the name. - new function lanes.finally(). Installs a function that gets called at Lanes shutdown after attempting to terminate all lanes. + If some lanes still run after the finalizer, Universe::__gc with raise an error or freeze, depending on its return value. - new function lanes.collectgarbage(), to force a full GC cycle in the keeper states. - If some lanes still run after the finalizer, Lanes with throw an exception or freeze, depending on its return value. - Configuration settings: - Boolean parameters only accept boolean values. - allocator provider function is called with a string hint to distinguish internal allocations, lane and keeper states. diff --git a/docs/index.html b/docs/index.html index 28acf3b..d0f3940 100644 --- a/docs/index.html +++ b/docs/index.html @@ -70,7 +70,7 @@

- This document was revised on 13-Dec-24, and applies to version 4.0.0. + This document was revised on 17-Mar-25, and applies to version 4.0.0.

@@ -380,6 +380,8 @@ Sets the duration in seconds Lanes will wait for graceful termination of running lanes at application shutdown. Default is 0.25.
Lanes signals all lanes for cancellation with "soft", "hard", and "all" modes, in that order. Each attempt has shutdown_timeout seconds to succeed before the next one.
Then there is a last chance at cleanup with lanes.finally(). If some lanes are still running after that point, shutdown will either freeze or throw. It is YOUR responsibility to cleanup properly after yourself. + IMPORTANT: If there are still running lanes at shutdown, an error is raised, which will be propagated by Lua to the handler installed by lua_setwarnf. If the finalizer returned a value, this will be used as the error message.
+ LANES SHUTDOWN WILL NOT BE COMPLETE IN THAT CASE, AND THE SUBSEQUENT CONSEQUENCES ARE UNDEFINED! @@ -487,7 +489,7 @@ The finalizer is called unprotected from inside __gc metamethod of Lanes's Universe. Therefore, if your finalizer raises an error, Lua rules regarding errors in finalizers apply normally.
The installed function is called after all free-running lanes got a chance to terminate (see shutdown_timeout), but before lindas become unusable.
The finalizer receives a single argument, a bool indicating whether all Lanes are successfully terminated at that point. It is possible to inspect them with tracking.
- If there are still running lanes when the finalizer returns: Lanes will throw a C++ std::logic_error if the finalizer returned "throw". Any other value will cause Lanes to freeze forever. + If there are still running lanes when the finalizer returns: If the finalizer returned "freeze", Lanes will freeze inside the Universe __gc. Any other value will cause Lanes to raise it as an error. If there is no return value, a default message will be used.

diff --git a/src/lane.cpp b/src/lane.cpp index 7a5c257..5cebdfa 100644 --- a/src/lane.cpp +++ b/src/lane.cpp @@ -756,6 +756,12 @@ static void lane_main(Lane* const lane_) } } + if (lane_->flaggedAfterUniverseGC.load(std::memory_order_relaxed)) { + // let's try not to crash if the lane didn't terminate gracefully and the Universe met its end + // there will be leaks, but what else can we do? + return; + } + if (_errorHandlerCount) { lua_remove(_L, 1); // L: retvals|error } diff --git a/src/lane.hpp b/src/lane.hpp index 9b678d6..5fe36b6 100644 --- a/src/lane.hpp +++ b/src/lane.hpp @@ -139,6 +139,10 @@ class Lane final ErrorTraceLevel const errorTraceLevel{ Basic }; + // when Universe is collected, and an uncooperative Lane refuses to terminate, this flag becomes true + // in case of crash, that's the Lane's fault! + std::atomic_bool flaggedAfterUniverseGC{ false }; + [[nodiscard]] static void* operator new(size_t size_, Universe* U_) noexcept { return U_->internalAllocator.alloc(size_); } // can't actually delete the operator because the compiler generates stack unwinding code that could call it in case of exception diff --git a/src/universe.cpp b/src/universe.cpp index 3da0801..dd7bc4b 100644 --- a/src/universe.cpp +++ b/src/universe.cpp @@ -209,6 +209,17 @@ static int luaG_provide_protected_allocator(lua_State* const L_) // ################################################################################################# +void Universe::flagDanglingLanes() const +{ + std::lock_guard _guard{ selfdestructMutex }; + Lane* _lane{ selfdestructFirst }; + while (_lane != SELFDESTRUCT_END) { + _lane->flaggedAfterUniverseGC.store(true, std::memory_order_relaxed); + _lane = _lane->selfdestruct_next; + } +} +// ################################################################################################# + // called once at the creation of the universe (therefore L_ is the master Lua state everything originates from) // Do I need to disable this when compiling for LuaJIT to prevent issues? void Universe::initializeAllocatorFunction(lua_State* const L_) @@ -399,6 +410,7 @@ bool Universe::terminateFreeRunningLanes(lua_Duration const shutdownTimeout_, Ca // ################################################################################################# // process end: cancel any still free-running threads +// as far as I can tell, this can only by called only from lua_close() int Universe::UniverseGC(lua_State* const L_) { lua_Duration const _shutdown_timeout{ lua_tonumber(L_, lua_upvalueindex(1)) }; @@ -416,23 +428,37 @@ int Universe::UniverseGC(lua_State* const L_) kFinalizerRegKey.pushValue(L_); // L_: U finalizer|nil if (!lua_isnil(L_, -1)) { lua_pushboolean(L_, _allLanesTerminated); // L_: U finalizer bool - // no protection. Lua rules for errors in finalizers apply normally - lua_call(L_, 1, 1); // L_: U ret|error + // no protection. Lua rules for errors in finalizers apply normally: + // Lua 5.4: error is propagated in the warn system + // older: error is swallowed + lua_call(L_, 1, 1); // L_: U msg? + // phew, no error in finalizer, since we reached that point } - STACK_CHECK(L_, 2); - // if some lanes are still running here, we have no other choice than crashing or freezing and let the client figure out what's wrong - bool const _throw{ luaG_tostring(L_, kIdxTop) == "throw" }; - lua_pop(L_, 1); // L_: U + if (lua_isnil(L_, kIdxTop)) { + lua_pop(L_, 1); // L_: U + // no finalizer, or it returned no value: push some default message on the stack, in case it is necessary + luaG_pushstring(L_, "uncooperative lanes detected at shutdown"); // L_: U "msg" + } + STACK_CHECK(L_, 2); - while (_U->selfdestructFirst != SELFDESTRUCT_END) { - if (_throw) { - throw std::logic_error{ "Some lanes are still running at shutdown" }; + // now, all remaining lanes are flagged. if they crash because we remove keepers and the Universe from under them, it is their fault + bool const _detectedUncooperativeLanes{ _U->selfdestructFirst != SELFDESTRUCT_END }; + if (_detectedUncooperativeLanes) { + _U->flagDanglingLanes(); + if (luaG_tostring(L_, kIdxTop) == "freeze") { + std::this_thread::sleep_until(std::chrono::time_point::max()); } else { - std::this_thread::yield(); + // take the value returned by the finalizer (or our default message) and throw it as an error + // since we are inside Lua's GCTM, it will be propagated through the warning system (Lua 5.4) or swallowed silently + raise_lua_error(L_); } } + // --------------------------------------------------------- + // we don't reach that point if some lanes are still running + // --------------------------------------------------------- + // no need to mutex-protect this as all lanes in the universe are gone at that point Linda::DeleteTimerLinda(L_, std::exchange(_U->timerLinda, nullptr), PK); diff --git a/src/universe.hpp b/src/universe.hpp index ab06907..2a3085d 100644 --- a/src/universe.hpp +++ b/src/universe.hpp @@ -106,7 +106,7 @@ class Universe final LaneTracker tracker; // Protects modifying the selfdestruct chain - std::mutex selfdestructMutex; + mutable std::mutex selfdestructMutex; // require() serialization std::recursive_mutex requireMutex; @@ -126,6 +126,7 @@ class Universe final private: static int UniverseGC(lua_State* L_); + void flagDanglingLanes() const; public: [[nodiscard]] diff --git a/unit_tests/_pch.cpp b/unit_tests/_pch.cpp index 189089a..0d37ba4 100644 --- a/unit_tests/_pch.cpp +++ b/unit_tests/_pch.cpp @@ -1,15 +1 @@ #include "_pch.hpp" - -// IMPORTANT INFORMATION: some relative paths coded in the test implementations suppose that the cwd when debugging is $(SolutionDir)Lanes -// Therefore that's what needs to be set in Google Test Adapter 'Working Directory' global setting... - -//int main(int argc, char* argv[]) -//{ -// // your setup ... -// -// int result = Catch::Session().run(argc, argv); -// -// // your clean-up... -// -// return result; -//} \ No newline at end of file diff --git a/unit_tests/lane_tests.cpp b/unit_tests/lane_tests.cpp index 0c4feba..d6ef2e0 100644 --- a/unit_tests/lane_tests.cpp +++ b/unit_tests/lane_tests.cpp @@ -327,10 +327,10 @@ TEST_CASE("scripted tests." #DIR "." #FILE) \ } MAKE_TEST_CASE(lane, cooperative_shutdown, AssertNoLuaError) -#if LUAJIT_FLAVOR() == 0 -// TODO: for some reason, even though we throw as expected, the test fails with LuaJIT. To be investigated -MAKE_TEST_CASE(lane, uncooperative_shutdown, AssertThrows) -#endif // LUAJIT_FLAVOR() +#if LUA_VERSION_NUM >= 504 // // warnings are a Lua 5.4 feature +// NOTE: when this test ends, there are resource leaks and a dangling thread +MAKE_TEST_CASE(lane, uncooperative_shutdown, AssertWarns) +#endif // LUA_VERSION_NUM MAKE_TEST_CASE(lane, tasking_basic, AssertNoLuaError) MAKE_TEST_CASE(lane, tasking_cancelling, AssertNoLuaError) MAKE_TEST_CASE(lane, tasking_comms_criss_cross, AssertNoLuaError) @@ -350,7 +350,7 @@ TEST_CASE("lanes.scripted tests") { auto const& _testParam = GENERATE( FileRunnerParam{ PUC_LUA_ONLY("lane/cooperative_shutdown"), TestType::AssertNoLuaError }, // 0 - FileRunnerParam{ "lane/uncooperative_shutdown", TestType::AssertThrows }, + FileRunnerParam{ "lane/uncooperative_shutdown", TestType::AssertWarns }, FileRunnerParam{ "lane/tasking_basic", TestType::AssertNoLuaError }, // 2 FileRunnerParam{ "lane/tasking_cancelling", TestType::AssertNoLuaError }, // 3 FileRunnerParam{ "lane/tasking_comms_criss_cross", TestType::AssertNoLuaError }, // 4 diff --git a/unit_tests/scripts/lane/uncooperative_shutdown.lua b/unit_tests/scripts/lane/uncooperative_shutdown.lua index 51e5762..89e1ff8 100644 --- a/unit_tests/scripts/lane/uncooperative_shutdown.lua +++ b/unit_tests/scripts/lane/uncooperative_shutdown.lua @@ -8,7 +8,7 @@ local lanes = require "lanes".configure{shutdown_timeout = 0.001, on_state_creat -- launch lanes that blocks forever local lane = function() local fixture = require "fixture" - fixture.forever() + fixture.sleep_for() end -- the generator @@ -20,5 +20,5 @@ local h1 = g1() -- wait until the lane is running repeat until h1.status == "running" --- let the script end, Lanes should throw std::logic_error because the lane did not gracefully terminate +-- this finalizer returns an error string that telling Universe::__gc will use to raise an error when it detects the uncooperative lane lanes.finally(fixture.throwing_finalizer) diff --git a/unit_tests/shared.cpp b/unit_tests/shared.cpp index d139579..2e2af73 100644 --- a/unit_tests/shared.cpp +++ b/unit_tests/shared.cpp @@ -25,35 +25,19 @@ namespace STACK_CHECK(L_, 0); } - static std::map sFinalizerHits; static std::mutex sCallCountsLock; // a finalizer that we can detect even after closing the state - lua_CFunction sThrowingFinalizer = +[](lua_State* L_) { + lua_CFunction sFreezingFinalizer = +[](lua_State* const L_) { std::lock_guard _guard{ sCallCountsLock }; sFinalizerHits[L_].test_and_set(); - luaG_pushstring(L_, "throw"); + luaG_pushstring(L_, "freeze"); // just freeze the thread in place so that it can be debugged return 1; }; - // a finalizer that we can detect even after closing the state - lua_CFunction sYieldingFinalizer = +[](lua_State* L_) { - std::lock_guard _guard{ sCallCountsLock }; - sFinalizerHits[L_].test_and_set(); - return 0; - }; - - // a function that runs forever - lua_CFunction sForever = +[](lua_State* L_) { - while (true) { - std::this_thread::yield(); - } - return 0; - }; - // a function that returns immediately (so that LuaJIT issues a function call for it) - lua_CFunction sGiveMeBack = +[](lua_State* L_) { + lua_CFunction sGiveMeBack = +[](lua_State* const L_) { return lua_gettop(L_); }; @@ -74,14 +58,42 @@ namespace return 0; }; + // a function that sleeps for the specified duration (in seconds) + lua_CFunction sSleepFor = +[](lua_State* const L_) { + std::chrono::time_point _until{ std::chrono::time_point::max() }; + lua_settop(L_, 1); + if (luaG_type(L_, kIdxTop) == LuaType::NUMBER) { // we don't want to use lua_isnumber() because of autocoercion + lua_Duration const _duration{ lua_tonumber(L_, kIdxTop) }; + if (_duration.count() >= 0.0) { + _until = std::chrono::steady_clock::now() + std::chrono::duration_cast(_duration); + } else { + raise_luaL_argerror(L_, kIdxTop, "duration cannot be < 0"); + } + + } else if (!lua_isnoneornil(L_, 2)) { + raise_luaL_argerror(L_, StackIndex{ 2 }, "incorrect duration type"); + } + std::this_thread::sleep_until(_until); + return 0; + }; + + // a finalizer that we can detect even after closing the state + lua_CFunction sThrowingFinalizer = +[](lua_State* const L_) { + std::lock_guard _guard{ sCallCountsLock }; + sFinalizerHits[L_].test_and_set(); + bool const _allLanesTerminated = lua_toboolean(L_, kIdxTop); + luaG_pushstring(L_, "Finalizer%s", _allLanesTerminated ? "" : ": Uncooperative lanes detected"); + return 1; + }; + static luaL_Reg const sFixture[] = { - { "forever", sForever }, + { "freezing_finalizer", sFreezingFinalizer }, { "give_me_back()", sGiveMeBack }, { "newlightuserdata", sNewLightUserData }, { "newuserdata", sNewUserData }, { "on_state_create", sOnStateCreate }, + { "sleep_for", sSleepFor }, { "throwing_finalizer", sThrowingFinalizer }, - { "yielding_finalizer", sYieldingFinalizer }, { nullptr, nullptr } }; } // namespace local @@ -448,16 +460,34 @@ FileRunner::FileRunner(std::string_view const& where_) void FileRunner::performTest(FileRunnerParam const& testParam_) { + static constexpr auto _atPanic = [](lua_State* const L_) { + throw std::logic_error("panic!"); + return 0; + }; + +#if LUA_VERSION_NUM >= 504 // // warnings are a Lua 5.4 feature + std::string _warnMessage; + static constexpr auto _onWarn = [](void* const opaque_, char const* const msg_, int const tocont_) { + std::string& _warnMessage = *static_cast(opaque_); + _warnMessage += msg_; + }; +#endif // LUA_VERSION_NUM + INFO(testParam_.script); switch (testParam_.test) { case TestType::AssertNoLuaError: requireSuccess(root, testParam_.script); break; - case TestType::AssertNoThrow: - REQUIRE_NOTHROW((std::ignore = doFile(root, testParam_.script), close())); - break; - case TestType::AssertThrows: - REQUIRE_THROWS_AS((std::ignore = doFile(root, testParam_.script), close()), std::logic_error); + +#if LUA_VERSION_NUM >= 504 // // warnings are a Lua 5.4 feature + case TestType::AssertWarns: + lua_atpanic(L, _atPanic); + lua_setwarnf(L, _onWarn, &_warnMessage); + std::ignore = doFile(root, testParam_.script); + close(); + WARN(_warnMessage); + REQUIRE(_warnMessage != std::string_view{}); break; +#endif // LUA_VERSION_NUM } } diff --git a/unit_tests/shared.h b/unit_tests/shared.h index 8a84a94..b884df0 100644 --- a/unit_tests/shared.h +++ b/unit_tests/shared.h @@ -66,8 +66,9 @@ class LuaState enum class [[nodiscard]] TestType { AssertNoLuaError, - AssertNoThrow, - AssertThrows, +#if LUA_VERSION_NUM >= 504 // warnings are a Lua 5.4 feature + AssertWarns, +#endif // LUA_VERSION_NUM }; struct FileRunnerParam -- cgit v1.2.3-55-g6feb