loadbalancer: make address assertion in LoadBalancerTest order-independent #3357

lawrencewang49 · 2025-10-30T08:53:39Z

Motivation

A flaky test was identified in LoadBalancerTest, specifically resubscribeToEventsWhenAllHostsAreUnhealthy, using NonDex.
The failure was caused by the use of assertAddresses(...), which relied on a specific order of elements in lb.usedAddresses().
Since the load balancer does not guarantee address ordering, this assertion was nondeterministic across runs.

Modifications

Replaced:

assertAddresses(lb.usedAddresses(), "address-2", "address-3", "address-4");

with:

assertThat(lb.usedAddresses(), containsInAnyOrder(
    hasProperty("key", is("address-2")),
    hasProperty("key", is("address-3")),
    hasProperty("key", is("address-4"))
));

to make the check independent of element order

Added missing imports for Collections, containsInAnyOrder, and hasProperty
Also, to resolve the flaky test further down caused by selection in p2c being non-deterministic, I changed the assertion from:

assertConnectionCount(lb.usedAddresses(), connectionsCount("address-2", 0),
                    connectionsCount("address-3", 1), connectionsCount("address-4", 1));

to

Map<String, Integer> connectionCounts = lb.usedAddresses().stream()
                .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().size()));

assertEquals(0, connectionCounts.getOrDefault("address-2", -1).intValue(), "address-2 count mismatch");
assertEquals(1, connectionCounts.getOrDefault("address-3", -1).intValue(), "address-3 count mismatch");
assertEquals(1, connectionCounts.getOrDefault("address-4", -1).intValue(), "address-4 count mismatch");

to always pass even if selection is non-deterministic, as it replaces the order-sensitive assertConnectionCount matcher with an explicit Map comparison

Result

The test now verifies the presence of expected addresses without depending on internal iteration order
Ensures consistent behavior across different JVM execution orders
No production or API changes, this is a test robustness improvement only

Steps to Reproduce:

Add the following to the top of build.gradle in the servicetalk-loadbalancer module:

plugins {
    id 'edu.illinois.nondex' version '2.1.7'
}

Add the following to the bottom of build.gradle in the servicetalk-loadbalancer module: apply plugin: 'edu.illinois.nondex'
Run ./gradlew :servicetalk-loadbalancer:nondexTest -DnondexRuns=10 in the terminal from the root repository. It should result in the output for a few of the runs below:

EagerNewRoundRobinLoadBalancerTest > resubscribeToEventsWhenAllHostsAreUnhealthy() FAILED
    java.lang.AssertionError: 
    Expected: iterable containing [hasProperty("key", is "address-2"), hasProperty("key", is "address-3"), hasProperty("key", is "address-4")]
         but: item 1:  property 'key' was "address-4"
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at io.servicetalk.loadbalancer.LoadBalancerTestScaffold.assertAddresses(LoadBalancerTestScaffold.java:184)
        at io.servicetalk.loadbalancer.LoadBalancerTest.resubscribeToEventsWhenAllHostsAreUnhealthy(LoadBalancerTest.java:704)

It should also result in the following output after the first assertion has been resolved:

EagerNewRoundRobinLoadBalancerTest > resubscribeToEventsWhenAllHostsAreUnhealthy() FAILED
    java.lang.AssertionError: 
    Expected: iterable containing [(hasProperty("key", is "address-2") and hasProperty("value", a collection with size <0>)), (hasProperty("key", is "address-3") and hasProperty("value", a collection with size <1>)), (hasProperty("key", is "address-4") and hasProperty("value", a collection with size <1>))]
         but: item 1: hasProperty("key", is "address-3")  property 'key' was "address-4"
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at io.servicetalk.loadbalancer.LoadBalancerTestScaffold.assertConnectionCount(LoadBalancerTestScaffold.java:173)
        at io.servicetalk.loadbalancer.LoadBalancerTest.resubscribeToEventsWhenAllHostsAreUnhealthy(LoadBalancerTest.java:726)

…ndent The test resubscribeToEventsWhenAllHostsAreUnhealthy previously relied on the iteration order of lb.usedAddresses(), which is not guaranteed. Updated the assertion to use containsInAnyOrder(...) with hasProperty(...) to make it deterministic across different execution orders. No functional or behavioral change; improves test robustness.

…ed (apple#3354) Motivation: When gRPC client receives a response, it first validates that it's formed according to gRPC specification. If any headers or trailers are missing, `validateResponseAndGetPayload` will throw an exception. In this case, we leak undrained response payload body. Modifications: 1. Try catch logic of `validateResponseAndGetPayload`, subscribe and cancel response message body in case of unexpected exceptions. 2. Enhance `ProtocolCompatibilityTest` to validate we never leak responses across all tests. Result: Responses are properly drained even when we receive malformed responses.

…ic for p2c

…ToEventsWhenAllHostsAreUnhealthy

bryce-anderson · 2025-11-03T19:01:23Z

servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/LoadBalancerTest.java


        // Events for the new Subscriber change the state
        sendServiceDiscoveryEvents(upEvent("address-2"), upEvent("address-3"), upEvent("address-4"));
-        assertAddresses(lb.usedAddresses(), "address-2", "address-3", "address-4");


In theory this will affect all uses of assertAddresses: can we fix assertAddresses directly?

I will investigate a way to fix assertAddress and assertConnectionCount directly!

hello @bryce-anderson, i have made changes to edit assertAddresses and assertConnectionCount! I have changed assertConnectionCount to use a containsInAnyOrder instead of a contains during the assertion, and used a list to iterate through the addresses for assertAddresses. the tests should now pass with both ./gradlew :servicetalk-loadbalancer:test --tests <qualified_test_path> and ./gradlew :servicetalk-loadbalancer:nondexTest --tests <qualified_test_path>

I removed ordering assumptions and avoided Hamcrest reflection matchers, comparing actual extracted keys instead. Because p2c didn't have deterministic selection, I used containsInAnyOrder to match this behavior

bryce-anderson · 2025-11-03T19:01:37Z

servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/LoadBalancerTest.java

        assertThat(selected1, is(anyOf(expected.values())));

        if (isRoundRobin()) {
-            // These asserts are flaky for p2c because we don't have deterministic selection.


This comment is still valuable.

bryce-anderson · 2025-11-03T19:02:15Z

servicetalk-loadbalancer/src/test/java/io/servicetalk/loadbalancer/LoadBalancerTest.java

-            // These asserts are flaky for p2c because we don't have deterministic selection.
            expected.remove(selected1);
            assertThat(lb.selectConnection(any(), null).toFuture().get().address(), is(anyOf(expected.values())));
-            assertConnectionCount(lb.usedAddresses(), connectionsCount("address-2", 0),


I think this is a similar problem with getConnectionCount: can we take the same approach as above and fix the assertion helper method?

…cally instead of editing test cases

lawrencewang49 · 2025-11-06T08:06:27Z

I also noticed that when running ./gradlew :servicetalk-loadbalancer:nondexTest on the whole module, the roundRobining() test in LoadBalancerTest.java fails intermittently when running tests in EagerP2CLoadBalancerTest.java and LingeringP2CLoadBalancerTest.java even though assumeTrue(isRoundRobin()) should skip it. I think this is because assumeTrue aborts the JUnit thread, but does not prevent async LB operations from already being scheduled. Under NonDex randomization, those background operations still run and can fail after the assumption triggers, producing a build-level failure instead of a skipped test.

Replacing the assumption by putting the test code inside of an if statement when roundRobining is true:

if (isRoundRobin()) {
    // assertion checks
}

ensures no reactive work is started, so the test is skipped deterministically and no nondeterministic async behavior leaks past the assumption. Are you open to accepting this change?

Running ./gradlew :servicetalk-loadbalancer:nondexTest --tests "io.servicetalk.loadbalancer.EagerP2CLoadBalancerTest.roundRobining" --stacktrace should result in output as shown below:

org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':servicetalk-loadbalancer:nondexTest'.
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.lambda$executeIfValid$1(ExecuteActionsTaskExecuter.java:130)
        at org.gradle.internal.Try$Failure.ifSuccessfulOrElse(Try.java:293)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeIfValid(ExecuteActionsTaskExecuter.java:128)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:116)
        at org.gradle.api.internal.tasks.execution.ProblemsTaskPathTrackingTaskExecuter.execute(ProblemsTaskPathTrackingTaskExecuter.java:41)
...

lawrencewang49 and others added 2 commits October 30, 2025 03:46

idelpivnitskiy assigned bryce-anderson Oct 31, 2025

idelpivnitskiy requested a review from bryce-anderson October 31, 2025 15:19

lawrencewang49 and others added 2 commits November 3, 2025 01:40

edit flaky assertions to pass even though selection isn't determinist…

f5f1972

…ic for p2c

Merge branch 'main' into fix-LingeringP2CLoadBalancerTest-resubscribe…

a4d4717

…ToEventsWhenAllHostsAreUnhealthy

bryce-anderson reviewed Nov 3, 2025

View reviewed changes

lawrencewang49 added 2 commits November 5, 2025 21:03

edit assertAddresses and assertConnectionCount to behave deterministi…

f3373a0

…cally instead of editing test cases

remove unused imports

75592a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

loadbalancer: make address assertion in LoadBalancerTest order-independent #3357

loadbalancer: make address assertion in LoadBalancerTest order-independent #3357

Uh oh!

lawrencewang49 commented Oct 30, 2025 •

edited

Loading

Uh oh!

bryce-anderson Nov 3, 2025

Uh oh!

lawrencewang49 Nov 4, 2025

Uh oh!

lawrencewang49 Nov 6, 2025 •

edited

Loading

Uh oh!

bryce-anderson Nov 3, 2025

Uh oh!

bryce-anderson Nov 3, 2025

Uh oh!

lawrencewang49 commented Nov 6, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

loadbalancer: make address assertion in LoadBalancerTest order-independent #3357

Are you sure you want to change the base?

loadbalancer: make address assertion in LoadBalancerTest order-independent #3357

Uh oh!

Conversation

lawrencewang49 commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Result

Uh oh!

bryce-anderson Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

lawrencewang49 Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

lawrencewang49 Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryce-anderson Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

bryce-anderson Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

lawrencewang49 commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lawrencewang49 commented Oct 30, 2025 •

edited

Loading

lawrencewang49 Nov 6, 2025 •

edited

Loading

lawrencewang49 commented Nov 6, 2025 •

edited

Loading