Skip to content

Commit c0c7018

Browse files
dkulpclaude
andcommitted
fix(overlays): never block on modelsLock from under a model's effectLock
The effect update threads can reach PixelOverlayModelSub::setState() while holding the model's effectLock (an effect stopping calls setState on its model), and an unresolved submodel parent then made foundParent() block on modelsLock via getModel(). Every HTTP and command path takes those locks the other way - modelsLock for the lookup, then effectLock to inspect or replace the running effect - so the two directions deadlock. Same shape as 37c8f92, one lock over, and worse: the effectLock side can run on the channel output thread via updateRunningEffects(), wedging output as well as the API. An unresolved parent is reachable at runtime because deleting a model resets every child's cached parent to null. Add PixelOverlayManager::tryGetModel(), a try_lock lookup, and use it for parent resolution. Paths that already hold modelsLock (all the manager and command paths) still resolve exactly as before - the recursive try_lock always succeeds. A contended effect-side resolve now returns unresolved and retries on the next setState instead of blocking, which leaves modelsLock -> effectLock as the only ordering in the subsystem, so the cycle can no longer close. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 8f5696d commit c0c7018

3 files changed

Lines changed: 37 additions & 9 deletions

File tree

src/overlays/PixelOverlay.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,14 @@ PixelOverlayModel* PixelOverlayManager::getModel(const std::string& name) {
388388
std::unique_lock<std::recursive_mutex> lock(modelsLock);
389389
return getModelLocked(name);
390390
}
391+
bool PixelOverlayManager::tryGetModel(const std::string& name, PixelOverlayModel*& model) {
392+
std::unique_lock<std::recursive_mutex> lock(modelsLock, std::try_to_lock);
393+
if (!lock.owns_lock()) {
394+
return false;
395+
}
396+
model = getModelLocked(name);
397+
return true;
398+
}
391399
PixelOverlayModel* PixelOverlayManager::getModelLocked(const std::string& name) {
392400
auto a = models.find(name);
393401
if (a != models.end()) {

src/overlays/PixelOverlay.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,15 @@ class PixelOverlayManager {
4040

4141
void addModel(Json::Value config);
4242
PixelOverlayModel* getModel(const std::string& name);
43+
// Non-blocking lookup for callers that may hold a model's effectLock (the
44+
// effect update threads). modelsLock -> effectLock is the sanctioned
45+
// order everywhere else; blocking on modelsLock from under an effectLock
46+
// is the reverse edge and deadlocks against any HTTP/command path. This
47+
// try-acquires modelsLock instead: returns false with model untouched if
48+
// the lock is contended (caller retries on a later call), true with the
49+
// lookup result (possibly nullptr for an unknown name) otherwise. A
50+
// thread already holding modelsLock always succeeds (recursive).
51+
bool tryGetModel(const std::string& name, PixelOverlayModel*& model);
4352

4453
void addModelListener(const std::string& name, const std::string& id, std::function<void(PixelOverlayModel*)> listener);
4554
void removeModelListener(const std::string& name, const std::string& id);

src/overlays/PixelOverlayModelSub.cpp

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -145,19 +145,30 @@ PixelOverlayModelSub::~PixelOverlayModelSub() {
145145

146146
// Resolve the parent model, caching the pointer.
147147
//
148-
// MUST NOT be called from the channel output thread. It takes modelsLock (via
149-
// getModel()), and the output thread reaches doOverlay() from
150-
// PixelOverlayManager::doOverlays() with activeModelsLock already held. Every
151-
// HTTP handler takes those two the other way round -- modelsLock for the model
152-
// lookup, then activeModelsLock when setState() reaches modelStateChanged() --
153-
// so acquiring them in this order deadlocks the two threads against each other.
154-
// setState() below is the only place that resolves, and it does so before the
155-
// model can become visible to the output thread at all.
148+
// MUST NOT be called from the channel output thread. The output thread reaches
149+
// doOverlay() from PixelOverlayManager::doOverlays() with activeModelsLock
150+
// already held, and every HTTP handler takes modelsLock before
151+
// activeModelsLock (via setState() -> modelStateChanged()), so a lookup from
152+
// that side would invert them. setState() below is the only caller, and the
153+
// resolution itself uses tryGetModel(): setState() can also run on the effect
154+
// update threads (an effect stopping calls model->setState() under this
155+
// model's effectLock), where BLOCKING on modelsLock is the reverse of the
156+
// HTTP/command paths' modelsLock -> effectLock order and deadlocks outright --
157+
// the same shape 37c8f92a8 fixed one lock over. The try-acquire never blocks:
158+
// from the manager paths (which already hold modelsLock recursively) it always
159+
// succeeds, and a contended effect-side resolve just stays unresolved until
160+
// the next setState().
156161
bool PixelOverlayModelSub::foundParent() {
157162
if (parent)
158163
return true;
159164

160-
parent = PixelOverlayManager::INSTANCE.getModel(config["Parent"].asString());
165+
PixelOverlayModel* p = nullptr;
166+
if (!PixelOverlayManager::INSTANCE.tryGetModel(config["Parent"].asString(), p)) {
167+
// modelsLock contended and not ours; do not log -- the parent may be
168+
// perfectly valid, we just can't look it up without risking deadlock.
169+
return false;
170+
}
171+
parent = p;
161172

162173
if (parent) {
163174
return true;

0 commit comments

Comments
 (0)