-
Notifications
You must be signed in to change notification settings - Fork 942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix race condition when initializing HealthCheckedEndpointGroup #6188
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the test in question has failed again: https://github.com/line/armeria/actions/runs/14173631131/job/39703141791?pr=6188
...st/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java
Show resolved
Hide resolved
I noticed that the test failure in In this test, registering the endpoint is essentially part of the setup phase. However, the internal initialization of HealthCheckedEndpointGroup is asynchronous, and Initially, I thought calling Internally, To address this, I changed the approach: This ensures that the state propagation is explicitly controlled and prevents any premature callback from breaking the test setup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
endpointGroup.addListener(endpointsListener, false); | ||
endpointsListener.accept(endpointGroup.endpoints()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do this?
endpointGroup.addListener(endpointsListener, false); | |
endpointsListener.accept(endpointGroup.endpoints()); | |
endpointGroup.addListener(endpointsListener, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works ! I think Fix duplicate endpoint handling in HealthCheckedEndpointGroup
this change helps the new group include endpoints from the previous group temporarily, which avoids any brief disappearance during the transition.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6188 +/- ##
============================================
+ Coverage 74.46% 74.53% +0.06%
- Complexity 22234 22364 +130
============================================
Files 1963 1969 +6
Lines 82437 82767 +330
Branches 10764 10777 +13
============================================
+ Hits 61385 61688 +303
- Misses 15918 15949 +31
+ Partials 5134 5130 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Motivation
Fixes #6018
A race condition can occur when adding a listener immediately after creating a HealthCheckedEndpointGroup.
Because the endpoint group initialization is asynchronous, the listener may receive endpoints in an incomplete or intermediate state.
Modification
Explicitly await the completion of HealthCheckedEndpointGroup initialization using whenReady().join() before adding a listener.
This ensures the listener will always receive a fully initialized endpoint list, eliminating any race conditions.
Result
• Race condition is fixed.
• Listener always receives a fully initialized EndpointGroup, ensuring reliable and predictable behavior.