Repository navigation
Guard Controller::m_connection against concurrent access - #1827
Merged
Merged
Conversation
Signal-poller threads and the Auto Optimizer copy the IPC connection through Controller::GetConnection() while the JS thread can reset it in disconnect(). Reading and writing one std::shared_ptr from two threads is a data race: a reader can obtain a pointer to an ipc::client_win that is being freed and then crash locking its mutex in ipc::client_win::call (seen in Sentry as an access violation in mtx_do_lock). - Protect every access to m_connection with a mutex. - Bump the connection epoch before dropping the connection, so finalizers that check it skip IPC for the old session. - Release the last client reference outside the lock, because ~client_win joins the IPC worker thread. - Make the System::Shutdown call on a local copy, outside the lock, and skip it if there is no connection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sandboxcoder
force-pushed
the
rno/fix-ipc-mutex
branch
from
November 2, 2026 14:13
e3af024 to
e56ae03
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The missing IPC client implementation prevents verification of destructor/worker-thread teardown and reentrancy, while the crash fix still needs Windows disconnect/reconnect stress validation with active pollers.
No open issues 🎉
What changed in this PR
This PR addresses a race between background connection readers and JS-thread disconnects by synchronizing access to Controller::m_connection.
Changes:
- Adds a mutex to protect connection reads and writes.
- Advances the connection epoch before clearing the connection and releases the client outside the lock.
- Performs server shutdown through a local connection copy, with a null check and without holding the mutex.
| File | Description |
|---|---|
obs-studio-client/source/controller.hpp |
Declares the connection mutex and includes <mutex>. |
obs-studio-client/source/controller.cpp |
Synchronizes connection access and adjusts disconnect ordering and client release. |
Review effort: Balanced
Exercise the Controller::m_connection locking: the global source callback worker and an output's WorkerSignals worker keep calling GetConnection() while IPC.disconnect() drops the connection on the JS thread. - Run 8 host/disconnect cycles with both pollers active, varying the delay so the disconnect lands at different points of the poll cycle. - Keep an output poller running across a reconnect, so it picks up the new connection, then check the new session still answers IPC calls. - Add OBSHandler.reconnect(), which hosts a new server on a fresh pipe. Pollers are stopped without IPC after a disconnect: reassigning signalHandler joins the output worker, and RemoveSourceCallback joins the global worker, which exits on its own once the connection is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The stress tests can skip poller cleanup on failure, potentially hanging the runner or leaving workers active across attempts, so teardown must unconditionally stop recording and global pollers.
1 open issue
Review effort: Balanced
If recording setup, a signal wait or the reconnect check threw, the test skipped its poller cleanup, and obs.shutdown() does not stop these pollers. An output worker could keep polling and the global callback stayed registered, which could hang the runner or leak into a retry. Track each recording as soon as it is created and the global poller as soon as it is registered, and stop all of them in afterEach. Neither stop makes an IPC call, so cleanup works connected or disconnected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Missing IPC client code and unavailable native Windows disconnect/reconnect stress testing leave safe client destruction and deadlock-free worker joins with active pollers unverified.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
sandboxcoder
marked this pull request as ready for review
November 2, 2026 16:44
summeroff
approved these changes
Nov 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Signal-poller threads and the Auto Optimizer copy the IPC connection through Controller::GetConnection() while the JS thread can reset it in disconnect(). Reading and writing one std::shared_ptr from two threads is a data race: a reader can obtain a pointer to an ipc::client_win that is being freed and then crash locking its mutex in ipc::client_win::call (seen in Sentry as an access violation in mtx_do_lock).
Motivation and Context
Want to fix Sentry minidump to improve app stability.
How Has This Been Tested?
Types of changes
Checklist: