diff --git a/CMakeLists.txt b/CMakeLists.txt index 0c2a128..8faeb22 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,7 +1,7 @@ cmake_minimum_required(VERSION 3.14) project(SimplePluginExample LANGUAGES CXX) -set(CMAKE_CXX_STANDARD 11) +set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED ON) # Find Qt packages diff --git a/README.md b/README.md index 3c2c0a1..1bd3a23 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,12 @@ void logos_core_process_events(); See `src/logos_core/logos_core.h` for the full API. +### Thread safety + +All C API functions that mutate plugin state (`logos_core_load_plugin`, `logos_core_load_plugin_with_dependencies`, `logos_core_unload_plugin`, `logos_core_refresh_plugins`) are serialised internally by a single mutex. It is safe to call them concurrently from multiple threads, including rapid and repeated load/unload cycles on the same module — each call waits for its turn and the process management layer handles teardown cleanly before the next launch. + +Read-only accessors (`logos_core_get_known_plugins`, `logos_core_get_loaded_plugins`) use a shared reader-writer lock and are safe to call concurrently with each other and with the mutating functions above. + ## Dev vs Portable Builds The library supports two build modes controlled by the `LOGOS_PORTABLE_BUILD` CMake flag: diff --git a/docs/project.md b/docs/project.md index 145cad4..c511b47 100644 --- a/docs/project.md +++ b/docs/project.md @@ -98,6 +98,8 @@ logos-liblogos/ **Purpose:** Thin facade that orchestrates `PluginRegistry`, `PluginLauncher`, and `DependencyResolver`. Provides the C++-level API for module lifecycle management. Each module runs in a separate `logos_host` process for isolation. +**Thread safety:** `loadPlugin`, `loadPluginWithDependencies`, and `unloadPlugin` are serialised by a static `loadMutex()` (one load/unload at a time). `discoverInstalledModules` delegates to `PluginRegistry` which has its own reader-writer lock. + **API (namespace `PluginManager`):** | Method | Description | @@ -123,12 +125,13 @@ logos-liblogos/ **Files:** `src/logos_core/plugin_registry.h`, `src/logos_core/plugin_registry.cpp` -**Purpose:** In-memory registry of discovered and loaded modules. Stores plugin paths, dependencies, and load state. +**Purpose:** In-memory registry of discovered and loaded modules. Stores plugin paths, dependencies, and load state. All public methods are thread-safe: mutating methods acquire a `std::unique_lock` on an internal `std::shared_mutex`; read-only methods acquire a `std::shared_lock`, allowing concurrent reads. **Data:** - `PluginInfo` struct — holds `path`, `dependencies` (QStringList), `loaded` flag - `QHash m_plugins` — plugin database keyed by name - `QStringList m_pluginsDirs` — configured plugin directories +- `std::shared_mutex m_mutex` — reader-writer lock protecting all fields **API (class `PluginRegistry`):** @@ -189,7 +192,7 @@ Takes callback functions (`IsKnownFn`, `GetDependenciesFn`) so it has no couplin **Purpose:** Isolates all Qt-specific code behind internal interfaces. - **QtAppContext** — Creates/manages `QCoreApplication`, runs event loop, processes events -- **QtProcessManager** — Manages `QProcess` instances for module subprocesses, handles exit/error signals, sends tokens via stdin +- **QtProcessManager** — Manages `QProcess` instances for module subprocesses, handles exit/error signals, sends tokens via local socket. A `std::mutex` (`s_processesMutex`) protects the `s_processes` map against concurrent access from calling threads and async signal deliveries. All teardown paths use a shared `destroyProcess` helper that disconnects signals before waiting, preventing double-free from deferred `deleteLater` calls. ### ProcessStats (external dependency) @@ -245,6 +248,15 @@ The public C API (`logos_core.h`) is the only exported interface. All functions | `logos_core_get_module_stats() → char*` | JSON array of CPU/memory stats (caller frees) | | `logos_core_get_token(key) → char*` | Get auth token by key (caller frees) | +### Thread Safety + +| Category | Guarantee | +|----------|-----------| +| `logos_core_load_plugin`, `logos_core_load_plugin_with_dependencies`, `logos_core_unload_plugin` | Serialised by a single internal mutex — safe to call concurrently from multiple threads | +| `logos_core_get_known_plugins`, `logos_core_get_loaded_plugins` | Protected by a shared reader-writer lock — safe to call concurrently with each other and with the mutating functions above | +| `logos_core_refresh_plugins` | Protected by `PluginRegistry`'s reader-writer lock (write side) — safe for concurrent registry access but not serialised against load/unload | +| `logos_core_init`, `logos_core_start`, `logos_core_cleanup` | Not thread-safe — must be called from a single thread during startup/shutdown | + ## Build Artifacts | Artifact | Description | diff --git a/docs/spec.md b/docs/spec.md index 6bbc2c7..9317ff0 100644 --- a/docs/spec.md +++ b/docs/spec.md @@ -137,6 +137,15 @@ Every module ships a `metadata.json` referenced by Qt's `Q_PLUGIN_METADATA` macr - Core Manager process is excluded from stats - Not available on iOS +### Thread Safety + +The C API is designed to be safe for use from multi-threaded host applications: + +- **Load/unload operations** (`load_plugin`, `load_plugin_with_dependencies`, `unload_plugin`) are serialised — only one runs at a time, so rapid concurrent load/unload cycles on the same or different modules do not produce data races. +- **Read-only queries** (`get_known_plugins`, `get_loaded_plugins`) use a shared reader-writer lock and may execute concurrently with each other and with load/unload operations. +- **Plugin discovery** (`refresh_plugins`) is protected by the registry's own write lock. +- **Lifecycle functions** (`init`, `start`, `cleanup`) are not thread-safe and must be called from a single thread. + ### Dev vs Portable Builds The platform supports two build variants: diff --git a/src/logos_core/app_lifecycle.cpp b/src/logos_core/app_lifecycle.cpp index 4a5535c..a5f0640 100644 --- a/src/logos_core/app_lifecycle.cpp +++ b/src/logos_core/app_lifecycle.cpp @@ -21,7 +21,7 @@ namespace AppLifecycle { } void cleanup() { - PluginManager::terminateAll(); + PluginManager::clear(); QtAppContext::cleanup(); } diff --git a/src/logos_core/plugin_manager.cpp b/src/logos_core/plugin_manager.cpp index e72e10d..d552663 100644 --- a/src/logos_core/plugin_manager.cpp +++ b/src/logos_core/plugin_manager.cpp @@ -4,6 +4,7 @@ #include "plugin_launcher.h" #include #include +#include #include #include #include "logos_api.h" @@ -16,6 +17,11 @@ namespace { return instance; } + std::mutex& loadMutex() { + static std::mutex mutex; + return mutex; + } + char** toNullTerminatedArray(const QStringList& list) { int count = list.size(); if (count == 0) { @@ -50,6 +56,44 @@ namespace { qWarning() << "Failed to register token with capability module for:" << name; } } + + bool loadPluginInternal(const char* pluginName) { + QString name = QString::fromUtf8(pluginName); + + if (!registryInstance().isKnown(name)) { + qWarning() << "Cannot load unknown plugin:" << name; + return false; + } + + if (registryInstance().isLoaded(name)) { + qWarning() << "Plugin already loaded:" << name; + return false; + } + + QString pluginPath = registryInstance().pluginPath(name); + + auto onTerminated = [](const QString& n) { + registryInstance().markUnloaded(n); + }; + + if (!PluginLauncher::launch(name, pluginPath, registryInstance().pluginsDirs(), onTerminated)) + return false; + + QString authToken = QUuid::createUuid().toString(QUuid::WithoutBraces); + + if (!PluginLauncher::sendToken(name, authToken)) + return false; + + registryInstance().markLoaded(name); + + TokenManager::instance().saveToken(name, authToken); + + notifyCapabilityModule(name, authToken); + + qInfo() << "Plugin loaded:" << name; + + return true; + } } namespace PluginManager { @@ -92,44 +136,13 @@ namespace PluginManager { } bool loadPlugin(const char* pluginName) { - QString name = QString::fromUtf8(pluginName); - - if (!registryInstance().isKnown(name)) { - qWarning() << "Cannot load unknown plugin:" << name; - return false; - } - - if (registryInstance().isLoaded(name)) { - qWarning() << "Plugin already loaded:" << name; - return false; - } - - QString pluginPath = registryInstance().pluginPath(name); - - auto onTerminated = [](const QString& n) { - registryInstance().markUnloaded(n); - }; - - if (!PluginLauncher::launch(name, pluginPath, registryInstance().pluginsDirs(), onTerminated)) - return false; - - QString authToken = QUuid::createUuid().toString(QUuid::WithoutBraces); - - if (!PluginLauncher::sendToken(name, authToken)) - return false; - - registryInstance().markLoaded(name); - - TokenManager::instance().saveToken(name, authToken); - - notifyCapabilityModule(name, authToken); - - qInfo() << "Plugin loaded:" << name; - - return true; + std::lock_guard lock(loadMutex()); + return loadPluginInternal(pluginName); } bool loadPluginWithDependencies(const char* pluginName) { + std::lock_guard lock(loadMutex()); + QString name = QString::fromUtf8(pluginName); QStringList requested; @@ -150,7 +163,7 @@ namespace PluginManager { for (const QString& moduleName : resolved) { if (registryInstance().isLoaded(moduleName)) continue; - if (!loadPlugin(moduleName.toUtf8().constData())) { + if (!loadPluginInternal(moduleName.toUtf8().constData())) { qWarning() << "Failed to load plugin:" << moduleName; allSucceeded = false; } @@ -160,10 +173,12 @@ namespace PluginManager { } bool initializeCapabilityModule() { + std::lock_guard lock(loadMutex()); + if (!registryInstance().isKnown("capability_module")) return false; - if (!loadPlugin("capability_module")) { + if (!loadPluginInternal("capability_module")) { qWarning() << "Failed to load capability module"; return false; } @@ -172,6 +187,8 @@ namespace PluginManager { } bool unloadPlugin(const char* pluginName) { + std::lock_guard lock(loadMutex()); + QString name = QString::fromUtf8(pluginName); if (!registryInstance().isLoaded(name)) { @@ -192,10 +209,17 @@ namespace PluginManager { } void terminateAll() { + std::lock_guard lock(loadMutex()); PluginLauncher::terminateAll(); registryInstance().clearLoaded(); } + void clear() { + std::lock_guard lock(loadMutex()); + PluginLauncher::terminateAll(); + registryInstance().clear(); + } + char** getLoadedPluginsCStr() { return toNullTerminatedArray(registryInstance().loadedPluginNames()); } diff --git a/src/logos_core/plugin_manager.h b/src/logos_core/plugin_manager.h index cdc5cb1..02633b8 100644 --- a/src/logos_core/plugin_manager.h +++ b/src/logos_core/plugin_manager.h @@ -22,6 +22,7 @@ namespace PluginManager { bool initializeCapabilityModule(); bool unloadPlugin(const char* pluginName); void terminateAll(); + void clear(); char** getLoadedPluginsCStr(); char** getKnownPluginsCStr(); diff --git a/src/logos_core/plugin_registry.cpp b/src/logos_core/plugin_registry.cpp index a8ebc4c..1159663 100644 --- a/src/logos_core/plugin_registry.cpp +++ b/src/logos_core/plugin_registry.cpp @@ -2,6 +2,8 @@ #include #include #include +#include +#include #include #include #include @@ -12,20 +14,25 @@ static PackageManagerLib& packageManagerInstance() { } void PluginRegistry::setPluginsDir(const QString& dir) { + std::unique_lock lock(m_mutex); m_pluginsDirs.clear(); m_pluginsDirs.append(dir); } void PluginRegistry::addPluginsDir(const QString& dir) { + std::unique_lock lock(m_mutex); if (m_pluginsDirs.contains(dir)) return; m_pluginsDirs.append(dir); } QStringList PluginRegistry::pluginsDirs() const { + std::shared_lock lock(m_mutex); return m_pluginsDirs; } void PluginRegistry::discoverInstalledModules() { + std::unique_lock lock(m_mutex); + PackageManagerLib& pm = packageManagerInstance(); if (!m_pluginsDirs.isEmpty()) { pm.setEmbeddedModulesDirectory(m_pluginsDirs.first().toStdString()); @@ -45,7 +52,7 @@ void PluginRegistry::discoverInstalledModules() { if (name.isEmpty() || mainFilePath.isEmpty()) continue; - QString pluginName = processPlugin(mainFilePath); + QString pluginName = processPluginInternal(mainFilePath); if (pluginName.isEmpty()) { qWarning() << "Failed to process plugin:" << mainFilePath; } @@ -53,6 +60,11 @@ void PluginRegistry::discoverInstalledModules() { } QString PluginRegistry::processPlugin(const QString& pluginPath) { + std::unique_lock lock(m_mutex); + return processPluginInternal(pluginPath); +} + +QString PluginRegistry::processPluginInternal(const QString& pluginPath) { std::string name = ModuleLib::LogosModule::getModuleName(pluginPath.toStdString()); if (name.empty()) { qWarning() << "No valid metadata for plugin:" << pluginPath; @@ -72,23 +84,28 @@ QString PluginRegistry::processPlugin(const QString& pluginPath) { } bool PluginRegistry::isKnown(const QString& name) const { + std::shared_lock lock(m_mutex); return m_plugins.contains(name); } QString PluginRegistry::pluginPath(const QString& name) const { + std::shared_lock lock(m_mutex); return m_plugins.value(name).path; } QStringList PluginRegistry::pluginDependencies(const QString& name) const { + std::shared_lock lock(m_mutex); return m_plugins.value(name).dependencies; } QStringList PluginRegistry::knownPluginNames() const { + std::shared_lock lock(m_mutex); return m_plugins.keys(); } void PluginRegistry::registerPlugin(const QString& name, const QString& path, const QStringList& dependencies) { + std::unique_lock lock(m_mutex); PluginInfo& info = m_plugins[name]; info.path = path; if (!dependencies.isEmpty()) @@ -96,23 +113,28 @@ void PluginRegistry::registerPlugin(const QString& name, const QString& path, } void PluginRegistry::registerDependencies(const QString& name, const QStringList& dependencies) { + std::unique_lock lock(m_mutex); m_plugins[name].dependencies = dependencies; } bool PluginRegistry::isLoaded(const QString& name) const { + std::shared_lock lock(m_mutex); return m_plugins.value(name).loaded; } void PluginRegistry::markLoaded(const QString& name) { + std::unique_lock lock(m_mutex); m_plugins[name].loaded = true; } void PluginRegistry::markUnloaded(const QString& name) { + std::unique_lock lock(m_mutex); if (m_plugins.contains(name)) m_plugins[name].loaded = false; } QStringList PluginRegistry::loadedPluginNames() const { + std::shared_lock lock(m_mutex); QStringList result; for (auto it = m_plugins.constBegin(); it != m_plugins.constEnd(); ++it) { if (it.value().loaded) @@ -122,11 +144,13 @@ QStringList PluginRegistry::loadedPluginNames() const { } void PluginRegistry::clearLoaded() { + std::unique_lock lock(m_mutex); for (auto it = m_plugins.begin(); it != m_plugins.end(); ++it) it.value().loaded = false; } void PluginRegistry::clear() { + std::unique_lock lock(m_mutex); m_pluginsDirs.clear(); m_plugins.clear(); } diff --git a/src/logos_core/plugin_registry.h b/src/logos_core/plugin_registry.h index f93295b..ffc33d1 100644 --- a/src/logos_core/plugin_registry.h +++ b/src/logos_core/plugin_registry.h @@ -4,6 +4,7 @@ #include #include #include +#include struct PluginInfo { QString path; @@ -37,6 +38,9 @@ public: void clear(); private: + QString processPluginInternal(const QString& pluginPath); + + mutable std::shared_mutex m_mutex; QStringList m_pluginsDirs; QHash m_plugins; }; diff --git a/src/logos_core/qt/qt_process_manager.cpp b/src/logos_core/qt/qt_process_manager.cpp index 9850691..9305cae 100644 --- a/src/logos_core/qt/qt_process_manager.cpp +++ b/src/logos_core/qt/qt_process_manager.cpp @@ -4,12 +4,31 @@ #include #include #include +#include namespace { QHash s_processes; + std::mutex s_processesMutex; QString toQ(const std::string& s) { return QString::fromStdString(s); } std::string fromQ(const QString& s) { return s.toStdString(); } + + // Tears down a process that has already been removed from s_processes. + // Must be called without s_processesMutex held (waitForFinished can block). + void destroyProcess(QProcess* process, const QString& name) { + // Disconnect all signal handlers before waiting. The finished-signal + // lambda calls process->deleteLater(), which would queue a deferred + // deletion on an object we are about to delete directly — causing a + // double-free when that deferred deletion fires later. + process->disconnect(); + process->terminate(); + if (!process->waitForFinished(5000)) { + qWarning() << "Process did not terminate gracefully, killing it:" << name; + process->kill(); + process->waitForFinished(2000); + } + delete process; + } } namespace QtProcessManager { @@ -33,13 +52,19 @@ namespace QtProcessManager { return false; } - s_processes.insert(qName, process); + { + std::lock_guard lock(s_processesMutex); + s_processes.insert(qName, process); + } QObject::connect(process, QOverload::of(&QProcess::finished), [name, qName, process, callbacks](int exitCode, QProcess::ExitStatus exitStatus) { bool crashed = (exitStatus == QProcess::CrashExit); - s_processes.remove(qName); + { + std::lock_guard lock(s_processesMutex); + s_processes.remove(qName); + } process->deleteLater(); if (callbacks.onFinished) { @@ -107,10 +132,12 @@ namespace QtProcessManager { qCritical() << "Failed to connect to token socket for:" << qName; tokenSocket->deleteLater(); - if (s_processes.contains(qName)) { - s_processes.value(qName)->terminate(); - delete s_processes.take(qName); + QProcess* p = nullptr; + { + std::lock_guard lock(s_processesMutex); + p = s_processes.take(qName); } + if (p) destroyProcess(p, qName); return false; } @@ -123,60 +150,48 @@ namespace QtProcessManager { return true; } + void terminateProcess(const std::string& name) { QString qName = toQ(name); - if (!s_processes.contains(qName)) return; - - QProcess* process = s_processes.take(qName); - if (!process) return; - - process->terminate(); - - if (!process->waitForFinished(5000)) { - qWarning() << "Process did not terminate gracefully, killing it:" << qName; - process->kill(); - process->waitForFinished(2000); + QProcess* process = nullptr; + { + std::lock_guard lock(s_processesMutex); + process = s_processes.take(qName); } - - delete process; + if (!process) return; + destroyProcess(process, qName); } void terminateAll() { - if (s_processes.isEmpty()) return; - - // Copy and clear first to avoid iterator invalidation from the - // finished signal handler which removes entries from s_processes. - QHash snapshot = s_processes; - s_processes.clear(); + QHash snapshot; + { + std::lock_guard lock(s_processesMutex); + if (s_processes.isEmpty()) return; + // Swap out the whole map so the finished-signal lambda (which also + // locks s_processesMutex) cannot race with our teardown loop. + snapshot.swap(s_processes); + } for (auto it = snapshot.begin(); it != snapshot.end(); ++it) { - QProcess* process = it.value(); - QString processName = it.key(); - - process->terminate(); - - if (!process->waitForFinished(3000)) { - qWarning() << "Process did not terminate gracefully, killing it:" << processName; - process->kill(); - process->waitForFinished(1000); - } - - delete process; + if (it.value()) + destroyProcess(it.value(), it.key()); } } bool hasProcess(const std::string& name) { + std::lock_guard lock(s_processesMutex); return s_processes.contains(toQ(name)); } int64_t getProcessId(const std::string& name) { + std::lock_guard lock(s_processesMutex); QString qName = toQ(name); - if (!s_processes.contains(qName)) return -1; QProcess* process = s_processes.value(qName); return process ? process->processId() : -1; } std::unordered_map getAllProcessIds() { + std::lock_guard lock(s_processesMutex); std::unordered_map result; for (auto it = s_processes.begin(); it != s_processes.end(); ++it) { if (it.value()) { @@ -187,18 +202,24 @@ namespace QtProcessManager { } void clearAll() { - for (auto it = s_processes.begin(); it != s_processes.end(); ++it) { + QHash snapshot; + { + std::lock_guard lock(s_processesMutex); + snapshot.swap(s_processes); + } + for (auto it = snapshot.begin(); it != snapshot.end(); ++it) { QProcess* process = it.value(); if (process) { + process->disconnect(); process->terminate(); process->waitForFinished(1000); delete process; } } - s_processes.clear(); } void registerProcess(const std::string& name) { + std::lock_guard lock(s_processesMutex); QString qName = toQ(name); if (!s_processes.contains(qName)) { s_processes.insert(qName, nullptr);