Fix a race condition in CompositeEndpointGroup

[minwoox]

Motivation:
CompositeEndpointGroup has a race condition that can occur when multiple child EndpointGroups notify listeners at the same time from different threads. Consider the following sequence of events involving two groups, A and B:

1. A's listener is invoked by thread A.

   armeria/core/src/main/java/com/linecorp/armeria/client/endpoint/CompositeEndpointGroup.java

   Line 64 in 2367de2

   endpointGroup.addListener(unused -> {

2. Thread A sets dirty = true.

   armeria/core/src/main/java/com/linecorp/armeria/client/endpoint/CompositeEndpointGroup.java

   Line 65 in 2367de2

   dirty.set(true);

3. Thread A sets dirty = false.

   armeria/core/src/main/java/com/linecorp/armeria/client/endpoint/CompositeEndpointGroup.java

   Line 89 in 2367de2

   if (!dirty.compareAndSet(true, false)) {

4. Thread A builds newEndpointsA.

   armeria/core/src/main/java/com/linecorp/armeria/client/endpoint/CompositeEndpointGroup.java

   Lines 95 to 98 in 2367de2

   	final ImmutableList.Builder<Endpoint> newEndpoints = ImmutableList.builder();
   	for (EndpointGroup endpointGroup : endpointGroups) {
   	newEndpoints.addAll(endpointGroup.endpoints());
   	}

5. B's listener is invoked by thread B.
6. Thread B sets dirty = true.
7. Thread B sets dirty = false.
8. Thread B builds newEndpointsB (which includes latest data from both A and B) and sets it.
9. Thread A sets the newEndpointsA, which does not contain updates from B.

   armeria/core/src/main/java/com/linecorp/armeria/client/endpoint/CompositeEndpointGroup.java

   Line 100 in 2367de2

   return merged = newEndpoints.build();

This results in stale endpoints being set, despite a newer state already being computed and applied.

Modifications:

• Fixed the race condition in CompositeEndpointGroup.

Result:

• CompositeEndpointGroup now handles concurrent updates correctly.

Motivation:

Explain why you're making this change and what problem you're trying to solve.

Modifications:

• List the modifications you've made in detail.

Result:

• Closes #. (If this resolves the issue.)
• Describe the consequences that a user will face after this PR is merged.

[github-actions]

🔍 Build Scan® (commit: cad8e44)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/vrn6mggopzh5g
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/ecwbxh5tlfqu6
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	❌ (failure)	https://ge.armeria.dev/s/a37owx3xzczgk
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/rdahno2i6cvsc
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/jhe4tv5jakqtw
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/dmpyldlx6kiyc
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/p4hvrql2ilmc2

[jrhee17]

The current implementation looks good in terms of correctness.

Alternatively, I think a lock based implementation could help avoid comparing endpoint lists which might be large.

[minwoox]

a lock based implementation

I wanted to avoid using a lock since we don't know what's inside in endpointGroup.endpoints().

avoid comparing endpoint lists which might be large.

CAS uses the references when comparing, so it's no big deal. 😉

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (8150425) to head (76f7d4a).
Report is 68 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #6220       +/-   ##
============================================
- Coverage     74.46%       0   -74.47%     
============================================
  Files          1963       0     -1963     
  Lines         82437       0    -82437     
  Branches      10764       0    -10764     
============================================
- Hits          61385       0    -61385     
+ Misses        15918       0    -15918     
+ Partials       5134       0     -5134     

☔ 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.

[ikhoon]

Because ImmutableList.of() returns a singleton instance, rebuildEndpoints may update outdated endpoints in rare cases.

1. Thread A updates endpointA.
   • Thread A prepares to set list(endpointA) in L93
2. Thread B removes endpointA
   • Thread B sets empty -> empty in L94
3. Thread A sets empty -> list(endpointA) in L94
4. merged is list(endpointA) but the actual endpoints in endpointGroups are empty.

[minwoox]

Thanks for the explanation! Updated. 😉

[ikhoon]

Thanks! 👍 👍