Skip to content

fix: race condition in watchable subscribe#6566

Merged
rudrakhp merged 1 commit intoenvoyproxy:mainfrom
rudrakhp:fix_subscribe_race
Jul 22, 2025
Merged

fix: race condition in watchable subscribe#6566
rudrakhp merged 1 commit intoenvoyproxy:mainfrom
rudrakhp:fix_subscribe_race

Conversation

@rudrakhp
Copy link
Copy Markdown
Member

What type of PR is this?

fix: race condition in watchable subscribe calls from main routine

What this PR does / why we need it:

To fix race conditions in Subscribe calls

Which issue(s) this PR fixes:

Fixes #5505
Related #6412

Release Notes: Yes

@rudrakhp rudrakhp requested a review from a team as a code owner July 21, 2025 11:16
@rudrakhp rudrakhp requested review from arkodg and removed request for arkodg July 21, 2025 11:16
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 21, 2025

Codecov Report

Attention: Patch coverage is 88.46154% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.79%. Comparing base (9b91cd1) to head (5fc53e6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/globalratelimit/runner/runner.go 28.57% 5 Missing ⚠️
internal/provider/kubernetes/status.go 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6566      +/-   ##
==========================================
+ Coverage   70.77%   70.79%   +0.02%     
==========================================
  Files         224      224              
  Lines       38614    38646      +32     
==========================================
+ Hits        27328    27359      +31     
- Misses       9695     9698       +3     
+ Partials     1591     1589       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we move this above r.watchResources ? so we can force ordering and make sure status are ready for subscriptions ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Don't see watchResources being called in offline controller, maybe move it to it's dedicated subscribeToResources?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved it to subscribeToResources.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jul 21, 2025

thanks @rudrakhp for reworking this into a simpler PR, appreciate it, added a minor comment

arkodg
arkodg previously approved these changes Jul 21, 2025
@arkodg arkodg requested review from a team July 21, 2025 20:38
@arkodg arkodg added this to the v1.5.0-rc.1 Release milestone Jul 21, 2025
zhaohuabing
zhaohuabing previously approved these changes Jul 22, 2025
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM. It would be helpful if a comment can be added to help later readers understand why subscribe() should be called from the main go routine.

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
@rudrakhp rudrakhp dismissed stale reviews from zhaohuabing and arkodg via 5fc53e6 July 22, 2025 05:02
@rudrakhp rudrakhp enabled auto-merge (squash) July 22, 2025 05:02
@rudrakhp
Copy link
Copy Markdown
Member Author

/retest

@rudrakhp rudrakhp merged commit e349987 into envoyproxy:main Jul 22, 2025
44 of 47 checks passed
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.

race condition between watcheabler.Map.Close and Subscribe

3 participants