From 04abc375f5b3a9aad42da32b0b98ccb489a49fca Mon Sep 17 00:00:00 2001 From: Marcus Tillmanns Date: Tue, 28 May 2024 07:40:25 +0200 Subject: [PATCH] Lua: Improve memory management Previously the PluginSpec would create a keep around a single lua vm that was then shared with the actual plugin instance. This made it unclear when the VM was freed. Now we instead cleanup the lua vm used to fetch the plugin infos immediately and instead create a fresh vm for the actual plugin instance that can then follow the normal Plugin lifecylce. Change-Id: I81bb9ecf57706c2ba1b0d8db83ab26b3b8e944f2 Reviewed-by: David Schulz --- src/plugins/lua/luaengine.cpp | 44 +++++++++++++++++++++---------- src/plugins/lua/luaengine.h | 6 ++--- src/plugins/lua/luapluginspec.cpp | 39 ++++++++++++++------------- src/plugins/lua/luapluginspec.h | 5 ++-- 4 files changed, 54 insertions(+), 40 deletions(-) diff --git a/src/plugins/lua/luaengine.cpp b/src/plugins/lua/luaengine.cpp index 505313da865..29ca17fad6e 100644 --- a/src/plugins/lua/luaengine.cpp +++ b/src/plugins/lua/luaengine.cpp @@ -4,6 +4,7 @@ #include "luaengine.h" #include "luapluginspec.h" +#include "luatr.h" #include #include @@ -172,14 +173,6 @@ expected_str LuaEngine::connectHooks( return {}; } -expected_str LuaEngine::connectHooks(sol::state_view lua, const sol::table &hookTable) -{ - if (!hookTable) - return {}; - - return instance().connectHooks(lua, hookTable, ""); -} - expected_str LuaEngine::loadPlugin(const Utils::FilePath &path) { auto contents = path.fileContents(); @@ -204,12 +197,16 @@ expected_str LuaEngine::loadPlugin(const Utils::FilePath &path) sol::table pluginInfo = result.get(); if (!pluginInfo.valid()) return make_unexpected(QString("Script did not return a table with plugin info")); - return LuaPluginSpec::create(path, std::move(lua), pluginInfo); + return LuaPluginSpec::create(path, pluginInfo); } -expected_str LuaEngine::prepareSetup( - sol::state_view &lua, const LuaPluginSpec &pluginSpec, sol::optional hookTable) +expected_str LuaEngine::prepareSetup( + sol::state_view lua, const LuaPluginSpec &pluginSpec) { + auto contents = pluginSpec.filePath().fileContents(); + if (!contents) + return make_unexpected(contents.error()); + // TODO: Only open libraries requested by the plugin lua.open_libraries( sol::lib::base, @@ -261,10 +258,29 @@ expected_str LuaEngine::prepareSetup( for (const auto &func : d->m_autoProviders) func(lua); - if (hookTable) - return LuaEngine::connectHooks(lua, *hookTable); + sol::protected_function_result result = lua.safe_script( + std::string_view(contents->data(), contents->size()), + sol::script_pass_on_error, + pluginSpec.filePath().fileName().toUtf8().constData()); - return {}; + auto pluginTable = result.get>(); + if (!pluginTable) + return make_unexpected(Tr::tr("Script did not return a table")); + + auto hookTable = pluginTable->get>("hooks"); + + if (hookTable) { + auto connectResult = connectHooks(lua, *hookTable, {}); + if (!connectResult) + return make_unexpected(connectResult.error()); + } + + auto setupFunction = pluginTable->get_or("setup", {}); + + if (!setupFunction) + return make_unexpected(Tr::tr("Plugin info table did not contain a setup function")); + + return setupFunction; } bool LuaEngine::isCoroutine(lua_State *state) diff --git a/src/plugins/lua/luaengine.h b/src/plugins/lua/luaengine.h index 688c728808e..f8fa085d6d0 100644 --- a/src/plugins/lua/luaengine.h +++ b/src/plugins/lua/luaengine.h @@ -55,15 +55,13 @@ public: static LuaEngine &instance(); Utils::expected_str loadPlugin(const Utils::FilePath &path); - Utils::expected_str prepareSetup( - sol::state_view &lua, const LuaPluginSpec &pluginSpec, sol::optional hookTable); + Utils::expected_str prepareSetup( + sol::state_view lua, const LuaPluginSpec &pluginSpec); static void registerProvider(const QString &packageName, const PackageProvider &provider); static void autoRegister(const std::function ®isterFunction); static void registerHook(QString name, const std::function &hookProvider); - static Utils::expected_str connectHooks(sol::state_view lua, const sol::table &hookTable); - static bool isCoroutine(lua_State *state); static sol::table toTable(const sol::state_view &lua, const QJsonValue &v); diff --git a/src/plugins/lua/luapluginspec.cpp b/src/plugins/lua/luapluginspec.cpp index 4c4ac66d4e0..c3fcd558a55 100644 --- a/src/plugins/lua/luapluginspec.cpp +++ b/src/plugins/lua/luapluginspec.cpp @@ -34,28 +34,19 @@ class LuaPluginSpecPrivate { public: FilePath pluginScriptPath; - - sol::state lua; - sol::table pluginTable; - - sol::function setupFunction; + bool printToOutputPane = false; + std::unique_ptr activeLuaState; }; LuaPluginSpec::LuaPluginSpec() : d(new LuaPluginSpecPrivate()) {} -expected_str LuaPluginSpec::create(const FilePath &filePath, - sol::state lua, - sol::table pluginTable) +expected_str LuaPluginSpec::create(const FilePath &filePath, sol::table pluginTable) { std::unique_ptr pluginSpec(new LuaPluginSpec()); - pluginSpec->d->lua = std::move(lua); - pluginSpec->d->pluginTable = pluginTable; - - pluginSpec->d->setupFunction = pluginTable.get_or("setup", {}); - if (!pluginSpec->d->setupFunction) + if (!pluginTable.get_or("setup", {})) return make_unexpected(QString("Plugin info table did not contain a setup function")); QJsonValue v = LuaEngine::toJson(pluginTable); @@ -75,6 +66,7 @@ expected_str LuaPluginSpec::create(const FilePath &filePath, pluginSpec->setLocation(filePath.parentDir()); pluginSpec->d->pluginScriptPath = filePath; + pluginSpec->d->printToOutputPane = pluginTable.get_or("printToOutputPane", false); return pluginSpec.release(); } @@ -94,16 +86,19 @@ bool LuaPluginSpec::loadLibrary() } bool LuaPluginSpec::initializePlugin() { - expected_str setupResult - = LuaEngine::instance() - .prepareSetup(d->lua, *this, d->pluginTable.get>("hooks")); + QTC_ASSERT(!d->activeLuaState, return false); + + std::unique_ptr activeLuaState = std::make_unique(); + + expected_str setupResult + = LuaEngine::instance().prepareSetup(*activeLuaState, *this); if (!setupResult) { setError(Lua::Tr::tr("Failed to prepare plugin setup: %1").arg(setupResult.error())); return false; } - auto result = d->setupFunction.call(); + auto result = setupResult->call(); if (result.get_type() == sol::type::boolean && result.get() == false) { setError(Lua::Tr::tr("Plugin setup function returned false")); @@ -117,6 +112,8 @@ bool LuaPluginSpec::initializePlugin() } } + d->activeLuaState = std::move(activeLuaState); + setState(PluginSpec::State::Initialized); return true; } @@ -133,14 +130,18 @@ bool LuaPluginSpec::delayedInitialize() } ExtensionSystem::IPlugin::ShutdownFlag LuaPluginSpec::stop() { + d->activeLuaState.reset(); return ExtensionSystem::IPlugin::ShutdownFlag::SynchronousShutdown; } -void LuaPluginSpec::kill() {} +void LuaPluginSpec::kill() +{ + d->activeLuaState.reset(); +} bool LuaPluginSpec::printToOutputPane() const { - return d->pluginTable.get_or("printToOutputPane", false); + return d->printToOutputPane; } } // namespace Lua diff --git a/src/plugins/lua/luapluginspec.h b/src/plugins/lua/luapluginspec.h index 2d72bb53907..7c2ab3318bd 100644 --- a/src/plugins/lua/luapluginspec.h +++ b/src/plugins/lua/luapluginspec.h @@ -39,9 +39,8 @@ class LuaPluginSpec : public ExtensionSystem::PluginSpec LuaPluginSpec(); public: - static Utils::expected_str create(const Utils::FilePath &filePath, - sol::state lua, - sol::table pluginTable); + static Utils::expected_str create( + const Utils::FilePath &filePath, sol::table pluginTable); ExtensionSystem::IPlugin *plugin() const override;