Copilot commented on code in PR #7092:
URL: https://github.com/apache/shenyu/pull/7092#discussion_r4032850280
##########
shenyu-spi/src/main/java/org/apache/shenyu/spi/ExtensionLoader.java:
##########
@@ -50,7 +50,7 @@ public final class ExtensionLoader<T> {
private static final String SHENYU_DIRECTORY = "META-INF/shenyu/";
- private static final Map<Class<?>, ExtensionLoader<?>> LOADERS = new
ConcurrentHashMap<>();
+ private static final Map<LoaderKey, ExtensionLoader<?>> LOADERS = new
ConcurrentHashMap<>();
Review Comment:
This static map now strongly retains every requesting plugin class loader
through both `LoaderKey` and `ExtensionLoader`. The hot-plugin flow
replaces/removes and closes `ShenyuPluginClassLoader` instances
(`ShenyuPluginClassLoaderHolder.java:51-68`), but nothing removes their SPI
loaders, so every reload permanently retains the old class loader, loaded
classes, and extension instances. Add an explicit class-loader eviction hook
called during plugin-loader close/removal, or use a cache design whose keys and
values do not prevent class-loader collection.
##########
shenyu-spi/src/test/java/org/apache/shenyu/spi/ExtensionLoaderClassLoaderTest.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shenyu.spi;
+
+import org.apache.shenyu.spi.fixture.JdbcSPI;
+import org.apache.shenyu.spi.fixture.MysqlSPI;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.IOException;
+import java.net.URLClassLoader;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
+
+class ExtensionLoaderClassLoaderTest {
+
+ @TempDir
+ private Path directory;
+
+ @Test
+ void shouldDiscoverExtensionsFromEachClassLoader() throws IOException {
+ Path serviceFile = directory.resolve("META-INF/shenyu/" +
JdbcSPI.class.getName());
+ Files.createDirectories(serviceFile.getParent());
+ Files.write(serviceFile, ("plugin=" +
MysqlSPI.class.getName()).getBytes(StandardCharsets.UTF_8));
+ ClassLoader applicationClassLoader =
ExtensionLoaderClassLoaderTest.class.getClassLoader();
+
+ try (URLClassLoader pluginClassLoader = new URLClassLoader(new
java.net.URL[]{directory.toUri().toURL()}, applicationClassLoader)) {
Review Comment:
The regression substitutes `URLClassLoader`, whose `getResources` enumerates
its URLs, for ShenYu's actual hot-plugin loader. `ShenyuPluginClassLoader`
keeps parsed JAR resources in memory and only overrides `getResourceAsStream`
(`ShenyuPluginClassLoader.java:103-115`), while `ExtensionLoader.loadDirectory`
calls `getResources`; consequently production hot-deployed `META-INF/shenyu`
entries still are not discovered even though this test passes. Exercise the
real loader and add resource-enumeration support (or make `ExtensionLoader` use
the supported resource API).
##########
shenyu-spi/src/main/java/org/apache/shenyu/spi/ExtensionLoader.java:
##########
@@ -99,12 +99,8 @@ public static <T> ExtensionLoader<T>
getExtensionLoader(final Class<T> clazz, fi
if (!clazz.isAnnotationPresent(SPI.class)) {
throw new IllegalArgumentException("extension clazz (" + clazz +
") without @" + SPI.class + " Annotation");
}
- ExtensionLoader<T> extensionLoader = (ExtensionLoader<T>)
LOADERS.get(clazz);
- if (Objects.nonNull(extensionLoader)) {
- return extensionLoader;
- }
- LOADERS.putIfAbsent(clazz, new ExtensionLoader<>(clazz, cl));
- return (ExtensionLoader<T>) LOADERS.get(clazz);
+ LoaderKey key = new LoaderKey(clazz, cl);
+ return (ExtensionLoader<T>) LOADERS.computeIfAbsent(key, ignored ->
new ExtensionLoader<>(clazz, cl));
Review Comment:
`computeIfAbsent` invokes the `ExtensionLoader` constructor while this map
is being computed, but that constructor calls
`getExtensionLoader(ExtensionFactory.class)` and can update `LOADERS` again
(line 80). `ConcurrentHashMap` forbids recursive map updates; when the two keys
land in the same bin, the first lookup throws `IllegalStateException: Recursive
update`. Keep construction outside the mapping callback, as the previous
`putIfAbsent` pattern did.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
