Merge branch 'async_crashes_48' into 'openmw-48'

Pull fixes for async Lua into 0.48

See merge request OpenMW/openmw!2482

apps/openmw/mwlua/asyncbindings.cpp

@@ -24,38 +24,34 @@ namespace MWLua
     {
         using TimerType = LuaUtil::ScriptsContainer::TimerType;
         sol::usertype<AsyncPackageId> api = context.mLua->sol().new_usertype<AsyncPackageId>("AsyncPackage");
-        api["registerTimerCallback"] = [](const AsyncPackageId& asyncId, std::string_view name, sol::function callback)
+        api["registerTimerCallback"]
-        {
+            = [](const AsyncPackageId& asyncId, std::string_view name, sol::main_protected_function callback) {
-            asyncId.mContainer->registerTimerCallback(asyncId.mScriptId, name, std::move(callback));
+                  asyncId.mContainer->registerTimerCallback(asyncId.mScriptId, name, std::move(callback));
-            return TimerCallback{asyncId, std::string(name)};
+                  return TimerCallback{ asyncId, std::string(name) };
+              };
+        api["newSimulationTimer"] = [world = context.mWorldView](const AsyncPackageId&, double delay,
+                                        const TimerCallback& callback, sol::main_object callbackArg) {
+            callback.mAsyncId.mContainer->setupSerializableTimer(TimerType::SIMULATION_TIME,
+                world->getSimulationTime() + delay, callback.mAsyncId.mScriptId, callback.mName,
+                std::move(callbackArg));
         };
-        api["newSimulationTimer"] = [world=context.mWorldView](const AsyncPackageId&, double delay,
+        api["newGameTimer"] = [world = context.mWorldView](const AsyncPackageId&, double delay,
-                                                               const TimerCallback& callback, sol::object callbackArg)
+                                  const TimerCallback& callback, sol::main_object callbackArg) {
-        {
+            callback.mAsyncId.mContainer->setupSerializableTimer(TimerType::GAME_TIME, world->getGameTime() + delay,
-            callback.mAsyncId.mContainer->setupSerializableTimer(
-                TimerType::SIMULATION_TIME, world->getSimulationTime() + delay,
                 callback.mAsyncId.mScriptId, callback.mName, std::move(callbackArg));
         };
-        api["newGameTimer"] = [world=context.mWorldView](const AsyncPackageId&, double delay,
+        api["newUnsavableSimulationTimer"] = [world = context.mWorldView](const AsyncPackageId& asyncId, double delay,
-                                                         const TimerCallback& callback, sol::object callbackArg)
+                                                 sol::main_protected_function callback) {
-        {
-            callback.mAsyncId.mContainer->setupSerializableTimer(
-                TimerType::GAME_TIME, world->getGameTime() + delay,
-                callback.mAsyncId.mScriptId, callback.mName, std::move(callbackArg));
-        };
-        api["newUnsavableSimulationTimer"] = [world=context.mWorldView](const AsyncPackageId& asyncId, double delay, sol::function callback)
-        {
             asyncId.mContainer->setupUnsavableTimer(
                 TimerType::SIMULATION_TIME, world->getSimulationTime() + delay, asyncId.mScriptId, std::move(callback));
         };
-        api["newUnsavableGameTimer"] = [world=context.mWorldView](const AsyncPackageId& asyncId, double delay, sol::function callback)
+        api["newUnsavableGameTimer"] = [world = context.mWorldView](const AsyncPackageId& asyncId, double delay,
-        {
+                                           sol::main_protected_function callback) {
             asyncId.mContainer->setupUnsavableTimer(
                 TimerType::GAME_TIME, world->getGameTime() + delay, asyncId.mScriptId, std::move(callback));
         };
-        api["callback"] = [](const AsyncPackageId& asyncId, sol::function fn) -> LuaUtil::Callback
+        api["callback"] = [](const AsyncPackageId& asyncId, sol::main_protected_function fn) -> LuaUtil::Callback {
-        {
+            return LuaUtil::Callback{ std::move(fn), asyncId.mHiddenData };
-            return LuaUtil::Callback{std::move(fn), asyncId.mHiddenData};
         };
 
         sol::usertype<LuaUtil::Callback> callbackType = context.mLua->sol().new_usertype<LuaUtil::Callback>("Callback");

apps/openmw/mwlua/luamanagerimp.cpp

@@ -180,7 +180,7 @@ namespace MWLua
 
         // Run queued callbacks
         for (CallbackWithData& c : mQueuedCallbacks)
-            c.mCallback.call(c.mArg);
+            c.mCallback.tryCall(c.mArg);
         mQueuedCallbacks.clear();
 
         // Engine handlers in local scripts

apps/openmw/mwlua/luamanagerimp.hpp

@@ -108,7 +108,7 @@ namespace MWLua
         void handleConsoleCommand(const std::string& consoleMode, const std::string& command, const MWWorld::Ptr& selectedPtr) override;
 
         // Used to call Lua callbacks from C++
-        void queueCallback(LuaUtil::Callback callback, sol::object arg)
+        void queueCallback(LuaUtil::Callback callback, sol::main_object arg)
         {
             mQueuedCallbacks.push_back({std::move(callback), std::move(arg)});
         }
@@ -119,7 +119,8 @@ namespace MWLua
         template <class Arg>
         std::function<void(Arg)> wrapLuaCallback(const LuaUtil::Callback& c)
         {
-            return [this, c](Arg arg) { this->queueCallback(c, sol::make_object(c.mFunc.lua_state(), arg)); };
+            return
+                [this, c](Arg arg) { this->queueCallback(c, sol::main_object(this->mLua.sol(), sol::in_place, arg)); };
         }
 
         LuaUi::ResourceManager* uiResourceManager() { return &mUiResourceManager; }
@@ -173,7 +174,7 @@ namespace MWLua
         struct CallbackWithData
         {
             LuaUtil::Callback mCallback;
-            sol::object mArg;
+            sol::main_object mArg;
         };
         std::vector<CallbackWithData> mQueuedCallbacks;
 

apps/openmw/mwlua/nearbybindings.cpp

@@ -109,14 +109,12 @@ namespace MWLua
             MWBase::Environment::get().getWorld()->castRenderingRay(res, from, to, false, false);
             return res;
         };
-        api["asyncCastRenderingRay"] =
+        api["asyncCastRenderingRay"] = [context](const LuaUtil::Callback& callback, const osg::Vec3f& from,
-            [manager=context.mLuaManager](const LuaUtil::Callback& callback, const osg::Vec3f& from, const osg::Vec3f& to)
+                                           const osg::Vec3f& to) {
-        {
+            context.mLuaManager->addAction([context, callback, from, to] {
-            manager->addAction([manager, callback, from, to]
-            {
                 MWPhysics::RayCastingResult res;
                 MWBase::Environment::get().getWorld()->castRenderingRay(res, from, to, false, false);
-                manager->queueCallback(callback, sol::make_object(callback.mFunc.lua_state(), res));
+                context.mLuaManager->queueCallback(callback, sol::main_object(context.mLua->sol(), sol::in_place, res));
             });
         };
 

apps/openmw_test_suite/CMakeLists.txt

@@ -27,6 +27,7 @@ file(GLOB UNITTEST_SRC_FILES
     lua/test_configuration.cpp
     lua/test_l10n.cpp
     lua/test_storage.cpp
+    lua/test_async.cpp
 
     lua/test_ui_content.cpp
 

apps/openmw_test_suite/lua/test_async.cpp

@@ -0,0 +1,55 @@
+#include "gmock/gmock.h"
+#include <gtest/gtest.h>
+
+#include <components/lua/luastate.hpp>
+#include <components/lua/scriptscontainer.hpp>
+
+#include "../testing_util.hpp"
+
+namespace
+{
+    using namespace testing;
+    using namespace TestingOpenMW;
+
+    struct LuaCoroutineCallbackTest : Test
+    {
+        void SetUp() override
+        {
+            mLua.open_libraries(sol::lib::coroutine);
+            mLua["callback"] = [&](sol::protected_function fn) -> LuaUtil::Callback {
+                sol::table hiddenData(mLua, sol::create);
+                hiddenData[LuaUtil::ScriptsContainer::sScriptIdKey] = sol::table(mLua, sol::create);
+                return LuaUtil::Callback{ std::move(fn), hiddenData };
+            };
+            mLua["pass"] = [this](LuaUtil::Callback callback) { mCb = callback; };
+        }
+
+        sol::state mLua;
+        LuaUtil::Callback mCb;
+    };
+
+    TEST_F(LuaCoroutineCallbackTest, CoroutineCallbacks)
+    {
+        internal::CaptureStdout();
+        mLua.safe_script(R"X(
+            local s = 'test'
+            coroutine.wrap(function()
+                pass(callback(function(v) print(s) end))
+            end)()
+        )X");
+        mLua.collect_garbage();
+        mCb.call();
+        EXPECT_THAT(internal::GetCapturedStdout(), "test\n");
+    }
+
+    TEST_F(LuaCoroutineCallbackTest, ErrorInCoroutineCallbacks)
+    {
+        mLua.safe_script(R"X(
+            coroutine.wrap(function()
+                pass(callback(function() error('COROUTINE CALLBACK') end))
+            end)()
+        )X");
+        mLua.collect_garbage();
+        EXPECT_ERROR(mCb.call(), "COROUTINE CALLBACK");
+    }
+}

components/lua/scriptscontainer.cpp

@@ -409,7 +409,8 @@ namespace LuaUtil
 
                 try
                 {
-                    timer.mArg = deserialize(mLua.sol(), savedTimer.mCallbackArgument, mSavedDataDeserializer);
+                    timer.mArg = sol::main_object(
+                        deserialize(mLua.sol(), savedTimer.mCallbackArgument, mSavedDataDeserializer));
                     // It is important if the order of content files was changed. The deserialize-serialize procedure
                     // updates refnums, so timer.mSerializedArg may be not equal to savedTimer.mCallbackArgument.
                     timer.mSerializedArg = serialize(timer.mArg, mSerializer);
@@ -456,7 +457,8 @@ namespace LuaUtil
         return it->second;
     }
 
-    void ScriptsContainer::registerTimerCallback(int scriptId, std::string_view callbackName, sol::function callback)
+    void ScriptsContainer::registerTimerCallback(
+        int scriptId, std::string_view callbackName, sol::main_protected_function callback)
     {
         getScript(scriptId).mRegisteredCallbacks.emplace(std::string(callbackName), std::move(callback));
     }
@@ -467,8 +469,8 @@ namespace LuaUtil
         std::push_heap(timerQueue.begin(), timerQueue.end());
     }
 
-    void ScriptsContainer::setupSerializableTimer(TimerType type, double time, int scriptId,
+    void ScriptsContainer::setupSerializableTimer(
-                                                  std::string_view callbackName, sol::object callbackArg)
+        TimerType type, double time, int scriptId, std::string_view callbackName, sol::main_object callbackArg)
     {
         Timer t;
         t.mCallback = std::string(callbackName);
@@ -480,7 +482,8 @@ namespace LuaUtil
         insertTimer(type == TimerType::GAME_TIME ? mGameTimersQueue : mSimulationTimersQueue, std::move(t));
     }
 
-    void ScriptsContainer::setupUnsavableTimer(TimerType type, double time, int scriptId, sol::function callback)
+    void ScriptsContainer::setupUnsavableTimer(
+        TimerType type, double time, int scriptId, sol::main_protected_function callback)
     {
         Timer t;
         t.mScriptId = scriptId;

components/lua/scriptscontainer.hpp

@@ -134,20 +134,20 @@ namespace LuaUtil
 
         // Callbacks for serializable timers should be registered in advance.
         // The script with the given path should already present in the container.
-        void registerTimerCallback(int scriptId, std::string_view callbackName, sol::function callback);
+        void registerTimerCallback(int scriptId, std::string_view callbackName, sol::main_protected_function callback);
 
         // Sets up a timer, that can be automatically saved and loaded.
         //   type - the type of timer, either SIMULATION_TIME or GAME_TIME.
         //   time - the absolute game time (in seconds or in hours) when the timer should be executed.
-        //   scriptPath - script path in VFS is used as script id. The script with the given path should already present in the container.
+        //   scriptPath - script path in VFS is used as script id. The script with the given path should already present
-        //   callbackName - callback (should be registered in advance) for this timer.
+        //   in the container. callbackName - callback (should be registered in advance) for this timer. callbackArg -
-        //   callbackArg - parameter for the callback (should be serializable).
+        //   parameter for the callback (should be serializable).
-        void setupSerializableTimer(TimerType type, double time, int scriptId,
+        void setupSerializableTimer(
-                                    std::string_view callbackName, sol::object callbackArg);
+            TimerType type, double time, int scriptId, std::string_view callbackName, sol::main_object callbackArg);
 
         // Creates a timer. `callback` is an arbitrary Lua function. These timers are called "unsavable"
         // because they can not be stored in saves. I.e. loading a saved game will not fully restore the state.
-        void setupUnsavableTimer(TimerType type, double time, int scriptId, sol::function callback);
+        void setupUnsavableTimer(TimerType type, double time, int scriptId, sol::main_protected_function callback);
 
     protected:
         struct Handler
@@ -195,8 +195,8 @@ namespace LuaUtil
             std::optional<sol::table> mInterface;
             std::string mInterfaceName;
             sol::table mHiddenData;
-            std::map<std::string, sol::function> mRegisteredCallbacks;
+            std::map<std::string, sol::main_protected_function> mRegisteredCallbacks;
-            std::map<int64_t, sol::function> mTemporaryCallbacks;
+            std::map<int64_t, sol::main_protected_function> mTemporaryCallbacks;
             std::string mPath;
         };
         struct Timer
@@ -204,8 +204,8 @@ namespace LuaUtil
             double mTime;
             bool mSerializable;
             int mScriptId;
-            std::variant<std::string, int64_t> mCallback;  // string if serializable, integer otherwise
+            std::variant<std::string, int64_t> mCallback; // string if serializable, integer otherwise
-            sol::object mArg;
+            sol::main_object mArg;
             std::string mSerializedArg;
 
             bool operator<(const Timer& t) const { return mTime > t.mTime; }
@@ -251,8 +251,8 @@ namespace LuaUtil
     // Needed to prevent callback calls if the script was removed.
     struct Callback
     {
-        sol::function mFunc;
+        sol::main_protected_function mFunc;
-        sol::table mHiddenData;  // same object as Script::mHiddenData in ScriptsContainer
+        sol::table mHiddenData; // same object as Script::mHiddenData in ScriptsContainer
 
         bool isValid() const { return mHiddenData[ScriptsContainer::sScriptIdKey] != sol::nil; }