Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

De-duplicate the filter returned by the getActivateExtension method #7600

Merged
merged 6 commits into from Jun 8, 2021
Merged

De-duplicate the filter returned by the getActivateExtension method #7600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e133238
fix #7587 De-duplicate the filter returned by the getActivateExtensio…
xiaoheng1 Apr 22, 2021
22ef71f
Merge remote-tracking branch 'dubbo/master' into up-dev-issue#7587
xiaoheng1 May 22, 2021
55df478
add log.
xiaoheng1 May 23, 2021
c986261
modify log.
xiaoheng1 May 23, 2021
10d674c
Merge branch 'dubbo-master' into up-dev-issue#7587
xiaoheng1 Jun 7, 2021
ff41121
merge master
xiaoheng1 Jun 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 36 additions & 15 deletions dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java
View file
Open in desktop
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
Open in desktop
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