This is an automated email from the ASF dual-hosted git repository.
albumenj pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/dubbo.git
The following commit(s) were added to refs/heads/3.0 by this push:
new 47e4b81 [3.0] fix urlInvokerMap concurrent access issue (#8701)
47e4b81 is described below
commit 47e4b813f096e8e0aed560396bb530ce5be6ff24
Author: zrlw <[email protected]>
AuthorDate: Wed Sep 22 00:08:04 2021 +0800
[3.0] fix urlInvokerMap concurrent access issue (#8701)
* fix localUrlInvokerMap concurrent access problem
* add forbidden check at isAvailable()
* fix RegistryDirectory localUrlInvokerMap concurrent access problem
* format code
* change oldUrlInvokerMap to LinkedHashMap and set default size
* change the initial capacity to avoid resizing
* define DEFAULT_HASHMAP_LOAD_FACTOR constant
---
.../dubbo/common/constants/RegistryConstants.java | 2 ++
.../client/ServiceDiscoveryRegistryDirectory.java | 22 +++++++++++++++------
.../registry/integration/RegistryDirectory.java | 23 ++++++++++++++++------
3 files changed, 35 insertions(+), 12 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 f76440b..6029293 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 4590355..ab5c47f 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
@@ -41,12 +41,14 @@ import java.util.ArrayList;
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;
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;
@@ -126,7 +128,7 @@ public class ServiceDiscoveryRegistryDirectory<T> extends
DynamicDirectory<T> {
@Override
public boolean isAvailable() {
- if (isDestroyed()) {
+ if (isDestroyed() || this.forbidden) {
return false;
}
Map<String, Invoker<T>> localUrlInvokerMap = urlInvokerMap;
@@ -248,12 +250,18 @@ public class ServiceDiscoveryRegistryDirectory<T> extends
DynamicDirectory<T> {
destroyAllInvokers(); // Close all invokers
} else {
this.forbidden = false; // Allow accessing
- Map<String, Invoker<T>> oldUrlInvokerMap = this.urlInvokerMap; //
local reference
if (CollectionUtils.isEmpty(invokerUrls)) {
return;
}
- Map<String, Invoker<T>> 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<String, Invoker<T>> 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_HASHMAP_LOAD_FACTOR));
+ this.urlInvokerMap.forEach(oldUrlInvokerMap::put);
+ }
+ Map<String, Invoker<T>> newUrlInvokerMap =
toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map
logger.info("Refreshed invoker size " + newUrlInvokerMap.size());
if (CollectionUtils.isEmptyMap(newUrlInvokerMap)) {
@@ -282,11 +290,13 @@ public class ServiceDiscoveryRegistryDirectory<T> extends
DynamicDirectory<T> {
/**
* 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<String, Invoker<T>> toInvokers(List<URL> urls) {
+ private Map<String, Invoker<T>> toInvokers(Map<String, Invoker<T>>
oldUrlInvokerMap, List<URL> urls) {
Map<String, Invoker<T>> newUrlInvokerMap = new HashMap<>();
if (urls == null || urls.isEmpty()) {
return newUrlInvokerMap;
@@ -311,7 +321,7 @@ public class ServiceDiscoveryRegistryDirectory<T> extends
DynamicDirectory<T> {
instanceAddressURL =
overrideWithConfigurator(instanceAddressURL);
}
- Invoker<T> invoker = urlInvokerMap == null ? null :
urlInvokerMap.get(instanceAddressURL.getAddress());
+ Invoker<T> invoker = oldUrlInvokerMap == null ? null :
oldUrlInvokerMap.get(instanceAddressURL.getAddress());
if (invoker == null || urlChanged(invoker, instanceAddressURL)) {
// Not in the cache, refer again
try {
boolean enabled = true;
@@ -331,7 +341,7 @@ public class ServiceDiscoveryRegistryDirectory<T> extends
DynamicDirectory<T> {
}
} else {
newUrlInvokerMap.put(instanceAddressURL.getAddress(), invoker);
- urlInvokerMap.remove(instanceAddressURL.getAddress(), invoker);
+ oldUrlInvokerMap.remove(instanceAddressURL.getAddress(),
invoker);
}
}
return newUrlInvokerMap;
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 b39aa9a..d1badc5 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
@@ -48,6 +48,7 @@ import java.util.ArrayList;
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;
@@ -69,6 +70,7 @@ import static
org.apache.dubbo.common.constants.RegistryConstants.COMPATIBLE_CON
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;
@@ -209,7 +211,7 @@ public class RegistryDirectory<T> extends
DynamicDirectory<T> {
destroyAllInvokers(); // Close all invokers
} else {
this.forbidden = false; // Allow to access
- Map<URL, Invoker<T>> oldUrlInvokerMap = this.urlInvokerMap; //
local reference
+
if (invokerUrls == Collections.<URL>emptyList()) {
invokerUrls = new ArrayList<>();
}
@@ -222,7 +224,15 @@ public class RegistryDirectory<T> extends
DynamicDirectory<T> {
if (invokerUrls.isEmpty()) {
return;
}
- Map<URL, Invoker<T>> 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<URL, Invoker<T>> 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_HASHMAP_LOAD_FACTOR));
+ this.urlInvokerMap.forEach(oldUrlInvokerMap::put);
+ }
+ Map<URL, Invoker<T>> newUrlInvokerMap =
toInvokers(oldUrlInvokerMap, invokerUrls);// Translate url list to Invoker map
/**
* If the calculation is wrong, it is not processed.
@@ -313,11 +323,13 @@ public class RegistryDirectory<T> extends
DynamicDirectory<T> {
/**
* 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<URL, Invoker<T>> toInvokers(List<URL> urls) {
+ private Map<URL, Invoker<T>> toInvokers(Map<URL, Invoker<T>>
oldUrlInvokerMap, List<URL> urls) {
Map<URL, Invoker<T>> newUrlInvokerMap = new ConcurrentHashMap<>();
if (urls == null || urls.isEmpty()) {
return newUrlInvokerMap;
@@ -351,8 +363,7 @@ public class RegistryDirectory<T> extends
DynamicDirectory<T> {
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<URL, Invoker<T>> localUrlInvokerMap = this.urlInvokerMap; //
local reference
- Invoker<T> invoker = localUrlInvokerMap == null ? null :
localUrlInvokerMap.remove(url);
+ Invoker<T> invoker = oldUrlInvokerMap == null ? null :
oldUrlInvokerMap.remove(url);
if (invoker == null) { // Not in the cache, refer again
try {
boolean enabled = true;
@@ -567,7 +578,7 @@ public class RegistryDirectory<T> extends
DynamicDirectory<T> {
@Override
public boolean isAvailable() {
- if (isDestroyed()) {
+ if (isDestroyed() || this.forbidden) {
return false;
}
Map<URL, Invoker<T>> localUrlInvokerMap = urlInvokerMap;
