fix(tcp): fix ignored proxy selector port and property updates - #7497

Hi, @lymerin, thanks for your PR. I do not see a blocking issue; a few notes below on the new per-selector operation lock.

Verification

On 5915dc0b5:

• ./mvnw -pl shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-tcp -am test -Dtest='TcpBootstrapFactoryTest,TcpProxySelectorDataHandlerTest,TcpUpstreamDataHandlerTest' -> 18 tests, 0 failures.
• ./mvnw -pl shenyu-admin -am test -Dtest=ProxySelectorServiceTest -> 19 tests, 0 failures.
• checkstyle:check passes for shenyu-admin and shenyu-plugin-tcp.

Notes

• Both halves of the bug are addressed: admin now publishes a PROXY_SELECTOR UPDATE event (registered on the transaction and published only for STATUS_COMMITTED), and the gateway factory replaces the cached listener when the port or props change instead of returning early. The payload uses the same mapToData shape as the existing discovery-processor events, so subscriber lookup and namespace routing reuse the established path.
• Comparing the listener configuration against a defensive snapshot (snapshot() plus Properties.equals) is what makes an unconditional handlerProxySelector safe: unrelated selector syncs do not restart the listener, and props are compared by content, not by object identity. That matters because a restart drops live TCP connections.
• The two replacement branches are ordered correctly: same port -> shutdown then start (the port must be free), different port -> start then shutdown (no gap on the old port). The recovery path that re-creates the previous configuration, and the removal of the cache entry when even that fails, keep the failure modes honest - shouldRetainPreviousConfigurationAfterUpdateFailure covers both branches.
• createServer initialising UpstreamProvider only when it has no entry is the subtle half of the fix; without it a selector update could wipe upstreams that arrived before the listener.

Possible Improvements

• acquireOperation is a spin over putIfAbsent + join@ and the futures double as per-selector locks. Correctness looks fine (a finallyalways completes the future, and the removal is compare-and-remove), but waiters block uninterruptibly, so a critical section that never returns - for exampleshutdown()on a hung listener - would block every later update for that selector indefinitely. A per-selectorReentrantLock` (or a bounded wait with an explicit failure) would be easier to reason about and to observe in production.
• clearCache() now takes the union of cache and operations keys and calls removeAndShutdown per selector. It is not called from inside an operation today, but if it ever were, it would self-deadlock on that selector's own entry; a comment would help.
• DataChangedEvent.getEventType() was widened to public only so the new test can assert the event type. Since ProxySelectorServiceTest lives in another package, keeping the accessor package-private and asserting through getGroupKey() and the payload would avoid widening the API.
• The asymmetry between the two replacement branches (shutdown-before-start vs start-before-shutdown) is deliberate but subtle; one comment per branch would stop a future reader from "simplifying" it into a port conflict.

Thank you for the detailed review and suggestions. I’ll take a closer look at the locking implementation, API exposure, and comments, and follow up here.

Thanks for the detailed review. I revisited the synchronization design after your comments.

The original CompletableFuture<Void> operation map came from trying to extend the single-flight idea introduced in #7012 beyond initial creation, so that creation, replacement, and removal could use a similar synchronization model.

After reviewing #7471 again, I think that extends the abstraction further than this issue requires. I’ve therefore narrowed the design.

1. Per-selector operation lock

CompletableFuture<BootstrapServer> is restored to the original #7012 role: initial creation single-flight, where concurrent callers participate in the same creation and share its result or failure.

Existing listener replacement and removal now use a per-selector ReentrantLock, since they only need mutual exclusion while modifying the same cached listener.

After waiting for an in-flight creation, the update path re-reads the cache; after acquiring the replacement lock, it re-reads and compares the configuration again. This avoids applying an update based on stale state.

I did not add a bounded wait or timeout/cancellation policy. A ReentrantLock still cannot make a hung shutdown() bounded, but handling that correctly would require defining what happens to the pending configuration update after timeout, including retry or reconciliation semantics. Since #7471 is specifically about ignored forwardPort / props updates, I’d prefer not to expand this fix into a broader lifecycle recovery design.

2. clearCache()

I removed the previous operation-registry handling from clearCache(). It now works only from the cached selector set, so the self-deadlock scenario you pointed out for the operation registry no longer exists.

3. DataChangedEvent.getEventType()

I restored getEventType() to package-private and removed the test-only API widening.

4. Replacement ordering

I added comments explaining the intentional asymmetry:

• same port: shut down the current listener first because the replacement cannot bind to the same port;
• different port: start the replacement first so a startup failure does not take the current listener offline.

There is one API point where I’d like your guidance. TcpBootstrapFactory.cache() previously accepted:

cache(String selectorName, BootstrapServer server)

but #7471 requires the cache to retain the listener’s effective forwardPort and props, so the current version uses:

cache(TcpServerConfiguration configuration, BootstrapServer server)

I found only test callers of this method in the repository, but changing the signature is still technically a public API change.

My preference is to keep the new signature. The old API does not provide enough information to build a correct configuration snapshot, and keeping a compatibility overload would require inventing an unknown/default port and properties, which could make later configuration comparisons incorrect and trigger unnecessary replacements.

However, whether this method needs source compatibility is a project-level API decision, so I’d like your opinion: is changing this public method acceptable here, or would you prefer that we preserve the old signature in some other way?