lymerin commented on PR #7497: URL: https://github.com/apache/shenyu/pull/7497#issuecomment-6064830633
> 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 `finally`always completes the future, and the removal is compare-and-remove), but waiters block uninterruptibly, so a critical section that never returns - for example`shutdown()`on a hung listener - would block every later update for that selector indefinitely. A per-selector`ReentrantLock` (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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
