From 1c33983edf994c04d25ecd9a8be4020d32e58c84 Mon Sep 17 00:00:00 2001 From: Andrew <159703254+andrewsoonqn@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:01:30 +0800 Subject: [PATCH] Confine plugin lifecycle callbacks to UI thread Drop the orphaned PluginCallbackList (dead after the PluginManager migration) and hop run_on_*_callbacks onto the UI thread via CallAfter, snapshotting under the mutex on the worker first. Keeps wx subscribers off the detached load/unload workers. --- src/slic3r/plugin/PluginCallbackList.hpp | 44 -------- src/slic3r/plugin/PluginManager.cpp | 101 +++++++++++------ tests/slic3rutils/CMakeLists.txt | 1 - .../slic3rutils/test_plugin_callback_list.cpp | 105 ------------------ 4 files changed, 64 insertions(+), 187 deletions(-) delete mode 100644 src/slic3r/plugin/PluginCallbackList.hpp delete mode 100644 tests/slic3rutils/test_plugin_callback_list.cpp diff --git a/src/slic3r/plugin/PluginCallbackList.hpp b/src/slic3r/plugin/PluginCallbackList.hpp deleted file mode 100644 index 4709881769..0000000000 --- a/src/slic3r/plugin/PluginCallbackList.hpp +++ /dev/null @@ -1,44 +0,0 @@ -#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/PluginManager.cpp b/src/slic3r/plugin/PluginManager.cpp index 6a306b9aec..32613602a1 100644 --- a/src/slic3r/plugin/PluginManager.cpp +++ b/src/slic3r/plugin/PluginManager.cpp @@ -1098,61 +1098,88 @@ std::vector PluginManager::copy_capability return it == m_capability_callbacks.end() ? std::vector{} : it->second; } -// The run_on_*_callbacks below are called from detached load/unload workers. They copy the -// subscriber list under m_mutex and invoke it OUTSIDE the lock: subscribers re-enter the manager, -// and no callback may run while the registry lock is held. +// The run_on_*_callbacks below are called from detached load/unload workers. They snapshot the +// subscriber list under m_mutex (the copy protects the in-flight loop from a callback that +// subscribes mid-dispatch, and serialises against a concurrent subscribe), then hand the snapshot +// to the UI thread: subscribers touch wx and must not run on a worker, and none may run while the +// registry lock is held. The dispatch runs inline when already on the UI thread, else via +// CallAfter, since calling wx off the main thread is UB. void PluginManager::run_on_load_callbacks(const std::string& plugin_key) { - for (auto& fn : copy_callbacks(CallbackType::Load)) { - try { - fn(plugin_key); - } catch (const std::exception& ex) { - BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key << ": " << ex.what(); - } catch (...) { - BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key; + auto work = [callbacks = copy_callbacks(CallbackType::Load), plugin_key] { + for (auto& fn : callbacks) { + try { + fn(plugin_key); + } catch (const std::exception& ex) { + BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key << ": " << ex.what(); + } catch (...) { + BOOST_LOG_TRIVIAL(error) << "Plugin load completion callback failed for " << plugin_key; + } } - } + }; + if (wxTheApp == nullptr || wxIsMainThread()) + work(); + else + GUI::wxGetApp().CallAfter(std::move(work)); } void PluginManager::run_on_unload_callbacks(const std::string& plugin_key) { - for (auto& fn : copy_callbacks(CallbackType::Unload)) { - try { - fn(plugin_key); - } catch (const std::exception& ex) { - BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": " << ex.what(); - } catch (...) { - BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": unknown error"; + auto work = [callbacks = copy_callbacks(CallbackType::Unload), plugin_key] { + for (auto& fn : callbacks) { + try { + fn(plugin_key); + } catch (const std::exception& ex) { + BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": " << ex.what(); + } catch (...) { + BOOST_LOG_TRIVIAL(error) << "Plugin unload completion callback failed for " << plugin_key << ": unknown error"; + } } - } + }; + if (wxTheApp == nullptr || wxIsMainThread()) + work(); + else + GUI::wxGetApp().CallAfter(std::move(work)); } void PluginManager::run_on_capability_load_callbacks(const PluginCapabilityId& id) { - for (auto& fn : copy_capability_callbacks(CallbackType::Load)) { - try { - fn(id); - } catch (const std::exception& ex) { - BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name << ": " - << ex.what(); - } catch (...) { - BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name; + auto work = [callbacks = copy_capability_callbacks(CallbackType::Load), id] { + for (auto& fn : callbacks) { + try { + fn(id); + } catch (const std::exception& ex) { + BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name << ": " + << ex.what(); + } catch (...) { + BOOST_LOG_TRIVIAL(error) << "Plugin capability load callback failed for " << id.plugin_key << "/" << id.name; + } } - } + }; + if (wxTheApp == nullptr || wxIsMainThread()) + work(); + else + GUI::wxGetApp().CallAfter(std::move(work)); } void PluginManager::run_on_capability_unload_callbacks(const PluginCapabilityId& id) { - for (auto& fn : copy_capability_callbacks(CallbackType::Unload)) { - try { - fn(id); - } catch (const std::exception& ex) { - BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name << ": " - << ex.what(); - } catch (...) { - BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name; + auto work = [callbacks = copy_capability_callbacks(CallbackType::Unload), id] { + for (auto& fn : callbacks) { + try { + fn(id); + } catch (const std::exception& ex) { + BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name << ": " + << ex.what(); + } catch (...) { + BOOST_LOG_TRIVIAL(error) << "Plugin capability unload callback failed for " << id.plugin_key << "/" << id.name; + } } - } + }; + if (wxTheApp == nullptr || wxIsMainThread()) + work(); + else + GUI::wxGetApp().CallAfter(std::move(work)); } // ── Cloud user ────────────────────────────────────────────────────────────────────────────── diff --git a/tests/slic3rutils/CMakeLists.txt b/tests/slic3rutils/CMakeLists.txt index 8b3630ff95..a055d9d937 100644 --- a/tests/slic3rutils/CMakeLists.txt +++ b/tests/slic3rutils/CMakeLists.txt @@ -3,7 +3,6 @@ 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_install.cpp test_plugin_lifecycle.cpp test_slicing_pipeline_bindings.cpp diff --git a/tests/slic3rutils/test_plugin_callback_list.cpp b/tests/slic3rutils/test_plugin_callback_list.cpp deleted file mode 100644 index 2d9c3678bf..0000000000 --- a/tests/slic3rutils/test_plugin_callback_list.cpp +++ /dev/null @@ -1,105 +0,0 @@ -#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); -}