core: Fix pick_first NPE when accepting resolved addresses and in `CONNECTING` state by schiemon · Pull Request #12814 · grpc/grpc-java

schiemon · 2026-05-19T12:36:09Z

This PR resolves #12796.

It makes sure that whenever PickFirstLeafLoadBalancer transitions into CONNECTING the current address in the addressIndex has a corresponding subchannel. This prevents an NPE when hitting

grpc-java/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java

Lines 143 to 154 in 86fa860

    
           if (rawConnectivityState == READY 
        
               || (rawConnectivityState == CONNECTING 
        
                 && (!enableHappyEyeballs || addressIndex.isValid()))) { 
        
             // If the previous ready (or connecting) subchannel exists in new address list, 
        
             // keep this connection and don't create new subchannels. Happy Eyeballs is excluded when 
        
             // connecting, because it allows multiple attempts simultaneously, thus is fine to start at 
        
             // the beginning. 
        
             SocketAddress previousAddress = addressIndex.getCurrentAddress(); 
        
             addressIndex.updateGroups(newImmutableAddressGroups); 
        
             if (addressIndex.seekTo(previousAddress)) { 
        
               SubchannelData subchannelData = subchannels.get(previousAddress); 
        
               subchannelData.getSubchannel().updateAddresses(addressIndex.getCurrentEagAsList());

in acceptResolvedAddresses in some situations.

Note that for READY, a state from which this path can also be entered, this is already guaranteed.

…NECTING state

linux-foundation-easycla · 2026-05-19T12:36:17Z

The committers listed above are authorized under a signed CLA.

✅ login: schiemon / name: szymon.habrainski (c76956a)

schiemon · 2026-05-19T12:59:20Z

    // addresses in the updated list.
    subchannelData.updateState(newState);
    if (rawConnectivityState == TRANSIENT_FAILURE || concludedState == TRANSIENT_FAILURE)  {
      if (newState == CONNECTING) {


If concludedState == TRANSIENT_FAILURE but rawConnectivityState != TRANSIENT_FAILURE, shouldn't we adapt our rawConnectivityState?

Without diving deep into it, I'd say since it is not related to the fix for the NPE, a new issue or discussion can be started with your concern.

schiemon · 2026-05-19T13:03:32Z


    if (rawConnectivityState == READY
        || (rawConnectivityState == CONNECTING
          && (!enableHappyEyeballs || addressIndex.isValid()))) {


Why not

Suggested change

&& addressIndex.isValid())) {

? If we !enableHappyEyeballs and !addressIndex.isValid() then we get an exception when calling addressIndex.getCurrentAddress().

After the fix, !enableHappyEyeballs && !addressIndex.isValid() can never evaluate to true because processSubchannelState does a seek and updates the address index.

schiemon · 2026-05-19T13:20:52Z

      addressIndex.updateGroups(newImmutableAddressGroups);
      if (addressIndex.seekTo(previousAddress)) {
        SubchannelData subchannelData = subchannels.get(previousAddress);
        subchannelData.getSubchannel().updateAddresses(addressIndex.getCurrentEagAsList());


With HE we have potentionally multiple connecting subchannels: why aren't we updating them all?

Is part of a short-circuit optimization. It says: "I have found the one address I was
focusing on. I'll update it and stop looking."

If you had multiple subchannels connecting and wanted to update them all, you would effectively be implementing a "Parallel Load Balancer" rather than a "Pick First" balancer. In PickFirst, the moment we can confirm our "focus" is still valid, we stop processing the update and get back to work.

kannanjgithub · 2026-05-22T06:14:13Z


    if (rawConnectivityState == READY
        || (rawConnectivityState == CONNECTING
          && (!enableHappyEyeballs || addressIndex.isValid()))) {


After the fix, !enableHappyEyeballs && !addressIndex.isValid() can never evaluate to true because processSubchannelState does a seek and updates the address index.

kannanjgithub · 2026-05-22T06:17:38Z

      addressIndex.updateGroups(newImmutableAddressGroups);
      if (addressIndex.seekTo(previousAddress)) {
        SubchannelData subchannelData = subchannels.get(previousAddress);
        subchannelData.getSubchannel().updateAddresses(addressIndex.getCurrentEagAsList());


Is part of a short-circuit optimization. It says: "I have found the one address I was
focusing on. I'll update it and stop looking."

If you had multiple subchannels connecting and wanted to update them all, you would effectively be implementing a "Parallel Load Balancer" rather than a "Pick First" balancer. In PickFirst, the moment we can confirm our "focus" is still valid, we stop processing the update and get back to work.

kannanjgithub · 2026-05-22T06:21:30Z

    // addresses in the updated list.
    subchannelData.updateState(newState);
    if (rawConnectivityState == TRANSIENT_FAILURE || concludedState == TRANSIENT_FAILURE)  {
      if (newState == CONNECTING) {


Without diving deep into it, I'd say since it is not related to the fix for the NPE, a new issue or discussion can be started with your concern.

kannanjgithub · 2026-05-22T06:22:16Z

/gcbrun

Copilot

Pull request overview

This PR fixes a NullPointerException in PickFirstLeafLoadBalancer.acceptResolvedAddresses() that can occur when the balancer is in CONNECTING and the addressIndex points at an address that doesn’t yet have a corresponding entry in subchannels (reported in #12796). The fix ensures that when transitioning into CONNECTING, the addressIndex is adjusted to point at an address backed by an existing subchannel, preventing subchannels.get(previousAddress) from returning null during address updates.

Changes:

Adjust processSubchannelState(CONNECTING) to realign addressIndex when its current address lacks a subchannel (or the index is invalid in non-Happy-Eyeballs mode).
Add a regression test that reproduces the state inconsistency leading to the NPE (health-check + transient failure scenario).
Expose rawConnectivityState via a @VisibleForTesting accessor to support the new test assertions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java	Ensures `addressIndex` points to an address with a subchannel when entering `CONNECTING`, preventing NPEs in `acceptResolvedAddresses`; adds a testing accessor.
core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java	Adds a regression test reproducing #12796 by driving the LB through the problematic CONNECTING/address-update sequence.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        // If we get a newly resolved address list via acceptResolvedAddresses,
+        // as we are in CONNECTING, we will try to .updateAddresses the currently
+        // connecting subchannel if it exists in the new list.
+        // As such, We need to make sure that with transitioning to CONNECTING the subchannel for
+        // the current address of a valid index exists.


+            || (addressIndex.isValid() && !subchannels.containsKey(
+            addressIndex.getCurrentAddress()))) {



+  // reproduces #12796
+  @Test
+  public void healthCheckWithTF_AllowsStateInconsistency() {


+    // HealthListener.onSubchannelState gets called. It updates the LBs balancing
+    // state/concludedState.
+    assertEquals(TRANSIENT_FAILURE, loadBalancer.getConcludedConnectivityState());


+    // Given the new list, it is now pointing to server 2 which does not have a subchannel.
+    assertEquals(0, loadBalancer.getIndexLocation());
+
+    // Subchannel 1 is still in TRANSIENT_FAILURE state. Is backoff expires,


core: Fix pick_first NPE when accepting resolved addresses and in CON…

c76956a

…NECTING state

schiemon commented May 19, 2026

View reviewed changes

schiemon mentioned this pull request May 19, 2026

NPE in PickFirstLeafLoadBalancer #12796

Closed

schiemon commented May 19, 2026

View reviewed changes

schiemon marked this pull request as ready for review May 19, 2026 14:03

kannanjgithub approved these changes May 22, 2026

View reviewed changes

kannanjgithub requested a review from Copilot May 22, 2026 06:22

Copilot started reviewing on behalf of kannanjgithub May 22, 2026 06:23 View session

Copilot AI reviewed May 22, 2026

View reviewed changes

kannanjgithub added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 22, 2026

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 22, 2026

kannanjgithub merged commit a6916c9 into grpc:master May 22, 2026
15 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Fix pick_first NPE when accepting resolved addresses and in `CONNECTING` state#12814

core: Fix pick_first NPE when accepting resolved addresses and in `CONNECTING` state#12814
kannanjgithub merged 1 commit into
grpc:masterfrom
schiemon:fix/pick-first-leaf-npe-12796

schiemon commented May 19, 2026 •

edited

Loading

Uh oh!

linux-foundation-easycla Bot commented May 19, 2026 •

edited

Loading

Uh oh!

schiemon May 19, 2026 •

edited

Loading

Uh oh!

kannanjgithub May 22, 2026

Uh oh!

schiemon May 19, 2026

Uh oh!

kannanjgithub May 22, 2026

Uh oh!

schiemon May 19, 2026

Uh oh!

kannanjgithub May 22, 2026

Uh oh!

kannanjgithub May 22, 2026

Uh oh!

kannanjgithub May 22, 2026

Uh oh!

kannanjgithub May 22, 2026

Uh oh!

kannanjgithub commented May 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (rawConnectivityState == READY
	\|\| (rawConnectivityState == CONNECTING
	&& (!enableHappyEyeballs \|\| addressIndex.isValid()))) {
	// If the previous ready (or connecting) subchannel exists in new address list,
	// keep this connection and don't create new subchannels. Happy Eyeballs is excluded when
	// connecting, because it allows multiple attempts simultaneously, thus is fine to start at
	// the beginning.
	SocketAddress previousAddress = addressIndex.getCurrentAddress();
	addressIndex.updateGroups(newImmutableAddressGroups);
	if (addressIndex.seekTo(previousAddress)) {
	SubchannelData subchannelData = subchannels.get(previousAddress);
	subchannelData.getSubchannel().updateAddresses(addressIndex.getCurrentEagAsList());

		\|\| (addressIndex.isValid() && !subchannels.containsKey(
		addressIndex.getCurrentAddress()))) {

Conversation

schiemon commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schiemon May 19, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kannanjgithub commented May 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

schiemon commented May 19, 2026 •

edited

Loading

linux-foundation-easycla Bot commented May 19, 2026 •

edited

Loading

schiemon May 19, 2026 •

edited

Loading