Fix a bug where all healthy endpoints are removed as HealthCheckedEndpointGroup is initialized

[ikhoon]

Motivation:

If new Endpoints are updated in HealthCheckedEndpointGroup, HealthCheckedEndpointGroup creates a new HealthCheckContextGroup. When the new HealthCheckContextGroup is initialized, the old HealthCheckContextGroup is removed for rolling updates.

The removal logic is added as a fallback to contextGroup.whenInitialized().

armeria/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java

Lines 175 to 187 in 3f54be0

	// Remove old contexts when the newly created contexts are fully initialized to smoothly transition
	// to new endpoints.
	contextGroup.whenInitialized().handle((unused, cause) -> {
	if (cause != null && !initialized) {
	if (logger.isWarnEnabled()) {
	logger.warn("The first health check failed for all endpoints. " +
	"numCandidates: {} candidates: {}",
	candidates.size(), truncate(candidates, 10), cause);
	}
	}
	initialized = true;
	destroyOldContexts(contextGroup);
	setEndpoints(allHealthyEndpoints());

Since context groups are stored in the order in which they were inserted, if the callback added first is executed first, the old value will always be deleted and the latest value will be maintained.

CompletableFuture uses a stack structure for callbacks, so the last callback will be executed first when it completes. If all contexts see the same future completion event, the old context group could remove the new context group. For example, D context group removes A to C context groups, and then C context group removes groups until it finds itself. But there is no C in the context group chain. As a result, C removes D, the newest and last one.

This situation will rarely occur when:

• An HealthCheckedEndpointGroup is not initialized yet.
• The delegate is updated with new endpoints that have the same value as the previous endpoints, but there are duplicate endpoints. The new endpoints create a new HealthCheckContextGroup that shares all HttpHealthCheckers with the previous one.

Modifications:

• Do not try to remove the old HealthCheckContextGroup with a context group if it was removed before.
• Fix a bug where the reference count of DefaultHealthCheckerContext is incorrectly counted if there are duplicate endpoints.
• Use the endpoints selected by HealthCheckStrategy instead of using the original endpoints updated by the delegate.

Result:

You no longer see EndpointSelectionTimeoutException when HealthCheckedEndpointGroup is initialized with duplicate Endpoints.

[ikhoon]

This change prevents the old contextGroup from deleting new contextGroups.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3f54be0) 73.94% compared to head (b001b46) 73.95%.

❗ Current head b001b46 differs from pull request most recent head a8106e1. Consider uploading reports for the commit a8106e1 to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #5343   +/-   ##
=========================================
  Coverage     73.94%   73.95%           
- Complexity    20104    20110    +6     
=========================================
  Files          1730     1730           
  Lines         74161    74165    +4     
  Branches       9465     9467    +2     
=========================================
+ Hits          54841    54849    +8     
+ Misses        14844    14840    -4     
  Partials       4476     4476           

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

[github-actions]

🔍 Build Scan® (commit: a8106e1)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/xygip5wib2iiu
build-self-hosted-jdk-8	✅	https://ge.armeria.dev/s/fkzndyeab6kvc
build-self-hosted-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/adyej2ylmmt3q
build-self-hosted-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/4mi3ene5yw3g6
build-self-hosted-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/oshorx7uv5dag
build-self-hosted-jdk-17-leak	✅	https://ge.armeria.dev/s/c3njufoj6opyk
build-self-hosted-jdk-11	✅	https://ge.armeria.dev/s/jxvhsnjponbts
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/thsshx2mema3k

[jrhee17]

CompletableFuture uses a stack structure for callbacks, so the last callback will be executed first when it completes.

TIL! The changes make sense. Thanks @ikhoon 🙇 👍 🙇

[minwoox]

Nice finding. Thanks, @ikhoon! 👍