fix: new subscribe pattern for snapshots to prevent race conditions#6412
fix: new subscribe pattern for snapshots to prevent race conditions#6412rudrakhp wants to merge 1 commit intoenvoyproxy:mainfrom rudrakhp:fix_subscribe_close_race
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6412 +/- ##
==========================================
+ Coverage 70.75% 70.83% +0.08%
==========================================
Files 220 221 +1
Lines 37757 37815 +58
==========================================
+ Hits 26715 26788 +73
+ Misses 9482 9467 -15
Partials 1560 1560 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest |
|
sorry i got caught up in lots of other things atm... hopefully i can give a quick review this week but cannot guarantee (Friday is also a us holiday) |
mathetake
left a comment
There was a problem hiding this comment.
I am struggling to understand why do you want to have a slice and keep having the reference to it.
No Subscribe() calls should exist outside subscription.go, enforcing strict control over subscription points.
At the end of the day, this is only thing we want to achieve right? Why do you want the list to keep track the channels as well as I am not sure how the fixed size on the slice helps.
|
my comments are mostly the question rather than blocking -- feel free to ignore them. I would appreciate you taking a shot at this bug anyways, thank you @rudrakhp ! |
|
@mathetake Thanks for the comments 🙌
We will need to fix the size of the list and know how many subscriptions we need to pre-subscribe. Dynamically increasing the list would basically mean not pre-subscribing that we were doing already today. I don't see major advantages of subscribing to snapshots in runtime as we should know required subscriptions as part of our code change/compile time.
Like you correctly mentioned we should never hit this path if we have correctly identified the required number of subscriptions in the controllers/runners. |
mathetake
left a comment
There was a problem hiding this comment.
i think what you explained made sense to me
|
/retest |
Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
|
hey @rudrakhp, if the goal is to move subscription to the main goroutine, can we instead move the logic into runner creation / gateway/internal/cmd/server.go Line 168 in df5a5e9 from runner.Start
|
|
Simpler fix in #6566 |
What type of PR is this?
fix: implement new subscribe pattern to prevent race condition with close
What this PR does / why we need it:
Propose a new pre-subscribe pattern for watchable Map snapshots to ensure that all subscriptions occur exclusively within the main routine. This guarantees that subscriptions are initialized in a single, centralized location and can be cleanly closed within the same context.
watchable.Mapor itsSubscribe()calls should exist outsidemessage/presubscribe.go, enforcing strict control over subscription points. This approach makes it easier to reason about subscription ownership and cleanup. Added a linter check for this.message/types.go. Update the count used for initializing each resource if more subscriptions are required after a code change.Which issue(s) this PR fixes:
Related #5505
Release Notes: Yes