fix: use thread-safe upstream lists in UpstreamCheckService (#6704) - #7053
juicewcode wants to merge 5 commits into
Conversation
…6704) Build upstream data with CopyOnWriteArrayList so submit and scheduled health checks can safely modify and iterate the same selector entries. Ensure replacement and health-check result lists stored in UPSTREAM_MAP use the same thread-safe implementation, and add regression assertions for lists loaded by fetchUpstreamData.
Reuse existing CopyOnWriteArrayList instances when updating UPSTREAM_MAP and only copy lists that are not thread-safe. This preserves the shared list reference used by asynchronous submit tasks while ensuring ordinary lists are converted before being stored.
There was a problem hiding this comment.
🟡 Changes recommended
Health-check completion can still overwrite concurrent upstream updates, and the race is not covered by an active regression test.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates upstream list storage to avoid ConcurrentModificationException during concurrent health checks and registrations.
Changes:
- Uses
CopyOnWriteArrayListfor fetched, replaced, and refreshed upstream lists. - Adds assertions verifying fetched lists use the thread-safe implementation.
File summaries
| File | Description |
|---|---|
UpstreamCheckService.java |
Normalizes shared upstream lists to CopyOnWriteArrayList. |
UpstreamCheckServiceTest.java |
Verifies fetched lists use CopyOnWriteArrayList. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| removePendingSync(successList); | ||
| if (!successList.isEmpty()) { | ||
| UPSTREAM_MAP.put(selectorId, successList); | ||
| UPSTREAM_MAP.put(selectorId, toThreadSafeList(successList)); |
| upstreamCheckService.fetchUpstreamData(); | ||
| assertTrue(upstreamMap.containsKey(MOCK_SELECTOR_NAME)); | ||
| assertEquals(2, upstreamMap.get(MOCK_SELECTOR_NAME).size()); | ||
| assertTrue(upstreamMap.get(MOCK_SELECTOR_NAME) instanceof CopyOnWriteArrayList); |
|
Heads-up (PMC Aias00): your PR's CI failures are in shared infrastructure checks ( |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Approved as PMC (Aias00). Reviewed the diff — small, well-scoped fix with regression tests; mergeable. Note: the only failing CI checks (shenyu-integrated-test-*/e2e-case-logging-rocketmq) are failing cluster-wide across unrelated PRs (shared CI-env issue), not caused by this change.
Fixes #6704
Summary
UpstreamCheckService#fetchUpstreamDatawithCopyOnWriteArrayListinstead ofLinkedList.ConcurrentModificationException.Test
UpstreamCheckServiceTest#testFetchUpstreamDatato verify the loaded upstream lists areCopyOnWriteArrayListinstances.
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.