RockteMQ-AI commented on code in PR #2196:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2196#discussion_r3781963377
##########
server/src/test/java/org/apache/rocketmq/studio/provider/tencent/TencentClientFactoryTest.java:
##########
@@ -45,4 +53,62 @@ void
endpointForShouldUseRegionalEndpointForFinancialRegionsTest() {
assertThat(TencentClientFactory.endpointFor("ap-shanghai-fsi"))
.isEqualTo("trocket.ap-shanghai-fsi.tencentcloudapi.com");
}
+
+ @Test
+ void invalidateCredentialShouldRemoveAnInFlightClientCreation() throws
Exception {
+ BlockingClientFactory factory = new BlockingClientFactory();
+ FutureTask<TrocketClient> firstClientTask = new FutureTask<>(
+ () -> factory.client("credential-a", "ap-shanghai"));
+ Thread creationThread = new Thread(firstClientTask,
"tencent-client-creation-test");
+ creationThread.start();
+ assertThat(factory.creationStarted.await(5,
TimeUnit.SECONDS)).isTrue();
+
+ Thread invalidationThread = new Thread(
+ () -> factory.invalidateCredential("credential-a"),
+ "tencent-client-invalidation-test");
+ invalidationThread.start();
+ awaitBlockedOrTerminated(invalidationThread);
+
+ factory.allowCreation.countDown();
+ TrocketClient firstClient = firstClientTask.get(5, TimeUnit.SECONDS);
+ invalidationThread.join(TimeUnit.SECONDS.toMillis(5));
+
+ assertThat(invalidationThread.isAlive()).isFalse();
+ assertThat(factory.client("credential-a",
"ap-shanghai")).isNotSameAs(firstClient);
+ assertThat(factory.creationCount).hasValue(2);
+ }
+
+ private static void awaitBlockedOrTerminated(Thread thread) {
+ long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5);
+ while (thread.getState() != Thread.State.BLOCKED && thread.isAlive()
Review Comment:
awaitBlockedOrTerminated relies on Thread.getState() and spin-waiting to
observe monitor contention. While deterministic in this controlled test,
assertions based on thread states can be brittle under different JVM schedulers.
##########
server/src/main/java/org/apache/rocketmq/studio/provider/tencent/TencentClientFactory.java:
##########
@@ -48,10 +48,16 @@ public class TencentClientFactory {
public TrocketClient client(String credentialId, String region) {
String key = cacheKey(credentialId, region);
- return clients.computeIfAbsent(key, ignored ->
createClient(credentialId, region));
+ TrocketClient cached = clients.get(key);
+ if (cached != null) {
+ return cached;
+ }
+ synchronized (this) {
Review Comment:
Cache misses now synchronize on the factory instance, serializing all client
creation across every credential/region. If this path is heavily contended,
consider a finer-grained lock (e.g., per-key or a read/write lock) instead of a
global monitor.
##########
server/src/main/java/org/apache/rocketmq/studio/provider/tencent/TencentClientFactory.java:
##########
@@ -48,10 +48,16 @@ public class TencentClientFactory {
public TrocketClient client(String credentialId, String region) {
String key = cacheKey(credentialId, region);
Review Comment:
The lock-free fast-path get can return a cached client while
invalidateCredential is concurrently removing it, leaving a brief stale-read
window. This appears intentional to preserve hit performance; confirm it is
acceptable for the credential-rotation semantics.
--
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]
