diff --git a/src/slic3r/CMakeLists.txt b/src/slic3r/CMakeLists.txt index 605ac738bd..dbd3ab1991 100644 --- a/src/slic3r/CMakeLists.txt +++ b/src/slic3r/CMakeLists.txt @@ -616,6 +616,7 @@ set(SLIC3R_GUI_SOURCES plugin/PluginFsUtils.hpp plugin/PluginCatalog.cpp plugin/PluginCatalog.hpp + plugin/PluginCallbackList.hpp plugin/PluginLoader.cpp plugin/PluginLoader.hpp plugin/PluginDescriptor.hpp diff --git a/src/slic3r/plugin/PluginCallbackList.hpp b/src/slic3r/plugin/PluginCallbackList.hpp new file mode 100644 index 0000000000..4709881769 --- /dev/null +++ b/src/slic3r/plugin/PluginCallbackList.hpp @@ -0,0 +1,44 @@ +#pragma once + +#include +#include +#include +#include +#include + +namespace Slic3r { + +// Thread-safe append-only callback storage. Each dispatch operates on a snapshot, +// so callbacks may safely subscribe more callbacks without deadlocking or changing +// the in-flight dispatch. +template class PluginCallbackList +{ +public: + using Callback = std::function; + using CallbackPtr = std::shared_ptr; + + void subscribe(Callback callback) + { + auto owned_callback = std::make_shared(std::move(callback)); + std::lock_guard lock(m_mutex); + m_callbacks.emplace_back(std::move(owned_callback)); + } + + template void dispatch(Dispatcher&& dispatcher) const + { + std::vector callbacks; + { + std::lock_guard lock(m_mutex); + callbacks = m_callbacks; + } + + for (const CallbackPtr& callback : callbacks) + dispatcher(*callback); + } + +private: + mutable std::mutex m_mutex; + std::vector m_callbacks; +}; + +} // namespace Slic3r diff --git a/src/slic3r/plugin/PluginLoader.cpp b/src/slic3r/plugin/PluginLoader.cpp index e5a96b87d3..489e4d33c7 100644 --- a/src/slic3r/plugin/PluginLoader.cpp +++ b/src/slic3r/plugin/PluginLoader.cpp @@ -1429,7 +1429,7 @@ void PluginLoader::unload_cloud_plugins() void PluginLoader::run_on_load_callbacks(const std::string& plugin_key) { - for (auto& fn : m_callbacks[CallbackType::Load]) { + m_load_callbacks.dispatch([&plugin_key](const PluginLifecycleCompleteFn& fn) { try { fn(plugin_key); } catch (const std::exception& ex) { @@ -1437,12 +1437,12 @@ void PluginLoader::run_on_load_callbacks(const std::string& plugin_key) } catch (...) { BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key; } - } + }); } void PluginLoader::run_on_unload_callbacks(const std::string& plugin_key) { - for (auto& fn : m_callbacks[CallbackType::Unload]) { + m_unload_callbacks.dispatch([&plugin_key](const PluginLifecycleCompleteFn& fn) { try { fn(plugin_key); } catch (const std::exception& ex) { @@ -1450,12 +1450,12 @@ void PluginLoader::run_on_unload_callbacks(const std::string& plugin_key) } catch (...) { BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": unknown error"; } - } + }); } void PluginLoader::run_on_capability_load_callbacks(const PluginCapabilityIdentifier& id) { - for (auto& fn : m_capability_callbacks[CallbackType::Load]) { + m_capability_load_callbacks.dispatch([&id](const CapabilityLifecycleFn& fn) { try { fn(id); } catch (const std::exception& ex) { @@ -1465,12 +1465,12 @@ void PluginLoader::run_on_capability_load_callbacks(const PluginCapabilityIdenti BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name << ": unknown error"; } - } + }); } void PluginLoader::run_on_capability_unload_callbacks(const PluginCapabilityIdentifier& id) { - for (auto& fn : m_capability_callbacks[CallbackType::Unload]) { + m_capability_unload_callbacks.dispatch([&id](const CapabilityLifecycleFn& fn) { try { fn(id); } catch (const std::exception& ex) { @@ -1480,27 +1480,27 @@ void PluginLoader::run_on_capability_unload_callbacks(const PluginCapabilityIden BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name << ": unknown error"; } - } + }); } void PluginLoader::subscribe_on_load_callback(PluginLifecycleCompleteFn fn) { - m_callbacks[CallbackType::Load].push_back(std::move(fn)); + m_load_callbacks.subscribe(std::move(fn)); } void PluginLoader::subscribe_on_unload_callback(PluginLifecycleCompleteFn fn) { - m_callbacks[CallbackType::Unload].push_back(std::move(fn)); + m_unload_callbacks.subscribe(std::move(fn)); } void PluginLoader::subscribe_on_capability_load_callback(CapabilityLifecycleFn fn) { - m_capability_callbacks[CallbackType::Load].push_back(std::move(fn)); + m_capability_load_callbacks.subscribe(std::move(fn)); } void PluginLoader::subscribe_on_capability_unload_callback(CapabilityLifecycleFn fn) { - m_capability_callbacks[CallbackType::Unload].push_back(std::move(fn)); + m_capability_unload_callbacks.subscribe(std::move(fn)); } } // namespace Slic3r diff --git a/src/slic3r/plugin/PluginLoader.hpp b/src/slic3r/plugin/PluginLoader.hpp index e3903001e4..cfa7baed23 100644 --- a/src/slic3r/plugin/PluginLoader.hpp +++ b/src/slic3r/plugin/PluginLoader.hpp @@ -1,5 +1,6 @@ #pragma once +#include "PluginCallbackList.hpp" #include "PluginDescriptor.hpp" #include @@ -197,8 +198,10 @@ private: mutable std::mutex m_mutex; mutable std::condition_variable m_plugin_load_cv; - std::unordered_map> m_callbacks{}; - std::unordered_map> m_capability_callbacks{}; + PluginCallbackList m_load_callbacks; + PluginCallbackList m_unload_callbacks; + PluginCallbackList m_capability_load_callbacks; + PluginCallbackList m_capability_unload_callbacks; /* callbacks: diff --git a/tests/slic3rutils/CMakeLists.txt b/tests/slic3rutils/CMakeLists.txt index e122e4e760..ad43adbfdb 100644 --- a/tests/slic3rutils/CMakeLists.txt +++ b/tests/slic3rutils/CMakeLists.txt @@ -3,6 +3,7 @@ add_executable(${_TEST_NAME}_tests ${_TEST_NAME}_tests_main.cpp test_action_source.cpp test_plugin_host_api.cpp + test_plugin_callback_list.cpp test_plugin_capability_identifier.cpp test_plugin_install.cpp test_plugin_sort.cpp diff --git a/tests/slic3rutils/test_plugin_callback_list.cpp b/tests/slic3rutils/test_plugin_callback_list.cpp new file mode 100644 index 0000000000..2d9c3678bf --- /dev/null +++ b/tests/slic3rutils/test_plugin_callback_list.cpp @@ -0,0 +1,105 @@ +#include + +#include + +#include +#include +#include +#include +#include +#include +#include + +using Slic3r::PluginCallbackList; + +TEST_CASE("PluginCallbackList dispatches subscribers in subscription order", "[plugin][callback-list]") +{ + PluginCallbackList callbacks; + std::vector calls; + + callbacks.subscribe([&calls] { calls.push_back(1); }); + callbacks.subscribe([&calls] { calls.push_back(2); }); + + callbacks.dispatch([](const auto& callback) { callback(); }); + + CHECK(calls == std::vector{1, 2}); +} + +TEST_CASE("PluginCallbackList defers subscribers added during dispatch", "[plugin][callback-list]") +{ + PluginCallbackList callbacks; + std::vector calls; + + callbacks.subscribe([&] { + calls.emplace_back("original"); + callbacks.subscribe([&calls] { calls.emplace_back("added"); }); + }); + + callbacks.dispatch([](const auto& callback) { callback(); }); + CHECK(calls == std::vector{"original"}); + + callbacks.dispatch([](const auto& callback) { callback(); }); + CHECK(calls == std::vector{"original", "original", "added"}); +} + +TEST_CASE("PluginCallbackList preserves mutable callback state across dispatches", "[plugin][callback-list]") +{ + PluginCallbackList callbacks; + std::vector calls; + + callbacks.subscribe([count = 0, &calls]() mutable { calls.push_back(++count); }); + + callbacks.dispatch([](const auto& callback) { callback(); }); + callbacks.dispatch([](const auto& callback) { callback(); }); + + CHECK(calls == std::vector{1, 2}); +} + +TEST_CASE("PluginCallbackList does not hold its mutex while invoking callbacks", "[plugin][callback-list]") +{ + using namespace std::chrono_literals; + + PluginCallbackList callbacks; + std::mutex mutex; + std::condition_variable callback_entered_cv; + std::condition_variable release_callback_cv; + bool callback_entered = false; + bool release_callback = false; + std::atomic late_calls{0}; + + callbacks.subscribe([&] { + std::unique_lock lock(mutex); + callback_entered = true; + callback_entered_cv.notify_one(); + release_callback_cv.wait(lock, [&release_callback] { return release_callback; }); + }); + + auto dispatch = std::async(std::launch::async, [&] { + callbacks.dispatch([](const auto& callback) { callback(); }); + }); + + { + std::unique_lock lock(mutex); + callback_entered_cv.wait_for(lock, 5s, [&callback_entered] { return callback_entered; }); + } + + auto subscribe = std::async(std::launch::async, [&] { + callbacks.subscribe([&late_calls] { ++late_calls; }); + }); + const bool subscribed_while_callback_blocked = subscribe.wait_for(5s) == std::future_status::ready; + + { + std::lock_guard lock(mutex); + release_callback = true; + } + release_callback_cv.notify_one(); + + dispatch.get(); + subscribe.get(); + CHECK(callback_entered); + CHECK(subscribed_while_callback_blocked); + CHECK(late_calls.load() == 0); + + callbacks.dispatch([](const auto& callback) { callback(); }); + CHECK(late_calls.load() == 1); +}