[ISSUE #6781] Fix stale proxy selector data after empty HTTP refresh. - #6909
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes stale TCP proxy selector runtime state when HTTP synchronization receives an empty full snapshot by ensuring empty snapshots trigger refresh() propagation through proxy selector subscribers/handlers, and by clearing/shutting down cached TCP bootstrap servers.
Changes:
- Invoke
ProxySelectorDataSubscriber.refresh()whenProxySelectorRefresh.refresh(...)receives an empty snapshot. - Propagate refresh from
CommonProxySelectorDataSubscriberintoProxySelectorDataHandler.refresh()and implement TCP handler refresh to clear the bootstrap cache. - Add unit/integration tests covering empty snapshot refresh propagation and TCP bootstrap cache shutdown/clearing behavior (including failure tolerance).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shenyu-sync-data-center/shenyu-sync-data-http/src/main/java/org/apache/shenyu/sync/data/http/refresh/ProxySelectorRefresh.java | Ensures empty proxy selector snapshots propagate refresh to subscribers to clear stale runtime state. |
| shenyu-sync-data-center/shenyu-sync-data-http/src/test/java/org/apache/shenyu/sync/data/http/refresh/ProxySelectorRefreshTest.java | Adds unit coverage verifying empty snapshot triggers subscriber.refresh() and non-empty snapshots subscribe data. |
| shenyu-sync-data-center/shenyu-sync-data-http/src/test/java/org/apache/shenyu/sync/data/http/HttpSyncDataServiceTest.java | Extends HTTP sync test fixture/expectations to include PROXY_SELECTOR group and verify refresh is invoked. |
| shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/handler/ProxySelectorDataHandler.java | Adds a default refresh() hook for proxy selector handlers. |
| shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/cache/CommonProxySelectorDataSubscriber.java | Implements subscriber-level refresh() by delegating to all handler refresh operations. |
| shenyu-plugin/shenyu-plugin-base/src/test/java/org/apache/shenyu/plugin/base/cache/CommonProxySelectorDataSubscriberTest.java | Adds tests asserting refresh() is propagated to all handlers and is safe with no handlers. |
| shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-tcp/src/main/java/org/apache/shenyu/plugin/tcp/handler/TcpProxySelectorDataHandler.java | Makes TCP handler refresh clear the bootstrap cache; hardens removal against null cache entries. |
| shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-tcp/src/main/java/org/apache/shenyu/plugin/tcp/handler/TcpBootstrapFactory.java | Adds cache clearing that shuts down servers, removes entries, and continues on shutdown failures. |
| shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-tcp/src/test/java/org/apache/shenyu/plugin/tcp/handler/TcpProxySelectorDataHandlerTest.java | Adds coverage for refresh-driven shutdown/removal, failure-tolerant shutdown, and idempotent selector removal. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Aias00
left a comment
There was a problem hiding this comment.
Review: #6909 — [ISSUE #6781] Fix stale proxy selector data after empty HTTP refresh
Verdict: ✅ Approve (with minor non-blocking notes)
This is a clean, well-scoped fix for #6781. Good work.
What's correct
- Root cause addressed.
CommonProxySelectorDataSubscriber.refresh()previously just calledProxySelectorDataSubscriber.super.refresh(), which I verified is a no-op (default void refresh() {}in the interface). So an empty full snapshot never cleared proxy-selector runtime data. The new implementation delegates to eachProxySelectorDataHandler.refresh(), which actually tears down state. This is strictly an improvement with no behavior lost from the (empty) interface default. ProxySelectorDataHandler.refresh()added as adefaultmethod → existing handlers keep compiling; only the TCP handler overrides it. Good backward-compat design.TcpBootstrapFactory.clearCache()shuts down each cachedBootstrapServerwith a try/catch so one failing shutdown doesn't abort the rest — exactly the "continue clearing remaining servers when one fails" goal. Using the value-conditionalcache.remove(name, server)is the safe way to mutate aConcurrentHashMapduring iteration (no CME, no stale removal).TcpProxySelectorDataHandler.removeProxySelectornow doesremoveCache()+ null-check before.shutdown(), closing a TOCTOU race where the oldinCache()-then-removeCache().shutdown()could NPE if another thread removed the entry in between.- Propagation wired through
ProxySelectorRefresh: empty snapshot now callsproxySelectorDataSubscribers.forEach(ProxySelectorDataSubscriber::refresh). - Test coverage is solid: subscriber refresh (with/without handlers), TCP handler refresh incl. the "continues when shutdown fails" case and
removeProxySelector, the HTTP sync data-service path, and a dedicatedProxySelectorRefreshTestasserting empty→clear and non-empty→subscribe.
Non-blocking suggestions
- Empty-snapshot semantics are global. Any sync mode that sends an empty
PROXY_SELECTORsnapshot will now hard-stop all TCP bootstrap servers. That matches the documented intent (#6781), but it's worth a one-line comment inProxySelectorRefresh.refresh()noting that an empty snapshot = "clear everything", so future readers don't treat it as a no-op/clear-local-only path. clearCache()mutates the map from insideforEach. Current implementation is safe forConcurrentHashMap, but ifshutdown()ever triggers re-registration (a lifecycle listener), a server could be missed. Not an issue today — just flagging the invariant to preserve.
Verdict
Approving. The fix is correct, the NPE race is real, and the regression tests back the behavior. Nice job.
|
thx for contribution, pls add my wechat: aias00 |
Fixes #6781.
This PR clears stale proxy selector runtime data when HTTP synchronization receives an empty full snapshot.
Changes:
refresh();Although the issue is triggered by HTTP synchronization,
CommonProxySelectorDataSubscriberis shared. Any synchronization mode that invokesrefresh()will now clear the TCP proxy selector runtime cache.Tests:
clean installpassed;git diff --checkpassed.Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.