RockteMQ-AI commented on code in PR #2861:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2861#discussion_r3896858283
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/broker/MqClientPool.java:
##########
@@ -79,15 +82,23 @@ private <C, T> T execute(ClientKey cacheKey, RPCHook
rpcHook,
if (closed) {
throw new BusinessException(503, "RocketMQ client pool is shutting
down");
}
- @SuppressWarnings("unchecked")
- C client = (C) cache.computeIfAbsent(cacheKey, key -> {
- // Re-check under the cache lock so a request that passed the
initial closed check
- // cannot create a fresh connection while the pool is shutting
down.
+ C client;
+ synchronized (lifecycleLock) {
+ // Re-check under the lifecycle lock so a request that passed the
initial closed check
+ // cannot create a fresh connection while the pool is shutting
down, and a just-created
+ // client cannot be inserted after the shutdown scan has already
run.
if (closed) {
throw new BusinessException(503, "RocketMQ client pool is
shutting down");
}
Review Comment:
The `synchronized(lifecycleLock)` block now wraps the entire client creation
path (`creator.create(...)`), which involves network I/O (NameServer lookup,
broker connect). During creation, all other `execute()` calls — including cache
hits for existing clients — will block on this lock.
For a dashboard with a handful of concurrent users this is acceptable, but
consider a two-phase approach if contention becomes an issue:
1. Create the client outside the lock
2. Acquire `lifecycleLock`, re-check `closed`, then insert into cache
This would preserve the correctness fix while allowing concurrent reads of
already-cached clients during slow creation.
--
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]
