mirror/OpenRGB
1
0
Fork 0
mirror of https://github.com/CalcProgrammer1/OpenRGB.git synced 2026-07-29 22:46:08 -04:00
Code Issues Packages Projects Releases Wiki Activity

RGBController: fix callback list race and dangling device-page callback on rescan

Browse Source
This commit is contained in:
Ken Sanislo
2026-07-23 02:59:30 +00:00
committed by Adam Honse
parent 8e8a9c8e05
commit fe53d90664
3 changed files with 136 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

101
RGBController/RGBController.cpp
View File

@@ -10,6 +10,8 @@
| SPDX-License-Identifier: GPL-2.0-or-later |
\*---------------------------------------------------------*/
#include <mutex>
#include <thread>
#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<std::mutex> 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<RGBControllerCallback> callbacks;
std::vector<void *> callback_args;
UpdateMutex.lock();
{
UpdateCallbacks[callback_idx](UpdateCallbackArgs[callback_idx], update_reason, this);
std::lock_guard<std::mutex> 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<std::mutex> 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<std::mutex> call_lock(SignalMutex);
SignalFrozen = true;
}
UpdateMutex.unlock();
std::unique_lock<std::mutex> wait_lock(SignalMutex);
SignalCallsDone.wait(wait_lock, [this]{ return SignalCalls == 0; });
}
void RGBController::UpdateLEDs()

31
RGBController/RGBController.h
View File

@@ -15,6 +15,7 @@
#include <atomic>
#include <thread>
#include <chrono>
#include <condition_variable>
#include <mutex>
#include <shared_mutex>
#include "RGBControllerInterface.h"
@@ -319,7 +320,37 @@ private:
std::vector<RGBControllerCallback> UpdateCallbacks;
std::vector<void *> 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 |

21
qt/OpenRGBDevicePage/OpenRGBDevicePage.cpp
View File

@@ -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 |