apache / apache/shenyu

[BUG] UpstreamCacheManager.findUpstreamListBySelectorId returns live ArrayList mutated by health-check thread without reader sync

Open
#6,857 1 comment 0 reactions 0 assignees View on GitHub
plugin: divide priority: medium type: bug
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.