lymerin commented on PR #7497: URL: https://github.com/apache/shenyu/pull/7497#issuecomment-6079656641
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: ```scss cache(String selectorName, BootstrapServer server) ``` but #7471 requires the cache to retain the listener’s effective `forwardPort` and `props`, so the current version uses: ```scss 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?** -- 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]
