From 1d5fe72643f1a868880c6dee2f19c624a5a01ded Mon Sep 17 00:00:00 2001 From: zrlw Date: Tue, 7 Sep 2021 01:01:50 +0800 Subject: [PATCH 1/7] fix localUrlInvokerMap concurrent access problem --- .../ServiceDiscoveryRegistryDirectory.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java index 6e125198ef7..cabce5c057a 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java @@ -241,12 +241,17 @@ private void refreshInvoker(List invokerUrls) { destroyAllInvokers(); // Close all invokers } else { this.forbidden = false; // Allow accessing - Map> oldUrlInvokerMap = this.urlInvokerMap; // local reference if (CollectionUtils.isEmpty(invokerUrls)) { return; } - Map> newUrlInvokerMap = toInvokers(invokerUrls);// Translate url list to Invoker map + // can't use local reference because this.urlInvokerMap might be accessed at isAvailable() by main thread concurrently. + Map> oldUrlInvokerMap = null; + if (this.urlInvokerMap != null) { + oldUrlInvokerMap = new HashMap<>(); + this.urlInvokerMap.forEach(oldUrlInvokerMap::put); + } + Map> newUrlInvokerMap = toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map logger.info("Refreshed invoker size " + newUrlInvokerMap.size()); if (CollectionUtils.isEmptyMap(newUrlInvokerMap)) { @@ -275,11 +280,13 @@ private void refreshInvoker(List invokerUrls) { /** * Turn urls into invokers, and if url has been refer, will not re-reference. + * the items that will be put into newUrlInvokeMap will be removed from oldUrlInvokerMap. * + * @param oldUrlInvokerMap * @param urls * @return invokers */ - private Map> toInvokers(List urls) { + private Map> toInvokers(Map> oldUrlInvokerMap, List urls) { Map> newUrlInvokerMap = new HashMap<>(); if (urls == null || urls.isEmpty()) { return newUrlInvokerMap; @@ -304,7 +311,7 @@ private Map> toInvokers(List urls) { instanceAddressURL = overrideWithConfigurator(instanceAddressURL); } - Invoker invoker = urlInvokerMap == null ? null : urlInvokerMap.get(instanceAddressURL.getAddress()); + Invoker invoker = oldUrlInvokerMap == null ? null : oldUrlInvokerMap.get(instanceAddressURL.getAddress()); if (invoker == null || urlChanged(invoker, instanceAddressURL)) { // Not in the cache, refer again try { boolean enabled = true; @@ -324,7 +331,7 @@ private Map> toInvokers(List urls) { } } else { newUrlInvokerMap.put(instanceAddressURL.getAddress(), invoker); - urlInvokerMap.remove(instanceAddressURL.getAddress(), invoker); + oldUrlInvokerMap.remove(instanceAddressURL.getAddress(), invoker); } } return newUrlInvokerMap; From 68df6358cae938d20b658e407f09192085a136d2 Mon Sep 17 00:00:00 2001 From: zrlw Date: Tue, 7 Sep 2021 02:20:53 +0800 Subject: [PATCH 2/7] add forbidden check at isAvailable() --- .../registry/client/ServiceDiscoveryRegistryDirectory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java index 886b4121026..aa643490478 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java @@ -124,7 +124,7 @@ public void buildRouterChain(URL url) { @Override public boolean isAvailable() { - if (isDestroyed()) { + if (isDestroyed() || this.forbidden) { return false; } Map> localUrlInvokerMap = urlInvokerMap; From 3b797e2452277654b36ec73574f4536a7d6f71b6 Mon Sep 17 00:00:00 2001 From: zrlw Date: Tue, 7 Sep 2021 21:46:19 +0800 Subject: [PATCH 3/7] fix RegistryDirectory localUrlInvokerMap concurrent access problem --- .../integration/RegistryDirectory.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java index 4fa1f7680fe..67775ca9d35 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java @@ -203,7 +203,7 @@ private void refreshInvoker(List invokerUrls) { destroyAllInvokers(); // Close all invokers } else { this.forbidden = false; // Allow to access - Map> oldUrlInvokerMap = this.urlInvokerMap; // local reference + if (invokerUrls == Collections.emptyList()) { invokerUrls = new ArrayList<>(); } @@ -216,7 +216,14 @@ private void refreshInvoker(List invokerUrls) { if (invokerUrls.isEmpty()) { return; } - Map> newUrlInvokerMap = toInvokers(invokerUrls);// Translate url list to Invoker map + + // can't use local reference because this.urlInvokerMap might be accessed at isAvailable() by main thread concurrently. + Map> oldUrlInvokerMap = null; + if (this.urlInvokerMap != null) { + oldUrlInvokerMap = new HashMap<>(); + this.urlInvokerMap.forEach(oldUrlInvokerMap::put); + } + Map> newUrlInvokerMap = toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map /** * If the calculation is wrong, it is not processed. @@ -307,11 +314,12 @@ private Optional> toRouters(List urls) { /** * Turn urls into invokers, and if url has been refer, will not re-reference. + * the items that will be put into newUrlInvokeMap will be removed from oldUrlInvokerMap. * - * @param urls + * @param oldUrlInvokerMap * @param urls * @return invokers */ - private Map> toInvokers(List urls) { + private Map> toInvokers(Map> oldUrlInvokerMap, List urls) { Map> newUrlInvokerMap = new ConcurrentHashMap<>(); if (urls == null || urls.isEmpty()) { return newUrlInvokerMap; @@ -345,8 +353,7 @@ private Map> toInvokers(List urls) { URL url = mergeUrl(providerUrl); // Cache key is url that does not merge with consumer side parameters, regardless of how the consumer combines parameters, if the server url changes, then refer again - Map> localUrlInvokerMap = this.urlInvokerMap; // local reference - Invoker invoker = localUrlInvokerMap == null ? null : localUrlInvokerMap.remove(url); + Invoker invoker = oldUrlInvokerMap == null ? null : oldUrlInvokerMap.remove(url); if (invoker == null) { // Not in the cache, refer again try { boolean enabled = true; @@ -561,7 +568,7 @@ public void setRegisteredConsumerUrl(URL url) { @Override public boolean isAvailable() { - if (isDestroyed()) { + if (isDestroyed() || this.forbidden) { return false; } Map> localUrlInvokerMap = urlInvokerMap; From b9a2f73f07ffeda8b5a08c91e9e6937aff1a049b Mon Sep 17 00:00:00 2001 From: zrlw Date: Tue, 7 Sep 2021 21:58:32 +0800 Subject: [PATCH 4/7] format code --- .../apache/dubbo/registry/integration/RegistryDirectory.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java index 67775ca9d35..a03601a5c12 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java @@ -316,7 +316,8 @@ private Optional> toRouters(List urls) { * Turn urls into invokers, and if url has been refer, will not re-reference. * the items that will be put into newUrlInvokeMap will be removed from oldUrlInvokerMap. * - * @param oldUrlInvokerMap * @param urls + * @param oldUrlInvokerMap + * @param urls * @return invokers */ private Map> toInvokers(Map> oldUrlInvokerMap, List urls) { From e42d0a14687e0a023b13b7e651bfb52366a3ee91 Mon Sep 17 00:00:00 2001 From: zrlw Date: Sat, 11 Sep 2021 23:07:32 +0800 Subject: [PATCH 5/7] change oldUrlInvokerMap to LinkedHashMap and set default size --- .../registry/client/ServiceDiscoveryRegistryDirectory.java | 3 ++- .../apache/dubbo/registry/integration/RegistryDirectory.java | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java index aa643490478..67265cab6ab 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java @@ -42,6 +42,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -253,7 +254,7 @@ private void refreshInvoker(List invokerUrls) { // can't use local reference because this.urlInvokerMap might be accessed at isAvailable() by main thread concurrently. Map> oldUrlInvokerMap = null; if (this.urlInvokerMap != null) { - oldUrlInvokerMap = new HashMap<>(); + oldUrlInvokerMap = new LinkedHashMap<>(this.urlInvokerMap.size()); this.urlInvokerMap.forEach(oldUrlInvokerMap::put); } Map> newUrlInvokerMap = toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java index a03601a5c12..a6edd1744ba 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java @@ -47,6 +47,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -220,7 +221,7 @@ private void refreshInvoker(List invokerUrls) { // can't use local reference because this.urlInvokerMap might be accessed at isAvailable() by main thread concurrently. Map> oldUrlInvokerMap = null; if (this.urlInvokerMap != null) { - oldUrlInvokerMap = new HashMap<>(); + oldUrlInvokerMap = new LinkedHashMap<>(this.urlInvokerMap.size()); this.urlInvokerMap.forEach(oldUrlInvokerMap::put); } Map> newUrlInvokerMap = toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map From 18ce8b71d8147bfe003619a4144ed327bf26ca6d Mon Sep 17 00:00:00 2001 From: zrlw Date: Tue, 21 Sep 2021 11:21:06 +0800 Subject: [PATCH 6/7] change the initial capacity to avoid resizing --- .../registry/client/ServiceDiscoveryRegistryDirectory.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java index 12ab3c4012d..ecf20aea9a0 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java @@ -56,6 +56,7 @@ public class ServiceDiscoveryRegistryDirectory extends DynamicDirectory { private static final Logger logger = LoggerFactory.getLogger(ServiceDiscoveryRegistryDirectory.class); + private static final float DEFAULT_LOAD_FACTOR = 0.75f; /** * instance address to invoker mapping. @@ -256,7 +257,8 @@ private void refreshInvoker(List invokerUrls) { // can't use local reference because this.urlInvokerMap might be accessed at isAvailable() by main thread concurrently. Map> oldUrlInvokerMap = null; if (this.urlInvokerMap != null) { - oldUrlInvokerMap = new LinkedHashMap<>(this.urlInvokerMap.size()); + // the initial capacity should be set greater than the maximum number of entries divided by the load factor to avoid resizing. + oldUrlInvokerMap = new LinkedHashMap<>(Math.round(1 + this.urlInvokerMap.size() / DEFAULT_LOAD_FACTOR)); this.urlInvokerMap.forEach(oldUrlInvokerMap::put); } Map> newUrlInvokerMap = toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map From 8bd04792337ca9d7a8bfac75131a8825e34a0406 Mon Sep 17 00:00:00 2001 From: zrlw Date: Tue, 21 Sep 2021 12:15:39 +0800 Subject: [PATCH 7/7] define DEFAULT_HASHMAP_LOAD_FACTOR constant --- .../org/apache/dubbo/common/constants/RegistryConstants.java | 2 ++ .../registry/client/ServiceDiscoveryRegistryDirectory.java | 4 ++-- .../apache/dubbo/registry/integration/RegistryDirectory.java | 4 +++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/constants/RegistryConstants.java b/dubbo-common/src/main/java/org/apache/dubbo/common/constants/RegistryConstants.java index f76440bd83d..6029293bad0 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/constants/RegistryConstants.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/constants/RegistryConstants.java @@ -130,4 +130,6 @@ public interface RegistryConstants { String REGISTRY_SERVICE_REFERENCE_PATH = "org.apache.dubbo.registry.RegistryService"; String INIT = "INIT"; + + float DEFAULT_HASHMAP_LOAD_FACTOR = 0.75f; } diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java index ecf20aea9a0..ab5c47ff988 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistryDirectory.java @@ -48,6 +48,7 @@ import static org.apache.dubbo.common.constants.CommonConstants.DISABLED_KEY; import static org.apache.dubbo.common.constants.CommonConstants.ENABLED_KEY; +import static org.apache.dubbo.common.constants.RegistryConstants.DEFAULT_HASHMAP_LOAD_FACTOR; import static org.apache.dubbo.common.constants.RegistryConstants.EMPTY_PROTOCOL; import static org.apache.dubbo.common.constants.RegistryConstants.REGISTRY_TYPE_KEY; import static org.apache.dubbo.common.constants.RegistryConstants.SERVICE_REGISTRY_TYPE; @@ -56,7 +57,6 @@ public class ServiceDiscoveryRegistryDirectory extends DynamicDirectory { private static final Logger logger = LoggerFactory.getLogger(ServiceDiscoveryRegistryDirectory.class); - private static final float DEFAULT_LOAD_FACTOR = 0.75f; /** * instance address to invoker mapping. @@ -258,7 +258,7 @@ private void refreshInvoker(List invokerUrls) { Map> oldUrlInvokerMap = null; if (this.urlInvokerMap != null) { // the initial capacity should be set greater than the maximum number of entries divided by the load factor to avoid resizing. - oldUrlInvokerMap = new LinkedHashMap<>(Math.round(1 + this.urlInvokerMap.size() / DEFAULT_LOAD_FACTOR)); + oldUrlInvokerMap = new LinkedHashMap<>(Math.round(1 + this.urlInvokerMap.size() / DEFAULT_HASHMAP_LOAD_FACTOR)); this.urlInvokerMap.forEach(oldUrlInvokerMap::put); } Map> newUrlInvokerMap = toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map diff --git a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java index 1fa48b720fa..d1badc53ba2 100644 --- a/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java +++ b/dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/integration/RegistryDirectory.java @@ -70,6 +70,7 @@ import static org.apache.dubbo.common.constants.RegistryConstants.CONFIGURATORS_CATEGORY; import static org.apache.dubbo.common.constants.RegistryConstants.CONSUMERS_CATEGORY; import static org.apache.dubbo.common.constants.RegistryConstants.DEFAULT_CATEGORY; +import static org.apache.dubbo.common.constants.RegistryConstants.DEFAULT_HASHMAP_LOAD_FACTOR; import static org.apache.dubbo.common.constants.RegistryConstants.DYNAMIC_CONFIGURATORS_CATEGORY; import static org.apache.dubbo.common.constants.RegistryConstants.EMPTY_PROTOCOL; import static org.apache.dubbo.common.constants.RegistryConstants.PROVIDERS_CATEGORY; @@ -227,7 +228,8 @@ private void refreshInvoker(List invokerUrls) { // can't use local reference because this.urlInvokerMap might be accessed at isAvailable() by main thread concurrently. Map> oldUrlInvokerMap = null; if (this.urlInvokerMap != null) { - oldUrlInvokerMap = new LinkedHashMap<>(this.urlInvokerMap.size()); + // the initial capacity should be set greater than the maximum number of entries divided by the load factor to avoid resizing. + oldUrlInvokerMap = new LinkedHashMap<>(Math.round(1 + this.urlInvokerMap.size() / DEFAULT_HASHMAP_LOAD_FACTOR)); this.urlInvokerMap.forEach(oldUrlInvokerMap::put); } Map> newUrlInvokerMap = toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map