From e133238ba35b05b6b2271cace6cbc13e9a71b1f0 Mon Sep 17 00:00:00 2001 From: xiaoheng1 <2018154970@qq.com> Date: Thu, 22 Apr 2021 10:19:23 +0800 Subject: [PATCH 1/4] fix #7587 De-duplicate the filter returned by the getActivateExtension method --- .../common/extension/ExtensionLoader.java | 10 +++- .../common/extension/ExtensionLoaderTest.java | 51 +++++++++++++------ 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java b/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java index 88c12814e88..f1a6c1bc45e 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java @@ -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; @@ -263,6 +264,7 @@ public List getActivateExtension(URL url, String[] values, String group) { // solve the bug of using @SPI's wrapper method to report a null pointer exception. TreeMap activateExtensionsMap = new TreeMap<>(ActivateComparator.COMPARATOR); List names = values == null ? new ArrayList<>(0) : asList(values); + Set loadedNames = new HashSet<>(); if (!names.contains(REMOVE_VALUE_PREFIX + DEFAULT_KEY)) { getExtensionClasses(); for (Map.Entry entry : cachedActivates.entrySet()) { @@ -283,8 +285,10 @@ public List 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()){ @@ -295,7 +299,8 @@ && isActive(activateValue, url)) { for (int i = 0; i < names.size(); i++) { String name = names.get(i); if (!name.startsWith(REMOVE_VALUE_PREFIX) - && !names.contains(REMOVE_VALUE_PREFIX + name)) { + && !names.contains(REMOVE_VALUE_PREFIX + name) + && !loadedNames.contains(name)) { if (DEFAULT_KEY.equals(name)) { if (!loadedExtensions.isEmpty()) { activateExtensions.addAll(0, loadedExtensions); @@ -304,6 +309,7 @@ && isActive(activateValue, url)) { } else { loadedExtensions.add(getExtension(name)); } + loadedNames.add(name); } } if (!loadedExtensions.isEmpty()) { diff --git a/dubbo-common/src/test/java/org/apache/dubbo/common/extension/ExtensionLoaderTest.java b/dubbo-common/src/test/java/org/apache/dubbo/common/extension/ExtensionLoaderTest.java index 5d01d9da8c0..3bccfbb0ed1 100644 --- a/dubbo-common/src/test/java/org/apache/dubbo/common/extension/ExtensionLoaderTest.java +++ b/dubbo-common/src/test/java/org/apache/dubbo/common/extension/ExtensionLoaderTest.java @@ -164,7 +164,7 @@ public void test_getExtension_WithWrapper() throws Exception { public void test_getActivateExtension_WithWrapper() throws Exception { URL url = URL.valueOf("test://localhost/test"); List list = getExtensionLoader(ActivateWrapperExt1.class) - .getActivateExtension(url, new String[]{}, "order"); + .getActivateExtension(url, new String[] {}, "order"); assertEquals(2, list.size()); } @@ -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")); } } @@ -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")); } } @@ -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); @@ -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)!")); } } @@ -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)!")); } } @@ -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")); } { @@ -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)")); } } @@ -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)")); } } @@ -386,31 +394,44 @@ 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 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 URL url = URL.valueOf("test://localhost/test"); List list = getExtensionLoader(ActivateExt1.class) - .getActivateExtension(url, new String[]{}, "default_group"); + .getActivateExtension(url, new String[] {}, "default_group"); Assertions.assertEquals(1, list.size()); Assertions.assertSame(list.get(0).getClass(), ActivateExt1Impl1.class); // test group url = url.addParameter(GROUP_KEY, "group1"); list = getExtensionLoader(ActivateExt1.class) - .getActivateExtension(url, new String[]{}, "group1"); + .getActivateExtension(url, new String[] {}, "group1"); Assertions.assertEquals(1, list.size()); Assertions.assertSame(list.get(0).getClass(), GroupActivateExtImpl.class); // test old @Activate group url = url.addParameter(GROUP_KEY, "old_group"); list = getExtensionLoader(ActivateExt1.class) - .getActivateExtension(url, new String[]{}, "old_group"); + .getActivateExtension(url, new String[] {}, "old_group"); Assertions.assertEquals(2, list.size()); Assertions.assertTrue(list.get(0).getClass() == OldActivateExt1Impl2.class || list.get(0).getClass() == OldActivateExt1Impl3.class); @@ -420,7 +441,7 @@ public void testLoadActivateExtension() throws Exception { url = url.addParameter(GROUP_KEY, "value"); url = url.addParameter("value", "value"); list = getExtensionLoader(ActivateExt1.class) - .getActivateExtension(url, new String[]{}, "value"); + .getActivateExtension(url, new String[] {}, "value"); Assertions.assertEquals(1, list.size()); Assertions.assertSame(list.get(0).getClass(), ValueActivateExtImpl.class); @@ -428,7 +449,7 @@ public void testLoadActivateExtension() throws Exception { url = URL.valueOf("test://localhost/test"); url = url.addParameter(GROUP_KEY, "order"); list = getExtensionLoader(ActivateExt1.class) - .getActivateExtension(url, new String[]{}, "order"); + .getActivateExtension(url, new String[] {}, "order"); Assertions.assertEquals(2, list.size()); Assertions.assertSame(list.get(0).getClass(), OrderActivateExtImpl1.class); Assertions.assertSame(list.get(1).getClass(), OrderActivateExtImpl2.class); From 55df4788162f60dbb7bac0f73c65607fa7adf9d8 Mon Sep 17 00:00:00 2001 From: xiaoheng1 <2018154970@qq.com> Date: Sun, 23 May 2021 15:57:41 +0800 Subject: [PATCH 2/4] add log. --- .../common/extension/ExtensionLoader.java | 46 ++++++++++++------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java b/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java index a10ff6376a4..301c9b22f67 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java @@ -149,7 +149,8 @@ public static List 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 boolean withExtensionAnnotation(Class type) { @@ -299,7 +300,7 @@ && isActive(activateValue, url) loadedNames.add(name); } } - if(!activateExtensionsMap.isEmpty()){ + if (!activateExtensionsMap.isEmpty()) { activateExtensions.addAll(activateExtensionsMap.values()); } } @@ -307,17 +308,20 @@ && isActive(activateValue, url) for (int i = 0; i < names.size(); i++) { String name = names.get(i); if (!name.startsWith(REMOVE_VALUE_PREFIX) - && !names.contains(REMOVE_VALUE_PREFIX + name) - && !loadedNames.contains(name)) { - if (DEFAULT_KEY.equals(name)) { - if (!loadedExtensions.isEmpty()) { - activateExtensions.addAll(0, loadedExtensions); - loadedExtensions.clear(); + && !names.contains(REMOVE_VALUE_PREFIX + name)) { + 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)); + logger.warn("name " + name + " filter has been duplicated, we will ignore the duplicated filter."); } - loadedNames.add(name); } } if (!loadedExtensions.isEmpty()) { @@ -792,8 +796,10 @@ private Map> loadExtensionClasses() { Map> 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; @@ -886,7 +892,9 @@ private void loadResource(Map> 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); } } @@ -925,7 +933,8 @@ private void loadClass(Map> 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); } } @@ -959,7 +968,8 @@ private void saveInExtensionClass(Map> 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); } @@ -976,7 +986,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); } @@ -1056,7 +1067,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); } From c9862613ababf4e9deeceae9e17e63600e435221 Mon Sep 17 00:00:00 2001 From: xiaoheng1 <2018154970@qq.com> Date: Sun, 23 May 2021 22:07:47 +0800 Subject: [PATCH 3/4] modify log. --- .../org/apache/dubbo/common/extension/ExtensionLoader.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java b/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java index 301c9b22f67..d229820ca3e 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java @@ -320,7 +320,10 @@ && isActive(activateValue, url) } loadedNames.add(name); } else { - logger.warn("name " + name + " filter has been duplicated, we will ignore the duplicated filter."); + // 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); } } } From ff41121a0ffc9c07173a4cef4df76ba2246be8e1 Mon Sep 17 00:00:00 2001 From: xiaoheng1 <2018154970@qq.com> Date: Mon, 7 Jun 2021 23:30:29 +0800 Subject: [PATCH 4/4] merge master --- .../java/org/apache/dubbo/common/extension/ExtensionLoader.java | 1 - 1 file changed, 1 deletion(-) diff --git a/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java b/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java index f88ae9e41f9..d85a34cf1b3 100644 --- a/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java +++ b/dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java @@ -272,7 +272,6 @@ public List getActivateExtension(URL url, String[] values, String group) { List activateExtensions = new ArrayList<>(); // solve the bug of using @SPI's wrapper method to report a null pointer exception. TreeMap activateExtensionsMap = new TreeMap<>(ActivateComparator.COMPARATOR); - List names = values == null ? new ArrayList<>(0) : asList(values); Set loadedNames = new HashSet<>(); Set names = CollectionUtils.ofSet(values); if (!names.contains(REMOVE_VALUE_PREFIX + DEFAULT_KEY)) {