Skip to content

Fix static loop counter in HybridABRManager and mutex unlock-before-r…#1338

Merged
pstroffolino merged 2 commits into
dev_sprint_25_2from
feature/VPAAMP-173
Apr 22, 2026
Merged

Fix static loop counter in HybridABRManager and mutex unlock-before-r…#1338
pstroffolino merged 2 commits into
dev_sprint_25_2from
feature/VPAAMP-173

Conversation

@benmesander
Copy link
Copy Markdown
Contributor

@benmesander benmesander commented Apr 21, 2026

  • Bug 1: Replace static int loop with per-instance mRampupFromSteadyStateLoop in HybridABRManager::CheckRampupFromSteadyState, matching the new ABR implementation.
  • Bug 2: Move lock.unlock() after all mProfiles reads in updateProfile() for both legacy ABRManager and new abr/abr.cpp to prevent races during period transitions.
  • Add unit tests for per-instance loop independence and updateProfile iframe selection.

@benmesander benmesander requested a review from a team as a code owner April 21, 2026 20:57
…ead in updateProfile

- Bug #1: Replace static int loop with per-instance mRampupFromSteadyStateLoop
  in HybridABRManager::CheckRampupFromSteadyState, matching the new ABR implementation.
- Bug #2: Move lock.unlock() after all mProfiles reads in updateProfile() for both
  legacy ABRManager and new abr/abr.cpp to prevent races during period transitions.
- Add unit tests for per-instance loop independence and updateProfile iframe selection.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes ABR state/race issues by making the steady-state ramp-up backoff counter instance-scoped in the Hybrid ABR path, and by ensuring updateProfile() keeps the profile mutex held until all mProfiles reads are complete (both legacy and new ABR implementations).

Changes:

  • Replace a function-static ramp-up loop counter with a per-instance member (mRampupFromSteadyStateLoop) in HybridABRManager::CheckRampupFromSteadyState.
  • Move lock.unlock() to after all mProfiles reads in ABRManager::updateProfile() in both support/aampabr/ABRManager.cpp and abr/abr.cpp.
  • Add unit tests for ramp-up loop independence and iframe profile selection behavior in updateProfile().

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/utests/tests/AampAbrTests/AbrTests.cpp Adds new unit tests for ramp-up loop independence and iframe profile selection in updateProfile().
support/aampabr/HybridABRManager.h Introduces per-instance mRampupFromSteadyStateLoop state for steady-state ramp-up.
support/aampabr/HybridABRManager.cpp Uses mRampupFromSteadyStateLoop instead of a function-static counter when computing mMaxBufferCountCheck.
support/aampabr/ABRManager.cpp Extends mutex hold in updateProfile() until after all mProfiles reads complete.
abr/abr.cpp Extends mutex hold in updateProfile() until after all mProfiles reads complete (new ABR path).

Comment thread test/utests/tests/AampAbrTests/AbrTests.cpp Outdated
Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
@pstroffolino pstroffolino merged commit 63e60ab into dev_sprint_25_2 Apr 22, 2026
6 checks passed
@pstroffolino pstroffolino deleted the feature/VPAAMP-173 branch April 22, 2026 14:06
Abhi-jith-S pushed a commit that referenced this pull request May 12, 2026
Reason for Change: Fix static loop counter in HybridABRManager and mutex unlock-before-read in updateProfile

- Bug #1: Replace static int loop with per-instance mRampupFromSteadyStateLoop
  in HybridABRManager::CheckRampupFromSteadyState, matching the new ABR implementation.
- Bug #2: Move lock.unlock() after all mProfiles reads in updateProfile() for both
  legacy ABRManager and new abr/abr.cpp to prevent races during period transitions.
- Add unit tests for per-instance loop independence and updateProfile iframe selection.

* Reason for Change: fixed test to cover intent

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>

---------

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
varatharajan568 pushed a commit that referenced this pull request May 20, 2026
Reason for Change: Fix static loop counter in HybridABRManager and mutex unlock-before-read in updateProfile

- Bug #1: Replace static int loop with per-instance mRampupFromSteadyStateLoop
  in HybridABRManager::CheckRampupFromSteadyState, matching the new ABR implementation.
- Bug #2: Move lock.unlock() after all mProfiles reads in updateProfile() for both
  legacy ABRManager and new abr/abr.cpp to prevent races during period transitions.
- Add unit tests for per-instance loop independence and updateProfile iframe selection.

* Reason for Change: fixed test to cover intent

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>

---------

Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants