Skip to content

fix: return EmptyResponse instead of null on exception in NacosLoadBalancer#4288

Merged
uuuyuqi merged 2 commits intoalibaba:2025.1.xfrom
daguimu:fix/nacos-loadbalancer-null-return
Mar 31, 2026
Merged

fix: return EmptyResponse instead of null on exception in NacosLoadBalancer#4288
uuuyuqi merged 2 commits intoalibaba:2025.1.xfrom
daguimu:fix/nacos-loadbalancer-null-return

Conversation

@daguimu
Copy link
Copy Markdown
Contributor

@daguimu daguimu commented Mar 27, 2026

Describe what this PR does / why we need it

In NacosLoadBalancer.getInstanceResponse(), when an exception occurs during instance selection, the method returns null instead of a proper EmptyResponse. This violates the Response<ServiceInstance> API contract and can cause NullPointerException in downstream code that calls response.hasServer() or similar methods on the returned value.

Does this pull request fix one issue?

No specific issue filed, found during code review.

Describe how you did it

Changed return null to return new EmptyResponse() in the catch block of getInstanceResponse().

Before:

catch (Exception e) {
    log.warn("NacosLoadBalancer error", e);
    return null;  // violates API contract
}

After:

catch (Exception e) {
    log.warn("NacosLoadBalancer error", e);
    return new EmptyResponse();  // consistent with the empty-instances case
}

This is consistent with the method's own behavior at line 138 where it already returns new EmptyResponse() when no instances are available.

Describe how to verify it

Run NacosLoadBalancerErrorHandlingTests — both tests pass.

Tests Added

Change Point Test
Exception returns EmptyResponse (not null) exceptionShouldReturnEmptyResponseNotNull()
Normal request still works correctly normalRequestShouldReturnValidResponse()

Special notes for reviews

One-line change with minimal risk. The EmptyResponse is already used in the same method for the empty-instances case.

@uuuyuqi uuuyuqi self-assigned this Mar 27, 2026
@uuuyuqi uuuyuqi added the area/nacos spring cloud alibaba nacos label Mar 27, 2026
@uuuyuqi
Copy link
Copy Markdown
Collaborator

uuuyuqi commented Mar 27, 2026

Thanks for the fix, the behavior change is correct.

One suggestion on tests: current tests verify the private helper via reflection, but the bug is observed on the public reactive path (choose()). Could we add one test that goes through choose() end-to-end (mock supplier -> non-empty instance list -> algorithm throws) and assert we get an EmptyResponse instead of an error/NPE?

This would better lock the API-level contract and prevent regressions on the actual call path.

Add three tests that exercise the public reactive choose() API
end-to-end using StepVerifier:
- chooseShouldReturnEmptyResponseWhenAlgorithmThrows
- chooseShouldReturnValidResponseOnNormalPath
- chooseShouldReturnEmptyResponseWhenNoInstances

This locks the API-level contract and prevents regressions on
the actual call path, as suggested in review.
@daguimu
Copy link
Copy Markdown
Contributor Author

daguimu commented Mar 27, 2026

@uuuyuqi Thanks for the suggestion! I've added three end-to-end tests that go through the public choose() reactive path using StepVerifier:

  • chooseShouldReturnEmptyResponseWhenAlgorithmThrows — mocks supplier with non-empty instances, algorithm throws → asserts EmptyResponse (not error/NPE)
  • chooseShouldReturnValidResponseOnNormalPath — verifies normal flow returns DefaultResponse with correct host/port
  • chooseShouldReturnEmptyResponseWhenNoInstances — verifies empty instance list returns EmptyResponse

The original reflection-based tests are kept alongside as a direct unit-level complement. All 5 tests pass locally.

Copy link
Copy Markdown
Collaborator

@uuuyuqi uuuyuqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@uuuyuqi uuuyuqi merged commit 0905316 into alibaba:2025.1.x Mar 31, 2026
3 checks passed
@daguimu daguimu deleted the fix/nacos-loadbalancer-null-return branch March 31, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/nacos spring cloud alibaba nacos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants