Copilot commented on code in PR #15891:
URL: https://github.com/apache/dubbo/pull/15891#discussion_r2638978810
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos
server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL
connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the
given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL
connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) ->
{
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}",
key);
+ NacosNamingServiceWrapper newWrapper =
createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
+ public static void releaseNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Decrement the reference count and close the connection if it
reaches zero.
+ SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ return null;
+ }
+
+ if (v.refCount.decrementAndGet() <= 0) {
Review Comment:
The condition `v.refCount.decrementAndGet() <= 0` has a race condition risk.
If the count becomes negative due to unbalanced release calls (more releases
than creates), the wrapper will be shut down and removed from the cache, but
subsequent releases will find `v == null` and silently return. This could mask
bugs where releaseNamingService is called more times than createNamingService
for the same URL.
Consider changing the condition to check `== 0` instead of `<= 0`, and add
explicit logging or an assertion when the count goes negative to help detect
misuse.
```suggestion
int newRefCount = v.refCount.decrementAndGet();
if (newRefCount < 0) {
logger.warn(REGISTRY_NACOS_EXCEPTION, "", "",
"NacosNamingService reference count for key " + key + "
dropped below zero: " + newRefCount);
}
if (newRefCount == 0) {
```
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos
server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL
connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the
given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL
connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) ->
{
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}",
key);
+ NacosNamingServiceWrapper newWrapper =
createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
Review Comment:
The new public method `releaseNamingService` lacks proper JavaDoc
documentation. While the internal comment on line 159 explains its purpose,
users of this API need to understand:
1. When and why to call this method
2. That it must be called exactly once for each corresponding
createNamingService() call
3. What happens if it's called without a prior createNamingService() call
4. Thread-safety guarantees
Consider adding comprehensive JavaDoc similar to the style used for
createNamingService().
```suggestion
/**
* Release a previously acquired shared {@link
NacosNamingServiceWrapper} for the given connection URL.
* <p>
* This method must be called exactly once for every successful
invocation of
* {@link #createNamingService(URL)} using an equivalent {@link URL}
(i.e., one that results in the same
* internal cache key). When the reference count for the corresponding
cached entry drops to zero, the
* underlying {@link NamingService} is shut down and removed from the
cache.
* </p>
* <p>
* If this method is called with a {@code connectionURL} for which no
{@link #createNamingService(URL)}
* call has been made before (or if the associated entry has already
been fully released), the call is a
* no-op and no exception is thrown.
* </p>
* <p>
* This method is thread-safe. Reference counting and cache updates are
performed atomically using a
* {@link ConcurrentMap} and {@link
java.util.concurrent.atomic.AtomicInteger}, so it is safe to call
* {@code createNamingService} and {@code releaseNamingService}
concurrently from multiple threads.
* </p>
*
* @param connectionURL the registry connection URL associated with the
naming service to release
* @since 2.7.5
*/
```
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos
server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL
connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the
given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL
connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) ->
{
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}",
key);
+ NacosNamingServiceWrapper newWrapper =
createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
Review Comment:
There's a subtle race condition in the reference counting logic. If two
threads call createNamingService() concurrently with the same key, both could
observe `v == null`, create separate NacosNamingServiceHolder instances, and
one would overwrite the other. While ConcurrentHashMap.compute() is atomic, the
logic inside creates a new holder on line 150 and then increments its refCount
on line 152. If the holder gets replaced between creation and incrementing, the
reference count will be incorrect.
The current implementation is actually safe because compute() guarantees
atomicity of the entire lambda execution, but for clarity and robustness,
consider initializing the refCount to 1 in the constructor instead of 0, and
only increment it for existing holders. This makes the intent clearer and
removes the possibility of future bugs if the code is refactored.
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos
server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL
connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the
given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL
connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) ->
{
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}",
key);
+ NacosNamingServiceWrapper newWrapper =
createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
+ public static void releaseNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Decrement the reference count and close the connection if it
reaches zero.
+ SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ return null;
+ }
+
+ if (v.refCount.decrementAndGet() <= 0) {
+ try {
+ logger.info("Destroying shared NacosNamingService for key:
{}", key);
+ v.wrapper.shutdown();
+ } catch (Exception e) {
+ logger.warn(
+ REGISTRY_NACOS_EXCEPTION, "", "", "Failed to
destroy naming service for key: " + key, e);
+ }
+ return null;
+ }
+ return v;
+ });
+ }
+
+ /**
+ * Create a new {@link NacosNamingServiceWrapper} using the normalized
+ * connection URL and configured retry options.
+ */
+ private static NacosNamingServiceWrapper createWrapperInternal(URL
connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+
boolean check = connectionURL.getParameter(NACOS_CHECK_KEY, true);
int retryTimes = connectionURL.getPositiveParameter(NACOS_RETRY_KEY,
10);
int sleepMsBetweenRetries =
connectionURL.getPositiveParameter(NACOS_RETRY_WAIT_KEY, 10);
+
if (check && !UrlUtils.isCheck(connectionURL)) {
check = false;
}
+
NacosConnectionManager nacosConnectionManager =
- new NacosConnectionManager(connectionURL, check, retryTimes,
sleepMsBetweenRetries);
+ new NacosConnectionManager(normalized, check, retryTimes,
sleepMsBetweenRetries);
return new NacosNamingServiceWrapper(nacosConnectionManager,
retryTimes, sleepMsBetweenRetries);
}
+
+ /**
+ * Normalizes the URL by removing group parameters and sorting the server
address list.
+ * This method removes grouping parameters and standardizes the server
address list
Review Comment:
The javadoc comment is incomplete - it ends abruptly without explaining what
the method does with the standardized server address list. Complete the
documentation to explain that addresses are trimmed, deduplicated (via sorted
stream), and sorted alphabetically to ensure consistent cache keys regardless
of the order addresses are specified in the URL.
```suggestion
* This method removes grouping parameters and standardizes the server
address list by trimming
* whitespace, discarding empty entries, deduplicating and sorting
addresses alphabetically so
* that the resulting URL produces consistent cache keys regardless of
the order in which
* server addresses are specified.
```
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos
server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL
connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the
given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL
connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) ->
{
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}",
key);
+ NacosNamingServiceWrapper newWrapper =
createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
+ public static void releaseNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Decrement the reference count and close the connection if it
reaches zero.
+ SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ return null;
+ }
+
+ if (v.refCount.decrementAndGet() <= 0) {
+ try {
+ logger.info("Destroying shared NacosNamingService for key:
{}", key);
+ v.wrapper.shutdown();
+ } catch (Exception e) {
+ logger.warn(
+ REGISTRY_NACOS_EXCEPTION, "", "", "Failed to
destroy naming service for key: " + key, e);
+ }
+ return null;
+ }
+ return v;
+ });
+ }
+
+ /**
+ * Create a new {@link NacosNamingServiceWrapper} using the normalized
+ * connection URL and configured retry options.
+ */
+ private static NacosNamingServiceWrapper createWrapperInternal(URL
connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+
boolean check = connectionURL.getParameter(NACOS_CHECK_KEY, true);
int retryTimes = connectionURL.getPositiveParameter(NACOS_RETRY_KEY,
10);
int sleepMsBetweenRetries =
connectionURL.getPositiveParameter(NACOS_RETRY_WAIT_KEY, 10);
+
if (check && !UrlUtils.isCheck(connectionURL)) {
check = false;
}
+
NacosConnectionManager nacosConnectionManager =
- new NacosConnectionManager(connectionURL, check, retryTimes,
sleepMsBetweenRetries);
+ new NacosConnectionManager(normalized, check, retryTimes,
sleepMsBetweenRetries);
return new NacosNamingServiceWrapper(nacosConnectionManager,
retryTimes, sleepMsBetweenRetries);
Review Comment:
The normalization logic recreates the URL using
`URL.valueOf(connectionURL.toServiceStringWithoutResolving())`, which may not
preserve all parameters from the original URL. Parameters like `nacos.retry`,
`nacos.retry-wait`, and `nacos.check` are needed in createWrapperInternal()
(lines 190-192) but are retrieved from the original `connectionURL` parameter,
not from `normalized`.
While this works correctly because createWrapperInternal() receives the
original connectionURL as a parameter, it creates a subtle inconsistency: the
normalized URL is used for creating the NacosConnectionManager, but retry
parameters come from the original URL. This could be confusing for future
maintainers. Consider adding a comment explaining why some parameters are taken
from the original URL rather than the normalized one.
##########
dubbo-registry/dubbo-registry-nacos/src/test/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtilsTest.java:
##########
@@ -172,4 +175,9 @@ public String getServerStatus() {
}
}
}
+
+ @AfterEach
+ void tearDown() {
+ NacosNamingServiceUtils.clearCacheForTest();
+ }
}
Review Comment:
The test suite is missing coverage for the core connection reuse
functionality introduced in this PR. There are no tests that verify:
1. Multiple createNamingService() calls with the same connection parameters
return the same wrapper instance
2. The reference counting mechanism works correctly (incrementing on create,
decrementing on release)
3. The connection is only shut down when the last reference is released
4. Connections are properly reused across different registry groups
Consider adding a test that creates multiple NamingService instances with
different group parameters but the same server address, verifies they share the
same underlying connection, then releases them and confirms the connection is
only closed after all references are released.
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos
server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL
connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the
given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL
connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) ->
{
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}",
key);
+ NacosNamingServiceWrapper newWrapper =
createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
+ public static void releaseNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Decrement the reference count and close the connection if it
reaches zero.
+ SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ return null;
+ }
+
+ if (v.refCount.decrementAndGet() <= 0) {
+ try {
+ logger.info("Destroying shared NacosNamingService for key:
{}", key);
+ v.wrapper.shutdown();
+ } catch (Exception e) {
+ logger.warn(
+ REGISTRY_NACOS_EXCEPTION, "", "", "Failed to
destroy naming service for key: " + key, e);
+ }
+ return null;
+ }
+ return v;
+ });
+ }
+
+ /**
+ * Create a new {@link NacosNamingServiceWrapper} using the normalized
+ * connection URL and configured retry options.
+ */
+ private static NacosNamingServiceWrapper createWrapperInternal(URL
connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+
boolean check = connectionURL.getParameter(NACOS_CHECK_KEY, true);
int retryTimes = connectionURL.getPositiveParameter(NACOS_RETRY_KEY,
10);
int sleepMsBetweenRetries =
connectionURL.getPositiveParameter(NACOS_RETRY_WAIT_KEY, 10);
+
if (check && !UrlUtils.isCheck(connectionURL)) {
check = false;
}
+
NacosConnectionManager nacosConnectionManager =
- new NacosConnectionManager(connectionURL, check, retryTimes,
sleepMsBetweenRetries);
+ new NacosConnectionManager(normalized, check, retryTimes,
sleepMsBetweenRetries);
return new NacosNamingServiceWrapper(nacosConnectionManager,
retryTimes, sleepMsBetweenRetries);
}
+
+ /**
+ * Normalizes the URL by removing group parameters and sorting the server
address list.
+ * This method removes grouping parameters and standardizes the server
address list
+ */
+ private static URL normalizeConnectionURL(URL connectionURL) {
+ URL normalized =
URL.valueOf(connectionURL.toServiceStringWithoutResolving())
+ .removeParameter(GROUP_KEY)
+ .removeParameter(NACOS_GROUP_KEY);
+
+ String serverAddr = normalized.getParameter("serverAddr",
normalized.getAddress());
+ if (serverAddr != null) {
+ String canonical = Arrays.stream(serverAddr.split(","))
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .sorted()
+ .collect(Collectors.joining(","));
+ normalized = normalized.addParameter("serverAddr", canonical);
+ }
Review Comment:
The fallback `normalized.getAddress()` on line 212 may not provide a
meaningful value when `serverAddr` parameter is present in the URL. In Nacos
configurations, the server address is typically specified via the `serverAddr`
parameter (comma-separated list), not via the URL host/port. If `serverAddr` is
null or empty, falling back to a single host from the URL may not accurately
represent the connection identity.
Consider whether the normalization should fail or log a warning when neither
`serverAddr` parameter nor a valid address is present, as this could indicate a
misconfiguration.
```suggestion
// Prefer explicitly configured serverAddr; only fall back to URL
address if non-empty.
String serverAddr = normalized.getParameter("serverAddr");
if (serverAddr == null || serverAddr.trim().isEmpty()) {
String addressFromUrl = normalized.getAddress();
if (addressFromUrl != null && !addressFromUrl.trim().isEmpty()) {
serverAddr = addressFromUrl;
} else {
// Neither serverAddr nor a valid URL address is present;
this likely indicates misconfiguration.
logger.warn(REGISTRY_NACOS_EXCEPTION, "", "",
"No valid Nacos server address found in URL
parameters or host/port: " + connectionURL);
return normalized;
}
}
String canonical = Arrays.stream(serverAddr.split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.sorted()
.collect(Collectors.joining(","));
normalized = normalized.addParameter("serverAddr", canonical);
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
