Connection: Add stop signals for connection failures and reconnections#36
Connection: Add stop signals for connection failures and reconnections#36danrossi wants to merge 3 commits into
Conversation
- add initial reconnection attempt feature. Needs to signal to keep trying to reconnect
WalkthroughThis PR introduces reconnection state tracking to the MoQOutput class. A new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/moq-output.cpp`:
- Line 90: Update the inline comment that currently reads "on swrver
disconnection. Signal to attempt to reconnect" to correct the typo so it reads
"on server disconnection. Signal to attempt to reconnect"; locate the comment
string (e.g., within the moq-output component where that disconnection/reconnect
logic is implemented) and replace "swrver" with "server" so the comment is clear
and accurate.
- Around line 86-88: The access to the boolean flag reconnecting is
inconsistent: replace the non-atomic read self->reconnecting with an atomic load
using os_atomic_load_bool(&self->reconnecting) so both read and write use the
same atomic API; update the check that currently reads self->reconnecting and
keep the atomic write via os_atomic_set_bool(&self->reconnecting, false)
unchanged to ensure thread-safe access to reconnecting.
- Line 149: In Stop() there’s an undefined identifier `self`; replace
`os_atomic_set_bool(&self->reconnecting, false);` with a proper member reference
such as `os_atomic_set_bool(&this->reconnecting, false);` (or `&reconnecting`)
so the class member `reconnecting` is accessed from Stop(); locate the Stop()
method and update that call (the lambda earlier that used `self` can remain
unchanged).
In `@src/moq-output.h`:
- Line 50: Remove the volatile qualifier from the member declaration
"reconnecting" so it is a plain bool; the code uses OBS atomic helpers like
os_atomic_set_bool/os_atomic_get_bool which expect a regular bool (or the type
they manage) and provide the necessary synchronization, so update the
declaration of reconnecting accordingly and ensure any other uses rely on
os_atomic_* functions rather than volatile semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 969ad5c6-15da-43d2-b2a3-fa3ebd8c37ae
📒 Files selected for processing (2)
src/moq-output.cppsrc/moq-output.h
|
|
||
| if (self->reconnecting) | ||
| os_atomic_set_bool(&self->reconnecting, false); |
There was a problem hiding this comment.
Inconsistent atomic access to reconnecting.
Line 87 reads self->reconnecting non-atomically (direct access), but line 88 writes it atomically using os_atomic_set_bool. For proper thread safety, both read and write operations should use atomic functions. Use os_atomic_load_bool(&self->reconnecting) for the read.
🔒 Proposed fix for consistent atomic access
- if (self->reconnecting)
- os_atomic_set_bool(&self->reconnecting, false);
+ if (os_atomic_load_bool(&self->reconnecting))
+ os_atomic_set_bool(&self->reconnecting, false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/moq-output.cpp` around lines 86 - 88, The access to the boolean flag
reconnecting is inconsistent: replace the non-atomic read self->reconnecting
with an atomic load using os_atomic_load_bool(&self->reconnecting) so both read
and write use the same atomic API; update the check that currently reads
self->reconnecting and keep the atomic write via
os_atomic_set_bool(&self->reconnecting, false) unchanged to ensure thread-safe
access to reconnecting.
| if (self->reconnecting) | ||
| os_atomic_set_bool(&self->reconnecting, false); | ||
| } else if (error_code == -2) { | ||
| //on swrver disconnection. Signal to attempt to reconnect |
There was a problem hiding this comment.
Fix typo in comment.
"swrver" should be "server".
📝 Proposed fix
- //on swrver disconnection. Signal to attempt to reconnect
+ //on server disconnection. Signal to attempt to reconnect📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //on swrver disconnection. Signal to attempt to reconnect | |
| //on server disconnection. Signal to attempt to reconnect |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/moq-output.cpp` at line 90, Update the inline comment that currently
reads "on swrver disconnection. Signal to attempt to reconnect" to correct the
typo so it reads "on server disconnection. Signal to attempt to reconnect";
locate the comment string (e.g., within the moq-output component where that
disconnection/reconnect logic is implemented) and replace "swrver" with "server"
so the comment is clear and accurate.
| obs_output_signal_stop(output, OBS_OUTPUT_SUCCESS); | ||
| } | ||
|
|
||
| os_atomic_set_bool(&self->reconnecting, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the code compiles and look for build errors
# Search for any similar patterns where self might be misused
rg -n '\bself->' src/moq-output.cppRepository: moq-dev/obs
Length of output: 925
🏁 Script executed:
# Read the code around the lambda callback (lines 78-100)
sed -n '75,105p' src/moq-output.cpp | cat -n
# Read the code around line 149
sed -n '140,160p' src/moq-output.cpp | cat -nRepository: moq-dev/obs
Length of output: 2299
Critical: Undefined variable self in Stop() method.
Line 149 uses &self->reconnecting, but self is not defined in this scope. The variable self only exists within the lambda callback (lines 78-103). In the Stop() method, use &this->reconnecting or &reconnecting instead.
This is a compilation error that will prevent the build from succeeding.
Proposed fix
- os_atomic_set_bool(&self->reconnecting, false);
+ os_atomic_set_bool(&reconnecting, false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os_atomic_set_bool(&self->reconnecting, false); | |
| os_atomic_set_bool(&reconnecting, false); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/moq-output.cpp` at line 149, In Stop() there’s an undefined identifier
`self`; replace `os_atomic_set_bool(&self->reconnecting, false);` with a proper
member reference such as `os_atomic_set_bool(&this->reconnecting, false);` (or
`&reconnecting`) so the class member `reconnecting` is accessed from Stop();
locate the Stop() method and update that call (the lambda earlier that used
`self` can remain unchanged).
| std::map<obs_encoder_t *, int> video_tracks; | ||
| std::map<obs_encoder_t *, int> audio_tracks; | ||
|
|
||
| volatile bool reconnecting; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check OBS threading utility signatures for atomic bool operations
# Search for os_atomic_set_bool and os_atomic_load_bool declarations
rg -n -A2 -B2 'os_atomic_(set|load)_bool' --type=c --type=cppRepository: moq-dev/obs
Length of output: 1220
🏁 Script executed:
# Find where os_atomic_set_bool is declared/defined
rg -n 'os_atomic_set_bool|define.*os_atomic_set_bool' --type=c --type=h --type=hppRepository: moq-dev/obs
Length of output: 83
🏁 Script executed:
# Also search in common OBS headers or utility files for atomic macros/functions
fd -e h -e hpp | xargs rg -l 'atomic' | head -20Repository: moq-dev/obs
Length of output: 37
🏁 Script executed:
# Check the actual declaration in moq-output.h around line 50
cat -n src/moq-output.h | sed -n '45,55p'Repository: moq-dev/obs
Length of output: 317
🏁 Script executed:
# Search for os_atomic_set_bool definition across header and source files (.h, .c)
rg -n 'os_atomic_set_bool' --type=c --type=h -B3 -A3Repository: moq-dev/obs
Length of output: 37
🏁 Script executed:
# Look for the definition in typical OBS locations or macros
rg -n 'define.*atomic|#define.*set_bool' --type=c --type=hRepository: moq-dev/obs
Length of output: 37
🏁 Script executed:
# Check what headers are included in moq-output.h to understand where os_atomic comes from
head -30 src/moq-output.hRepository: moq-dev/obs
Length of output: 591
🏁 Script executed:
# Search for all os_atomic usages in the codebase to see the pattern
rg -n 'os_atomic' --type=c --type=hRepository: moq-dev/obs
Length of output: 37
🏁 Script executed:
# Look for other volatile declarations in the codebase
rg -n 'volatile' --type=c --type=hRepository: moq-dev/obs
Length of output: 107
🏁 Script executed:
# Check for any compilation or type-related issues - look at build configuration
find . -name "CMakeLists.txt" -o -name "Makefile" -o -name "build.sh" | head -5Repository: moq-dev/obs
Length of output: 72
🏁 Script executed:
# Check CMakeLists.txt to understand build configuration and dependencies
head -50 ./CMakeLists.txtRepository: moq-dev/obs
Length of output: 1646
🏁 Script executed:
# Look at actual usage context in moq-output.cpp around the os_atomic_set_bool calls
sed -n '85,100p' src/moq-output.cppRepository: moq-dev/obs
Length of output: 740
🏁 Script executed:
# Check if there are any warnings or issues related to type mismatches
# Look for any casting or type conversions around reconnecting variable
rg -n 'reconnecting' --type=c --type=h -B1 -A1Repository: moq-dev/obs
Length of output: 151
Remove volatile qualifier from reconnecting member—it conflicts with atomic operation semantics.
The reconnecting member is declared as volatile bool but accessed using os_atomic_set_bool in the implementation. In C++, volatile does not provide atomicity or memory ordering guarantees—it only prevents compiler optimizations. When using atomic operations, the variable should be declared as a plain bool, allowing the OBS atomic functions to handle all necessary synchronization.
Proposed fix
- volatile bool reconnecting;
+ bool reconnecting;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| volatile bool reconnecting; | |
| bool reconnecting; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/moq-output.h` at line 50, Remove the volatile qualifier from the member
declaration "reconnecting" so it is a plain bool; the code uses OBS atomic
helpers like os_atomic_set_bool/os_atomic_get_bool which expect a regular bool
(or the type they manage) and provide the necessary synchronization, so update
the declaration of reconnecting accordingly and ensure any other uses rely on
os_atomic_* functions rather than volatile semantics.
1 participant
The OBS plugin was missing stop signals on connection failures to stop the broadcast and produce a dialog. This has been added including signalling for reconnection. It retries the connection once but another signal is required to continue reattempts. The flags is setup for that. I am not sure of the right signal to tell OBS to keep reconnecting on a server cut.