From 4f25796029d1b630f89aec4cfb72b37f66469269 Mon Sep 17 00:00:00 2001 From: uramer Date: Sun, 2 Oct 2022 12:47:33 +0200 Subject: [PATCH 1/3] Execute async callbacks on the main Lua stack --- apps/openmw/mwlua/asyncbindings.cpp | 42 +++++++++++++--------------- apps/openmw/mwlua/luamanagerimp.hpp | 7 +++-- apps/openmw/mwlua/nearbybindings.cpp | 10 +++---- components/lua/scriptscontainer.cpp | 13 +++++---- components/lua/scriptscontainer.hpp | 26 ++++++++--------- 5 files changed, 48 insertions(+), 50 deletions(-) diff --git a/apps/openmw/mwlua/asyncbindings.cpp b/apps/openmw/mwlua/asyncbindings.cpp index d70e9bfea5..968920ab7f 100644 --- a/apps/openmw/mwlua/asyncbindings.cpp +++ b/apps/openmw/mwlua/asyncbindings.cpp @@ -24,38 +24,34 @@ namespace MWLua { using TimerType = LuaUtil::ScriptsContainer::TimerType; sol::usertype api = context.mLua->sol().new_usertype("AsyncPackage"); - api["registerTimerCallback"] = [](const AsyncPackageId& asyncId, std::string_view name, sol::function callback) - { - asyncId.mContainer->registerTimerCallback(asyncId.mScriptId, name, std::move(callback)); - return TimerCallback{asyncId, std::string(name)}; + api["registerTimerCallback"] + = [](const AsyncPackageId& asyncId, std::string_view name, sol::main_protected_function callback) { + asyncId.mContainer->registerTimerCallback(asyncId.mScriptId, name, std::move(callback)); + 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, - const TimerCallback& callback, sol::object callbackArg) - { - callback.mAsyncId.mContainer->setupSerializableTimer( - TimerType::SIMULATION_TIME, world->getSimulationTime() + delay, + api["newGameTimer"] = [world = context.mWorldView](const AsyncPackageId&, double delay, + const TimerCallback& callback, sol::main_object callbackArg) { + callback.mAsyncId.mContainer->setupSerializableTimer(TimerType::GAME_TIME, world->getGameTime() + delay, callback.mAsyncId.mScriptId, callback.mName, std::move(callbackArg)); }; - api["newGameTimer"] = [world=context.mWorldView](const AsyncPackageId&, double delay, - const TimerCallback& callback, sol::object callbackArg) - { - 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) - { + api["newUnsavableSimulationTimer"] = [world = context.mWorldView](const AsyncPackageId& asyncId, double delay, + sol::main_protected_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 - { - return LuaUtil::Callback{std::move(fn), asyncId.mHiddenData}; + api["callback"] = [](const AsyncPackageId& asyncId, sol::main_protected_function fn) -> LuaUtil::Callback { + return LuaUtil::Callback{ std::move(fn), asyncId.mHiddenData }; }; sol::usertype callbackType = context.mLua->sol().new_usertype("Callback"); diff --git a/apps/openmw/mwlua/luamanagerimp.hpp b/apps/openmw/mwlua/luamanagerimp.hpp index 25c867e601..fb5d7faf14 100644 --- a/apps/openmw/mwlua/luamanagerimp.hpp +++ b/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 std::function 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 mQueuedCallbacks; diff --git a/apps/openmw/mwlua/nearbybindings.cpp b/apps/openmw/mwlua/nearbybindings.cpp index 7a40bdc40c..ce10a19e47 100644 --- a/apps/openmw/mwlua/nearbybindings.cpp +++ b/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"] = - [manager=context.mLuaManager](const LuaUtil::Callback& callback, const osg::Vec3f& from, const osg::Vec3f& to) - { - manager->addAction([manager, callback, from, to] - { + api["asyncCastRenderingRay"] = [context](const LuaUtil::Callback& callback, const osg::Vec3f& from, + const osg::Vec3f& to) { + context.mLuaManager->addAction([context, 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)); }); }; diff --git a/components/lua/scriptscontainer.cpp b/components/lua/scriptscontainer.cpp index 9e20a215fa..f6758e92d7 100644 --- a/components/lua/scriptscontainer.cpp +++ b/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, - std::string_view callbackName, sol::object callbackArg) + void ScriptsContainer::setupSerializableTimer( + 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; diff --git a/components/lua/scriptscontainer.hpp b/components/lua/scriptscontainer.hpp index 1599966376..201218a6e2 100644 --- a/components/lua/scriptscontainer.hpp +++ b/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. - // callbackName - callback (should be registered in advance) for this timer. - // callbackArg - parameter for the callback (should be serializable). - void setupSerializableTimer(TimerType type, double time, int scriptId, - std::string_view callbackName, sol::object callbackArg); + // scriptPath - script path in VFS is used as script id. The script with the given path should already present + // in the container. callbackName - callback (should be registered in advance) for this timer. callbackArg - + // parameter for the callback (should be serializable). + void setupSerializableTimer( + 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 mInterface; std::string mInterfaceName; sol::table mHiddenData; - std::map mRegisteredCallbacks; - std::map mTemporaryCallbacks; + std::map mRegisteredCallbacks; + std::map mTemporaryCallbacks; std::string mPath; }; struct Timer @@ -204,8 +204,8 @@ namespace LuaUtil double mTime; bool mSerializable; int mScriptId; - std::variant mCallback; // string if serializable, integer otherwise - sol::object mArg; + std::variant mCallback; // string if serializable, integer otherwise + 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::table mHiddenData; // same object as Script::mHiddenData in ScriptsContainer + sol::main_protected_function mFunc; + sol::table mHiddenData; // same object as Script::mHiddenData in ScriptsContainer bool isValid() const { return mHiddenData[ScriptsContainer::sScriptIdKey] != sol::nil; } From 5cf96a808e747dd233f1e176f18da92e88f1f65a Mon Sep 17 00:00:00 2001 From: uramer Date: Mon, 17 Oct 2022 17:53:13 +0200 Subject: [PATCH 2/3] Lua coroutine crash tests --- apps/openmw_test_suite/CMakeLists.txt | 1 + apps/openmw_test_suite/lua/test_async.cpp | 55 +++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 apps/openmw_test_suite/lua/test_async.cpp diff --git a/apps/openmw_test_suite/CMakeLists.txt b/apps/openmw_test_suite/CMakeLists.txt index 113a0b61ec..40d9c57b13 100644 --- a/apps/openmw_test_suite/CMakeLists.txt +++ b/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 diff --git a/apps/openmw_test_suite/lua/test_async.cpp b/apps/openmw_test_suite/lua/test_async.cpp new file mode 100644 index 0000000000..c1e1fd6c86 --- /dev/null +++ b/apps/openmw_test_suite/lua/test_async.cpp @@ -0,0 +1,55 @@ +#include "gmock/gmock.h" +#include + +#include +#include + +#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"); + } +} From 710ad11dc870ea9e087eb7c089375f83897e8bf9 Mon Sep 17 00:00:00 2001 From: Petr Mikheev Date: Mon, 10 Oct 2022 23:32:17 +0200 Subject: [PATCH 3/3] Fix #7039: freeze after throwing an error in a queued Lua callback --- apps/openmw/mwlua/luamanagerimp.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/openmw/mwlua/luamanagerimp.cpp b/apps/openmw/mwlua/luamanagerimp.cpp index 1eb0352e3a..862deb0d3a 100644 --- a/apps/openmw/mwlua/luamanagerimp.cpp +++ b/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