lymerin opened a new pull request, #7483:
URL: https://github.com/apache/shenyu/pull/7483
Fixes #7476
### Problem
The default failover retry path calls `.stream()` directly on the result of
`UpstreamCacheManager.findUpstreamListBySelectorId(selectorId)`, although that
method returns `null` when the selector's healthy-upstream cache entry is
absent.
An in-flight request can reach this state when the selector or discovery
upstream data is removed, or an empty upstream update removes the cache entry
before the retry reads it. Health checks that remove individual upstreams
retain the cache key and produce an empty list, which the existing failover
exhaustion check already handles.
The crash occurs with failover enabled, the default retry strategy selected,
and a positive retry count. The retry callback accepts any error signal. Both
WebClient and Netty already convert upstream HTTP 500–599 responses into
`ShenyuUpstreamStatusException` in failover mode, so a connection failure or
timeout is not required. With `retryTimes=0`, no failover callback is installed
and this null-list crash does not occur. The existing 5xx trigger requires no
change for this issue.
### Impact
The NPE escapes the existing error mappings and reaches
`GlobalErrorHandler`, producing HTTP 500 instead of the controlled failover
exhaustion response. With the default result implementation, the response is
`application/json` with the body `{"code":500,"message":"Internal Server
Error"}`. The handler also calls
`AlarmSender.alarmMediumCritical("ShenYu-Gateway-Global-Error", ...)`.
Both HTTP client plugins share the affected default retry strategy. Requests
retrying after a cache-entry removal are affected, including requests whose
upstream returned an ordinary 5xx response. Missing lists fail before upstream
selection; empty lists already follow the controlled error path.
After the fix, missing lists produce the existing HTTP 503 failover
exhaustion response with reason
`CANNOT_FIND_HEALTHY_UPSTREAM_URL_AFTER_FAILOVER`. The global handler's
medium-critical alarm call also applies to this controlled 503 response; this
change does not remove that alarm.
### Changes
- Read the latest upstream list once into a local variable and check it for
`null` before streaming.
- Return the existing
`ShenyuException("CANNOT_FIND_HEALTHY_UPSTREAM_URL_AFTER_FAILOVER")` for a
missing list, allowing the existing mapping to produce HTTP 503.
- Preserve the existing filtering of previously visited upstreams,
empty-list handling, load balancing, retry count, timeout handling, and request
replay behavior.
- Add only two regression tests in existing test classes:
- A missing cache entry with three failover retries returns the controlled
503 error without invoking another upstream request.
- A WebClient HTTP 500 response, supplied through the mocked exchange
function after removing a previously populated real cache entry with
`removeByKey`, follows the actual 5xx-to-error conversion and returns the same
controlled 503 error. Only the initial HTTP exchange occurs, and the downstream
plugin chain is not invoked.
Production changes are confined to `DefaultRetryStrategy`; no cache API,
global error handler, or unrelated code is changed.
### Validation
- Before applying the production fix, ran both new regression tests with the
Maven wrapper:
```powershell
.\mvnw.cmd -pl shenyu-plugin/shenyu-plugin-httpclient
'-Dtest=RetryStrategyTest#testFailoverWithMissingUpstreamList,WebClientPluginTest#testServerErrorAfterUpstreamRemoval'
test
```
Both tests failed on the original null-list `.stream()` call with the
expected NPE, confirming that the regressions detect the issue, including the
ordinary HTTP 500 trigger.
- After the fix, ran the complete HTTP client module test suite:
```powershell
.\mvnw.cmd -pl shenyu-plugin/shenyu-plugin-httpclient test
```
All **41 tests passed**, with zero failures, errors, or skipped tests.
Checkstyle passed with zero violations, and the Maven build completed
successfully.
- The contributor also completed the full local `clean install` successfully.
### Checklist
Make sure that:
- [x] You have read the [contribution
guidelines](https://shenyu.apache.org/community/contributor-guide).
- [x] You submit test cases (unit or integration tests) that back your
changes.
- [x] Your local test passed `./mvnw clean install
-Dmaven.javadoc.skip=true`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
