-
Notifications
You must be signed in to change notification settings - Fork 942
Fix race condition when initializing HealthCheckedEndpointGroup #6188
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
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.57% +0.11%
- Complexity 22234 22460 +226
============================================
Files 1963 1972 +9
Lines 82437 82992 +555
Branches 10764 10798 +34
============================================
+ Hits 61385 61894 +509
- Misses 15918 15951 +33
- Partials 5134 5147 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
assertThat(endpointGroup.endpoints().get(0).attrs().attr(EndpointAttributeKeys.DEGRADED_ATTR)) | ||
.isFalse(); | ||
} | ||
final HealthCheckedEndpointGroup endpointGroup = new HealthCheckedEndpointGroup( |
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.
Question) Shouldn't the HealthCheckedEndpointGroup
still be closed to close the ScheduledFuture
at L539?
@@ -59,6 +59,12 @@ import { TRANSPORTS } from '../../lib/transports'; | |||
import { SelectOption } from '../../lib/types'; | |||
import DebugInputs from './DebugInputs'; | |||
|
|||
const stringifyHeaders = (headers: [string, string[]][]): string => |
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 you pushed the changes of #6191 to this PR.
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.
Reapplied correct changes after reverting accidental commits (rollback to 018a7b1).
Thanks for the notice!
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.