fix: prevent duplicate TCP bootstrap server creation - #7012
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Review: Prevent duplicate TCP bootstrap server creation (#6735)
Verdict: APPROVE
Analysis
Solid, well-tested fix addressing three related issues in the TCP bootstrap lifecycle.
1. Duplicate-creation guard (TcpBootstrapFactory)
- Introduces a
creationsConcurrentMap<String, CompletableFuture<BootstrapServer>>to track in-flight creations. createBootstrapServerIfAbsent:cache.containsKeyfast path returnsfalseif already present.creations.putIfAbsentensures only one thread performs creation; concurrent callersawaitCreation(which correctly unwrapsCompletionExceptionto the original cause) and returnfalse.- Double-checked
cache.getafter winning the creation future; if another server already landed, the new one isshutdown()(no leak) and the existing is returned. - On
RuntimeException,completeExceptionally+ rethrow, withfinallydoingcreations.remove(selectorName, creation)(atomic, value-checked cleanup).getCacheexists (verified) andcreateBootstrapServercallsstart()internally, so a bind failure propagates and the partially-built server is discarded correctly.
removeAndShutdownis idempotent (no-op when absent).
2. Resource leak on bind failure (TcpBootstrapServer.start)
start()now wrapsbindNow()in try/catch; on failure it disposes theLoopResources(which was already assigned to the field before bind) before rethrowing. No leaked event-loop resources.
3. Idempotent / safe shutdown (TcpBootstrapServer.shutdown)
- Now
synchronizedwith adisposedguard, so repeated shutdown is a no-op. BothserverandloopResourcesare null-guarded, and failures are collected withaddSuppressedso no exception is silently swallowed.
Tests: TcpBootstrapServerTest + TcpBootstrapFactoryTest cover concurrent-single-creation, retry-after-failure, failure unwrapping, non-blocking removal of a different selector during shutdown, loop-resource disposal on bind failure, single shutdown, and exception propagation on disposal failure. Coverage is thorough.
Conclusion
The concurrency model is correct, imports are clean (no leftover unused imports in TcpProxySelectorDataHandler), and the edge cases are exercised. Approving.
Fixes #6735.
What is changed
TcpProxySelectorDataHandlerpreviously used a non-atomic check-then-act sequence:Concurrent selector synchronization events could both observe a cache miss and start bootstrap servers for the same selector. This could cause port binding failures or leave an overwritten server and its event-loop resources without lifecycle management.
This change:
TcpBootstrapFactory;ConcurrentHashMapatomic callbacks and without a global lifecycle lock;putIfAbsentwhen publishing the started server and shuts down a losing instance if another server was already cached;TcpProxySelectorDataHandlerthrough the atomic creation and removal methods;LoopResourceswhen bootstrap startup fails;TcpBootstrapServer.shutdown()idempotent and preserves shutdown failures using suppressed exceptions.The change is limited to the duplicate bootstrap creation and related resource lifecycle described in #6735. Existing selector configuration update behavior is unchanged.
Tests
Added or extended tests covering:
The following targeted Maven build passed locally:
mvn -pl shenyu-protocol/shenyu-protocol-tcp,shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-tcp \ -am \ -Dtest=TcpBootstrapFactoryTest,TcpBootstrapServerTest,TcpProxySelectorDataHandlerTest \ -Dsurefire.failIfNoSpecifiedTests=false \ testResults:
TcpBootstrapServerTest: 11 tests passedTcpBootstrapFactoryTest: 4 tests passedTcpProxySelectorDataHandlerTest: 3 tests passedChecklist
./mvnw clean install -Dmaven.javadoc.skip=truebuild.