Raise a regular Lua error instead of throwing a C++ std::logic_error exception in Universe::UniverseGC

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 @@
                         </p>
 
                         <p>
-                                This document was revised on 13-Dec-24, and applies to version <tt>4.0.0</tt>.
+                                This document was revised on 17-Mar-25, and applies to version <tt>4.0.0</tt>.
                         </p>
                 </font>
         </center>
@@ -380,6 +380,8 @@
                                 Sets the duration in seconds Lanes will wait for graceful termination of running lanes at application shutdown. Default is <tt>0.25</tt>.<br />
                                 Lanes signals all lanes for cancellation with <tt>"soft"</tt>, <tt>"hard"</tt>, and <tt>"all"</tt> modes, in that order. Each attempt has <tt>shutdown_timeout</tt> seconds to succeed before the next one.<br />
                                 Then there is a last chance at cleanup with <a href="#finally"><tt>lanes.finally()</tt></a>. 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 <tt>lua_setwarnf</tt>. If the finalizer returned a value, this will be used as the error message.<br />
+                                LANES SHUTDOWN WILL NOT BE COMPLETE IN THAT CASE, AND THE SUBSEQUENT CONSEQUENCES ARE UNDEFINED!
                         </td>
                 </tr>
 
@@ -487,7 +489,7 @@
         The finalizer is called unprotected from inside <tt>__gc</tt> metamethod of Lanes's Universe. Therefore, if your finalizer raises an error, Lua rules regarding errors in finalizers apply normally.<br />
         The installed function is called after all free-running lanes got a chance to terminate (see <a href="#shutdown_timeout"><tt>shutdown_timeout</tt></a>), but before lindas become unusable.<br />
         The finalizer receives a single argument, a <tt>bool</tt> indicating whether all Lanes are successfully terminated at that point. It is possible to inspect them with <a href="#tracking">tracking</a>.<br />
-        If there are still running lanes when the finalizer returns: Lanes will throw a C++ <tt>std::logic_error</tt> if the finalizer returned <tt>"throw"</tt>. Any other value will cause Lanes to freeze forever.
+        If there are still running lanes when the finalizer returns: If the finalizer returned <tt>"freeze"</tt>, Lanes will freeze inside the Universe <tt>__gc</tt>. Any other value will cause Lanes to raise it as an error. If there is no return value, a default message will be used.
 </p>
 
 <hr/>
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<std::mutex> _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<std::chrono::steady_clock>::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<lua_State*, std::atomic_flag> 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<std::chrono::steady_clock> _until{ std::chrono::time_point<std::chrono::steady_clock>::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<std::chrono::steady_clock::duration>(_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<std::string*>(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