Skip to content

Commit

Permalink
De-duplicate the filter returned by the getActivateExtension method (#…
Browse files Browse the repository at this point in the history 
…7600)

* fix #7587 De-duplicate the filter returned by the getActivateExtension method

* add log.

* modify log.

* merge master
  • Loading branch information
@xiaoheng1
xiaoheng1 committed Jun 8, 2021
1 parent c8d176f commit 275ac51
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 24 deletions.
51 changes: 36 additions & 15 deletions dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java
View file
Expand Up @@ -42,6 +42,7 @@
import java.util.Collections;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
Expand Down Expand Up @@ -148,7 +149,8 @@ public static List<LoadingStrategy> getLoadingStrategies() {

private ExtensionLoader(Class<?> type) {
this.type = type;
objectFactory = (type == ExtensionFactory.class ? null : ExtensionLoader.getExtensionLoader(ExtensionFactory.class).getAdaptiveExtension());
objectFactory =
(type == ExtensionFactory.class ? null : ExtensionLoader.getExtensionLoader(ExtensionFactory.class).getAdaptiveExtension());
}

private static <T> boolean withExtensionAnnotation(Class<T> type) {
Expand Down Expand Up @@ -270,6 +272,7 @@ public List<T> getActivateExtension(URL url, String[] values, String group) {
List<T> activateExtensions = new ArrayList<>();
// solve the bug of using @SPI's wrapper method to report a null pointer exception.
TreeMap<Class, T> activateExtensionsMap = new TreeMap<>(ActivateComparator.COMPARATOR);
Set<String> loadedNames = new HashSet<>();
Set<String> names = CollectionUtils.ofSet(values);
if (!names.contains(REMOVE_VALUE_PREFIX + DEFAULT_KEY)) {
getExtensionClasses();
Expand All @@ -291,25 +294,35 @@ public List<T> getActivateExtension(URL url, String[] values, String group) {
if (isMatchGroup(group, activateGroup)
&& !names.contains(name)
&& !names.contains(REMOVE_VALUE_PREFIX + name)
&& isActive(activateValue, url)) {
&& isActive(activateValue, url)
&& !loadedNames.contains(name)) {
activateExtensionsMap.put(getExtensionClass(name), getExtension(name));
loadedNames.add(name);
}
}
if(!activateExtensionsMap.isEmpty()){
if (!activateExtensionsMap.isEmpty()) {
activateExtensions.addAll(activateExtensionsMap.values());
}
}
List<T> loadedExtensions = new ArrayList<>();
for (String name : names) {
if (!name.startsWith(REMOVE_VALUE_PREFIX)
&& !names.contains(REMOVE_VALUE_PREFIX + name)) {
if (DEFAULT_KEY.equals(name)) {
if (!loadedExtensions.isEmpty()) {
activateExtensions.addAll(0, loadedExtensions);
loadedExtensions.clear();
if (!loadedNames.contains(name)) {
if (DEFAULT_KEY.equals(name)) {
if (!loadedExtensions.isEmpty()) {
activateExtensions.addAll(0, loadedExtensions);
loadedExtensions.clear();
}
} else {
loadedExtensions.add(getExtension(name));
}
loadedNames.add(name);
} else {
loadedExtensions.add(getExtension(name));
// If getExtension(name) exists, getExtensionClass(name) must exist, so there is no null pointer processing here.
String simpleName = getExtensionClass(name).getSimpleName();
logger.warn("Catch duplicated filter, ExtensionLoader will ignore one of them. Please check. Filter Name: " + name +
". Ignored Class Name: " + simpleName);
}
}
}
Expand Down Expand Up @@ -785,8 +798,10 @@ private Map<String, Class<?>> loadExtensionClasses() {
Map<String, Class<?>> extensionClasses = new HashMap<>();

for (LoadingStrategy strategy : strategies) {
loadDirectory(extensionClasses, strategy.directory(), type.getName(), strategy.preferExtensionClassLoader(), strategy.overridden(), strategy.excludedPackages());
loadDirectory(extensionClasses, strategy.directory(), type.getName().replace("org.apache", "com.alibaba"), strategy.preferExtensionClassLoader(), strategy.overridden(), strategy.excludedPackages());
loadDirectory(extensionClasses, strategy.directory(), type.getName(), strategy.preferExtensionClassLoader(),
strategy.overridden(), strategy.excludedPackages());
loadDirectory(extensionClasses, strategy.directory(), type.getName().replace("org.apache", "com.alibaba"),
strategy.preferExtensionClassLoader(), strategy.overridden(), strategy.excludedPackages());
}

return extensionClasses;
Expand Down Expand Up @@ -879,7 +894,9 @@ private void loadResource(Map<String, Class<?>> extensionClasses, ClassLoader cl
loadClass(extensionClasses, resourceURL, Class.forName(clazz, true, classLoader), name, overridden);
}
} catch (Throwable t) {
IllegalStateException e = new IllegalStateException("Failed to load extension class (interface: " + type + ", class line: " + line + ") in " + resourceURL + ", cause: " + t.getMessage(), t);
IllegalStateException e = new IllegalStateException(
"Failed to load extension class (interface: " + type + ", class line: " + line + ") in " + resourceURL +
", cause: " + t.getMessage(), t);
exceptions.put(line, e);
}
}
Expand Down Expand Up @@ -918,7 +935,8 @@ private void loadClass(Map<String, Class<?>> extensionClasses, java.net.URL reso
if (StringUtils.isEmpty(name)) {
name = findAnnotationName(clazz);
if (name.length() == 0) {
throw new IllegalStateException("No such extension name for the class " + clazz.getName() + " in the config " + resourceURL);
throw new IllegalStateException(
"No such extension name for the class " + clazz.getName() + " in the config " + resourceURL);
}
}

Expand Down Expand Up @@ -952,7 +970,8 @@ private void saveInExtensionClass(Map<String, Class<?>> extensionClasses, Class<
} else if (c != clazz) {
// duplicate implementation is unacceptable
unacceptableExceptions.add(name);
String duplicateMsg = "Duplicate extension " + type.getName() + " name " + name + " on " + c.getName() + " and " + clazz.getName();
String duplicateMsg =
"Duplicate extension " + type.getName() + " name " + name + " on " + c.getName() + " and " + clazz.getName();
logger.error(duplicateMsg);
throw new IllegalStateException(duplicateMsg);
}
Expand All @@ -969,7 +988,8 @@ private void cacheActivateClass(Class<?> clazz, String name) {
cachedActivates.put(name, activate);
} else {
// support com.alibaba.dubbo.common.extension.Activate
com.alibaba.dubbo.common.extension.Activate oldActivate = clazz.getAnnotation(com.alibaba.dubbo.common.extension.Activate.class);
com.alibaba.dubbo.common.extension.Activate oldActivate =
clazz.getAnnotation(com.alibaba.dubbo.common.extension.Activate.class);
if (oldActivate != null) {
cachedActivates.put(name, oldActivate);
}
Expand Down Expand Up @@ -1049,7 +1069,8 @@ private Class<?> getAdaptiveExtensionClass() {
private Class<?> createAdaptiveExtensionClass() {
String code = new AdaptiveClassCodeGenerator(type, cachedDefaultName).generate();
ClassLoader classLoader = findClassLoader();
org.apache.dubbo.common.compiler.Compiler compiler = ExtensionLoader.getExtensionLoader(org.apache.dubbo.common.compiler.Compiler.class).getAdaptiveExtension();
org.apache.dubbo.common.compiler.Compiler compiler =
ExtensionLoader.getExtensionLoader(org.apache.dubbo.common.compiler.Compiler.class).getAdaptiveExtension();
return compiler.compile(code, classLoader);
}

Expand Down
39 changes: 30 additions & 9 deletions dubbo-common/src/test/java/org/apache/dubbo/common/extension/ExtensionLoaderTest.java
View file
Expand Up @@ -174,7 +174,8 @@ public void test_getExtension_ExceptionNoExtension() throws Exception {
getExtensionLoader(SimpleExt.class).getExtension("XXX");
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("No such extension org.apache.dubbo.common.extension.ext1.SimpleExt by name XXX"));
assertThat(expected.getMessage(),
containsString("No such extension org.apache.dubbo.common.extension.ext1.SimpleExt by name XXX"));
}
}

Expand All @@ -184,7 +185,8 @@ public void test_getExtension_ExceptionNoExtension_WrapperNotAffactName() throws
getExtensionLoader(WrappedExt.class).getExtension("XXX");
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("No such extension org.apache.dubbo.common.extension.ext6_wrap.WrappedExt by name XXX"));
assertThat(expected.getMessage(),
containsString("No such extension org.apache.dubbo.common.extension.ext6_wrap.WrappedExt by name XXX"));
}
}

Expand Down Expand Up @@ -257,7 +259,8 @@ public void test_AddExtension() throws Exception {
getExtensionLoader(AddExt1.class).getExtension("Manual1");
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("No such extension org.apache.dubbo.common.extension.ext8_add.AddExt1 by name Manual"));
assertThat(expected.getMessage(),
containsString("No such extension org.apache.dubbo.common.extension.ext8_add.AddExt1 by name Manual"));
}

getExtensionLoader(AddExt1.class).addExtension("Manual1", AddExt1_ManualAdd1.class);
Expand Down Expand Up @@ -286,7 +289,8 @@ public void test_AddExtension_ExceptionWhenExistedExtension() throws Exception {
getExtensionLoader(AddExt1.class).addExtension("impl1", AddExt1_ManualAdd1.class);
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("Extension name impl1 already exists (Extension interface org.apache.dubbo.common.extension.ext8_add.AddExt1)!"));
assertThat(expected.getMessage(), containsString(
"Extension name impl1 already exists (Extension interface org.apache.dubbo.common.extension.ext8_add.AddExt1)!"));
}
}

Expand All @@ -309,7 +313,8 @@ public void test_AddExtension_Adaptive_ExceptionWhenExistedAdaptive() throws Exc
loader.addExtension(null, AddExt1_ManualAdaptive.class);
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("Adaptive Extension already exists (Extension interface org.apache.dubbo.common.extension.ext8_add.AddExt1)!"));
assertThat(expected.getMessage(), containsString(
"Adaptive Extension already exists (Extension interface org.apache.dubbo.common.extension.ext8_add.AddExt1)!"));
}
}

Expand All @@ -319,7 +324,8 @@ public void test_replaceExtension() throws Exception {
getExtensionLoader(AddExt1.class).getExtension("Manual2");
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("No such extension org.apache.dubbo.common.extension.ext8_add.AddExt1 by name Manual"));
assertThat(expected.getMessage(),
containsString("No such extension org.apache.dubbo.common.extension.ext8_add.AddExt1 by name Manual"));
}

{
Expand Down Expand Up @@ -360,7 +366,8 @@ public void test_replaceExtension_ExceptionWhenNotExistedExtension() throws Exce
getExtensionLoader(AddExt1.class).replaceExtension("NotExistedExtension", AddExt1_ManualAdd1.class);
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("Extension name NotExistedExtension doesn't exist (Extension interface org.apache.dubbo.common.extension.ext8_add.AddExt1)"));
assertThat(expected.getMessage(), containsString(
"Extension name NotExistedExtension doesn't exist (Extension interface org.apache.dubbo.common.extension.ext8_add.AddExt1)"));
}
}

Expand All @@ -372,7 +379,8 @@ public void test_replaceExtension_Adaptive_ExceptionWhenNotExistedExtension() th
loader.replaceExtension(null, AddExt4_ManualAdaptive.class);
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("Adaptive Extension doesn't exist (Extension interface org.apache.dubbo.common.extension.ext8_add.AddExt4)"));
assertThat(expected.getMessage(), containsString(
"Adaptive Extension doesn't exist (Extension interface org.apache.dubbo.common.extension.ext8_add.AddExt4)"));
}
}

Expand All @@ -386,11 +394,24 @@ public void test_InitError() throws Exception {
loader.getExtension("error");
fail();
} catch (IllegalStateException expected) {
assertThat(expected.getMessage(), containsString("Failed to load extension class (interface: interface org.apache.dubbo.common.extension.ext7.InitErrorExt"));
assertThat(expected.getMessage(), containsString(
"Failed to load extension class (interface: interface org.apache.dubbo.common.extension.ext7.InitErrorExt"));
assertThat(expected.getMessage(), containsString("java.lang.ExceptionInInitializerError"));
}
}

@Test
public void testLoadActivateExtensionWithNoRepeat() {
URL url = URL.valueOf("test://localhost/test");
List<ActivateExt1> list = getExtensionLoader(ActivateExt1.class)
.getActivateExtension(url, new String[] {"old1", "old2"}, "default_group");
Assertions.assertEquals(3, list.size());

list = getExtensionLoader(ActivateExt1.class)
.getActivateExtension(url, new String[] {"old1", "old2", "old1"}, "default_group");
Assertions.assertEquals(3, list.size());
}

@Test
public void testLoadActivateExtension() throws Exception {
// test default
Expand Down

0 comments on commit 275ac51

Please sign in to comment.