diff --git a/RGBController/RGBController.cpp b/RGBController/RGBController.cpp index 68ea1d65a..d20c963d0 100644 --- a/RGBController/RGBController.cpp +++ b/RGBController/RGBController.cpp @@ -70,11 +70,6 @@ using namespace std::chrono_literals; -/*---------------------------------------------------------*\ -| One counter per thread, shared across all controllers. | -\*---------------------------------------------------------*/ -thread_local unsigned int RGBController::SignalCallDepth = 0; - RGBController::RGBController() { /*-----------------------------------------------------*\ @@ -90,6 +85,9 @@ RGBController::RGBController() flags = 0; type = DEVICE_TYPE_UNKNOWN; + SignalCalls = 0; + SignalShutdown = false; + /*-----------------------------------------------------*\ | Initialize device thread | \*-----------------------------------------------------*/ @@ -1841,27 +1839,10 @@ 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(); - for(unsigned int callback_idx = 0; callback_idx < UpdateCallbackArgs.size(); callback_idx++ ) + for(unsigned int callback_idx = 0; callback_idx < UpdateCallbackArgs.size(); callback_idx++) { if(UpdateCallbackArgs[callback_idx] == callback_arg) { @@ -1872,10 +1853,21 @@ void RGBController::UnregisterUpdateCallback(void * callback_arg) UpdateMutex.unlock(); /*-----------------------------------------------------*\ - | Wait for in-flight calls so the caller may free the | - | callback owner once this returns | + | If this is executing from within any controller's | + | callback, return immediately rather than waiting for | + | callback completion to avoid deadlock. | \*-----------------------------------------------------*/ - WaitSignalCalls(); + if(SignalCalls != 0) + { + return; + } + + /*-----------------------------------------------------*\ + | Otherwise, wait for all currently active callbacks to | + | complete before returning | + \*-----------------------------------------------------*/ + std::unique_lock wait_lock(SignalMutex); + SignalCallsDone.wait(wait_lock, [this]{ return SignalCalls == 0; }); } void RGBController::ClearCallbacks() @@ -1886,62 +1878,81 @@ void RGBController::ClearCallbacks() UpdateMutex.unlock(); /*-----------------------------------------------------*\ - | Wait for in-flight calls so the caller may free the | - | callback owners once this returns | + | If this is executing from within any controller's | + | callback, return immediately rather than waiting for | + | callback completion to avoid deadlock. | \*-----------------------------------------------------*/ - WaitSignalCalls(); + if(SignalCalls != 0) + { + return; + } + + /*-----------------------------------------------------*\ + | Otherwise, wait for all currently active callbacks to | + | complete before returning | + \*-----------------------------------------------------*/ + std::unique_lock wait_lock(SignalMutex); + SignalCallsDone.wait(wait_lock, [this]{ return SignalCalls == 0; }); } void RGBController::SignalUpdate(unsigned int update_reason) { - /*-----------------------------------------------------*\ - | 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. | - \*-----------------------------------------------------*/ std::vector callbacks; std::vector callback_args; + /*-----------------------------------------------------*\ + | Lock the update mutex while obtaining a copy of the | + | callbacks, but release it before actually calling | + | them so that any call to Register/Unregister from | + | within the callback doesn't deadlock. | + \*-----------------------------------------------------*/ UpdateMutex.lock(); + + /*-----------------------------------------------------*\ + | Lock the signal mutex and increment the signal call | + | count. Return immediately if the controller has been | + | shut down. | + \*-----------------------------------------------------*/ + SignalMutex.lock(); + if(SignalShutdown) { - std::lock_guard call_lock(SignalMutex); - - if(SignalFrozen) - { - UpdateMutex.unlock(); - return; - } - - SignalCalls++; + UpdateMutex.unlock(); + return; } + SignalCalls++; + SignalMutex.unlock(); + + /*-----------------------------------------------------*\ + | Copy the list of callbacks and unlock the update | + | mutex | + \*-----------------------------------------------------*/ callbacks = UpdateCallbacks; callback_args = UpdateCallbackArgs; + UpdateMutex.unlock(); /*-----------------------------------------------------*\ - | Invoke the copied callbacks with no lock held | + | Invoke the copied callbacks | \*-----------------------------------------------------*/ - 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 | + | Decrement the signal call count once callbacks have | + | been called. Notify anyone waiting on signal calls | + | to be done. | \*-----------------------------------------------------*/ + SignalMutex.lock(); + SignalCalls--; + + if(SignalCalls == 0) { - std::lock_guard call_lock(SignalMutex); - - SignalCalls--; - - if(SignalCalls == 0) - { - SignalCallsDone.notify_all(); - } + SignalCallsDone.notify_all(); } + + SignalMutex.unlock(); } /*---------------------------------------------------------*\ @@ -1962,19 +1973,17 @@ void RGBController::Shutdown() AccessMutex.lock(); /*-----------------------------------------------------*\ - | 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. | + | Shut down signal handling and unlock mutexes | \*-----------------------------------------------------*/ UpdateMutex.lock(); - { - std::lock_guard call_lock(SignalMutex); - SignalFrozen = true; - } + SignalMutex.lock(); + SignalShutdown = true; + SignalMutex.unlock(); UpdateMutex.unlock(); + /*-----------------------------------------------------*\ + | Wait for any remaining signal calls to complete | + \*-----------------------------------------------------*/ std::unique_lock wait_lock(SignalMutex); SignalCallsDone.wait(wait_lock, [this]{ return SignalCalls == 0; }); } diff --git a/RGBController/RGBController.h b/RGBController/RGBController.h index bb939870c..24da2cecc 100644 --- a/RGBController/RGBController.h +++ b/RGBController/RGBController.h @@ -320,37 +320,10 @@ 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; + unsigned int SignalCalls; + bool SignalShutdown; /*-----------------------------------------------------*\ | Private path used internally by DetectionManager |