[BUG] UpstreamCacheManager.findUpstreamListBySelectorId returns live ArrayList mutated by health-check thread without reader sync
- Dominant language
- Java
- Stars
- 8.8k
- Forks
- 3.1k
- Avg merge
- 7d 1h
- Merged PRs (30d)
- 85
Description
## Description
`findUpstreamListBySelectorId` returns `task.getHealthyUpstream().get(selectorId)` — a direct reference to the live `ArrayList` stored in `UpstreamCheckTask.healthyUpstream` (a `ConcurrentMap` of plain `ArrayList` values). The health-check thread modifies this same list under `synchronized(lock)`: `putToMap` calls `list.add(upstream)` and `removeFromMap` calls `list.remove(upstream)`. However, the readers (`DividePlugin:94`, `WebSocketPlugin:92`, `ApacheDubboGrayLoadBalance:57`, `DefaultRetryStrategy:109`) iterate and index the returned list on request threads WITHOUT acquiring `lock`. `ArrayList` is not thread-safe: a concurrent `add` (which may trigger `Arrays.copyOf` resize) or `remove` (which shifts elements via `System.arraycopy`) while a reader calls `size()` then `get(i)` can produce `IndexOutOfBoundsException`, null reads, or stale/inconsistent list state.
## Location
- `shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/cache/UpstreamCacheManager.java:128`
- `shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTask.java:289-292,305-308,355`
- `shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide/src/main/java/org/apache/shenyu/plugin/divide/DividePlugin.java:94`
## Impact
Occasional `IndexOutOfBoundsException` or NPE in the load balancer during a health-check cycle, causing intermittent 500 errors for proxied requests. Also possible: load balancer sees a partially-updated list (missing or phantom upstreams), routing to a stale or non-existent endpoint.
## Suggested fix
Either (a) return a snapshot copy from `findUpstreamListBySelectorId` (`new ArrayList<>(list)` under `synchronized(lock)`), or (b) change the `healthyUpstream` values to `CopyOnWriteArrayList` so readers get a consistent snapshot. Option (b) is lower overhead for the read-heavy path.
## Related existing
None — distinct from #6570 (BaseDataCache.removeSelectData/removeRuleData mutate live list in place) which covers `shenyu-common`'s selector/rule cache, and distinct from N35 (admin-side `UpstreamCheckService.fetchUpstreamData` LinkedList in `shenyu-admin`). This covers the gateway-side `UpstreamCacheManager`/`UpstreamCheckTask` in `shenyu-loadbalancer`.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with UpstreamCacheManager.java:128 and UpstreamCheckTask.java:289-292, 305-308, and 355 to trace how healthyUpstream lists are returned and mutated. Review the listed readers in DividePlugin.java:94, WebSocketPlugin.java:92, ApacheDubboGrayLoadBalance.java:57, and DefaultRetryStrategy.java:109; done means request threads no longer observe unsafe concurrent list state during health checks.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 58/100