diff --git a/RGBController/RGBController.cpp b/RGBController/RGBController.cpp index eebd4f14a..68ea1d65a 100644 --- a/RGBController/RGBController.cpp +++ b/RGBController/RGBController.cpp @@ -10,6 +10,8 @@ | SPDX-License-Identifier: GPL-2.0-or-later | \*---------------------------------------------------------*/ +#include +#include #include "LogManager.h" #include "RGBController.h" #include "StringUtils.h" @@ -68,6 +70,11 @@ using namespace std::chrono_literals; +/*---------------------------------------------------------*\ +| One counter per thread, shared across all controllers. | +\*---------------------------------------------------------*/ +thread_local unsigned int RGBController::SignalCallDepth = 0; + RGBController::RGBController() { /*-----------------------------------------------------*\ @@ -1834,6 +1841,23 @@ void RGBController::RegisterUpdateCallback(RGBControllerCallback new_callback, v UpdateMutex.unlock(); } +/*---------------------------------------------------------*\ +| Wait until no callback call is in flight, so the | +| caller may free the callback owner once this returns. | +| Skipped when the caller is itself inside a call on | +| this thread (see SignalCallDepth). | +\*---------------------------------------------------------*/ +void RGBController::WaitSignalCalls() +{ + if(SignalCallDepth != 0) + { + return; + } + + std::unique_lock wait_lock(SignalMutex); + SignalCallsDone.wait(wait_lock, [this]{ return SignalCalls == 0; }); +} + void RGBController::UnregisterUpdateCallback(void * callback_arg) { UpdateMutex.lock(); @@ -1846,6 +1870,12 @@ void RGBController::UnregisterUpdateCallback(void * callback_arg) } } UpdateMutex.unlock(); + + /*-----------------------------------------------------*\ + | Wait for in-flight calls so the caller may free the | + | callback owner once this returns | + \*-----------------------------------------------------*/ + WaitSignalCalls(); } void RGBController::ClearCallbacks() @@ -1854,19 +1884,64 @@ void RGBController::ClearCallbacks() UpdateCallbacks.clear(); UpdateCallbackArgs.clear(); UpdateMutex.unlock(); + + /*-----------------------------------------------------*\ + | Wait for in-flight calls so the caller may free the | + | callback owners once this returns | + \*-----------------------------------------------------*/ + WaitSignalCalls(); } void RGBController::SignalUpdate(unsigned int update_reason) { - SignalMutex.lock(); /*-----------------------------------------------------*\ - | Client info has changed, call the callbacks | + | Copy the callback list under UpdateMutex and mark a | + | call in flight, then release before invoking so no | + | lock is held across callback code. A frozen | + | controller (shutting down) calls nothing. | \*-----------------------------------------------------*/ - for(unsigned int callback_idx = 0; callback_idx < UpdateCallbacks.size(); callback_idx++) + std::vector callbacks; + std::vector callback_args; + + UpdateMutex.lock(); { - UpdateCallbacks[callback_idx](UpdateCallbackArgs[callback_idx], update_reason, this); + std::lock_guard call_lock(SignalMutex); + + if(SignalFrozen) + { + UpdateMutex.unlock(); + return; + } + + SignalCalls++; + } + callbacks = UpdateCallbacks; + callback_args = UpdateCallbackArgs; + UpdateMutex.unlock(); + + /*-----------------------------------------------------*\ + | Invoke the copied callbacks with no lock held | + \*-----------------------------------------------------*/ + SignalCallDepth++; + for(unsigned int callback_idx = 0; callback_idx < callbacks.size(); callback_idx++) + { + callbacks[callback_idx](callback_args[callback_idx], update_reason, this); + } + SignalCallDepth--; + + /*-----------------------------------------------------*\ + | Wake anyone waiting once the last call is out | + \*-----------------------------------------------------*/ + { + std::lock_guard call_lock(SignalMutex); + + SignalCalls--; + + if(SignalCalls == 0) + { + SignalCallsDone.notify_all(); + } } - SignalMutex.unlock(); } /*---------------------------------------------------------*\ @@ -1887,9 +1962,21 @@ void RGBController::Shutdown() AccessMutex.lock(); /*-----------------------------------------------------*\ - | Lock the signal mutex | + | Freeze the controller so no further SignalUpdate | + | calls its callbacks, then wait for any call already | + | in flight. Nothing is held across teardown, so a late | + | or cross-controller SignalUpdate returns at the | + | frozen check instead of blocking forever. | \*-----------------------------------------------------*/ - SignalMutex.lock(); + UpdateMutex.lock(); + { + std::lock_guard call_lock(SignalMutex); + SignalFrozen = true; + } + UpdateMutex.unlock(); + + std::unique_lock wait_lock(SignalMutex); + SignalCallsDone.wait(wait_lock, [this]{ return SignalCalls == 0; }); } void RGBController::UpdateLEDs() diff --git a/RGBController/RGBController.h b/RGBController/RGBController.h index f717f4730..bb939870c 100644 --- a/RGBController/RGBController.h +++ b/RGBController/RGBController.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include "RGBControllerInterface.h" @@ -319,7 +320,37 @@ private: std::vector UpdateCallbacks; std::vector UpdateCallbackArgs; + /*-----------------------------------------------------*\ + | Callback call state, guarded by SignalMutex. The | + | mutex is never held across callback code: a call | + | copies the callback list, releases, then invokes. | + | | + | SignalCalls : calls in flight for this controller. | + | SignalFrozen : set at Shutdown; no further calls run, | + | so a late SignalUpdate returns rather | + | than blocking on teardown. | + | | + | Unregister/ClearCallbacks/Shutdown remove or freeze | + | the list then wait out any call already running, so | + | on return a removed callback is not executing and | + | cannot start again, and its owner may be freed. | + \*-----------------------------------------------------*/ std::mutex SignalMutex; + std::condition_variable SignalCallsDone; + unsigned int SignalCalls = 0; + bool SignalFrozen = false; + + void WaitSignalCalls(); + + /*-----------------------------------------------------*\ + | Depth of SignalUpdate callback calls on this thread. | + | Per-thread and shared across all controllers, not | + | per-instance, so WaitSignalCalls can tell when a | + | callback is unregistering from inside its own call | + | and must not wait on itself. Not guarded by | + | SignalMutex; thread_local needs no lock. | + \*-----------------------------------------------------*/ + static thread_local unsigned int SignalCallDepth; /*-----------------------------------------------------*\ | Private path used internally by DetectionManager | diff --git a/qt/OpenRGBDevicePage/OpenRGBDevicePage.cpp b/qt/OpenRGBDevicePage/OpenRGBDevicePage.cpp index 53b34ed28..f9a22873e 100644 --- a/qt/OpenRGBDevicePage/OpenRGBDevicePage.cpp +++ b/qt/OpenRGBDevicePage/OpenRGBDevicePage.cpp @@ -100,17 +100,18 @@ OpenRGBDevicePage::OpenRGBDevicePage(RGBController *dev, QWidget *parent) : OpenRGBDevicePage::~OpenRGBDevicePage() { /*-----------------------------------------------------*\ - | Unregister update callback from the controller if the | - | controller still exists | + | Unregister update callback from the controller. The | + | controller always outlives its device page in every | + | teardown path (rescan, unplug, app exit), so the | + | pointer is valid here. The old list-membership guard | + | skipped this during rescan because the controller was | + | already removed from the list but not yet deleted, | + | leaving a dangling callback that a running effect | + | could invoke. UnregisterUpdateCallback waits for any | + | in-flight SignalUpdate before returning, so no call | + | can invoke this page after it is freed. | \*-----------------------------------------------------*/ - for(unsigned int controller_idx = 0; controller_idx < ResourceManager::get()->GetRGBControllers().size(); controller_idx++) - { - if(ResourceManager::get()->GetRGBControllers()[controller_idx] == device) - { - device->UnregisterUpdateCallback(this); - break; - } - } + device->UnregisterUpdateCallback(this); /*-----------------------------------------------------*\ | Unregister settings manager callbacks |