Stop updating endpoints when HealthCheckedEndpointGroup is closing

[ikhoon]

Motivation:

When HealthCheckedEndpointGroup is closing, the internally managed endpoints are cleared.

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

Line 135 in 011741d

onUpdateHealth.accept(originalEndpoint.withAttrs(endpointAttributes), false);

In fact, the closing process is not a problem. However, when the endpoints change, the change is propagated to EndpointSelector. Some EndpointSelector implementations warn about the empty endpoints. The warning is not a real problem but a false positive alarm.

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

Line 81 in 62da203

logger.warn("No valid endpoint with weight > 0. endpoints: {}", toShortString(endpoints));

For better compatibility with EndpointSelector implementations, I propose to stop additional endpoints from being updated when HealthCheckedEndpointGroup is closed. As a result, EndpointSelector will not updated with empty endpoints.

Modifications:

• Stop updating endpoints when HealthCheckedEndpointGroup is closing

Result:

You no loger see a false positive warning log when HealthCheckedEndpointGroup is closing.

[github-actions]

🔍 Build Scan® (commit: 5fd2e9f)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/kq7nvnjq6le6y
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/w33vg6m4xcack
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/bdrojohavgdoc
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/azdnfqdt2odxm
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/l2dr4pmpbynnw
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/gjlm7u4lt2r2a
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/iywezo34bn2go

[minwoox]

What do you think of not calling the updateHealth at all from the DefaultHealthCheckerContext?

private DefaultHealthCheckerContext newCheckerContext(Endpoint endpoint) {
    return new DefaultHealthCheckerContext(endpoint, port, protocol, clientOptions, retryBackoff,
                                           this::updateHealth, this);
}

DefaultHealthCheckerContext(Endpoint endpoint, int port, SessionProtocol protocol,
                            ClientOptions clientOptions, Backoff retryBackoff,
                            BiConsumer<Endpoint, Boolean> onUpdateHealth,
                            ListenableAsyncCloseable listenableAsyncCloseable) {
  ...
}

// Use listenableAsyncCloseable.isClosing( ) to check if the HealthCheckedEndpointGroup is closing or not.

[minwoox]

I realized that there's no heavy logic in the DefaultHealthCheckerContext.
So probably just check isClosing() in the updateHealth() method would be enough.

[ikhoon]

updateHealth() is not the only method that calls setEndpoint(). If a context is lazily initialized after HealthCheckedEndpointGroup is closed, empty endpoints will be set to the delegate. That situation will not happen normally though.

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

Line 197 in d7a4d29

setEndpoints(allHealthyEndpoints());

[minwoox]

What I meant was adding the logic that checks isClosing() instead of replacing the current location.
There is a logic that iterates the contextGroupChain while holding a lock in updateHealth() method and I thought we don't have to do this when isClosing() is true.

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

Line 250 in 62da203

private DefaultHealthCheckerContext findContext(Endpoint endpoint) {

But, if it's a relatively light job you think, I'm fine with the current logic. 👍

[ikhoon]

Understood, fixed.