Copilot commented on code in PR #7089:
URL: https://github.com/apache/shenyu/pull/7089#discussion_r4032826171
##########
shenyu-web/src/main/java/org/apache/shenyu/web/loader/ShenyuPluginClassLoaderHolder.java:
##########
@@ -51,10 +51,10 @@ public static ShenyuPluginClassLoaderHolder getSingleton() {
public ShenyuPluginClassLoader createPluginClassLoader(final
PluginJarParser.PluginJar pluginJar) {
ShenyuPluginClassLoader shenyuPluginClassLoader = new
ShenyuPluginClassLoader(pluginJar);
String jarKey =
Optional.ofNullable(pluginJar.getAbsolutePath()).orElse(pluginJar.getJarKey());
- if (pluginCache.containsKey(jarKey)) {
- pluginCache.remove(jarKey).close();
- }
- pluginCache.put(jarKey, shenyuPluginClassLoader);
+ pluginCache.compute(jarKey, (key, previous) -> {
+
Optional.ofNullable(previous).ifPresent(ShenyuPluginClassLoader::close);
+ return shenyuPluginClassLoader;
+ });
return shenyuPluginClassLoader;
Review Comment:
`compute` serializes only the map mutation; it does not protect the loader
after the callback returns. Another creator can immediately enter `compute`,
close this loader, and replace it before this method returns or before
`ShenyuLoaderService` calls `loadUploadedJarPlugins()` (lines 84-85/90-91), so
the reported race can still return and use a displaced loader. The per-key
critical section needs to cover creation, plugin loading, and activation (or
provide an equivalent lease/version check), not just the cache write.
##########
shenyu-web/src/test/java/org/apache/shenyu/web/loader/ShenyuPluginClassLoaderHolderTest.java:
##########
@@ -54,4 +67,57 @@ public void removePluginClassLoader() {
ShenyuPluginClassLoaderHolder singleton =
ShenyuPluginClassLoaderHolder.getSingleton();
singleton.removePluginClassLoader("testKey");
}
+
+ @Test
+ public void createPluginClassLoaderAtomicallyClosesEveryDisplacedLoader()
throws InterruptedException {
+ int threadCount = 32;
+ String jarKey = "concurrent-test-key";
+ CountingBeanFactory beanFactory = new CountingBeanFactory();
+ ConfigurableApplicationContext context =
mock(ConfigurableApplicationContext.class);
+ when(context.getBeanFactory()).thenReturn(beanFactory);
+ SpringBeanUtils.getInstance().setApplicationContext(context);
+ PluginJarParser.PluginJar concurrentPluginJar =
mock(PluginJarParser.PluginJar.class);
+ when(concurrentPluginJar.getAbsolutePath()).thenReturn(jarKey);
+
when(concurrentPluginJar.getClazzMap()).thenReturn(Collections.singletonMap("sample.Plugin",
new byte[0]));
+ ExecutorService executor = Executors.newFixedThreadPool(threadCount);
+ CountDownLatch ready = new CountDownLatch(threadCount);
+ CountDownLatch start = new CountDownLatch(1);
+
+ List<Runnable> tasks = Collections.nCopies(threadCount, () -> {
+ ready.countDown();
+ try {
+ start.await();
+
ShenyuPluginClassLoaderHolder.getSingleton().createPluginClassLoader(concurrentPluginJar);
+ } catch (InterruptedException ex) {
+ Thread.currentThread().interrupt();
+ }
+ });
+ tasks.forEach(executor::submit);
+ assertTrue(ready.await(5, TimeUnit.SECONDS));
+ start.countDown();
+ executor.shutdown();
+ assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS));
+
+ assertEquals(threadCount - 1, beanFactory.destroyCount.get());
+
ShenyuPluginClassLoaderHolder.getSingleton().removePluginClassLoader(jarKey);
Review Comment:
The submitted `Future`s are discarded, so exceptions from
`createPluginClassLoader` are swallowed and `awaitTermination` can still let
this regression test pass. Also, an assertion timeout before cleanup can leave
all pool threads blocked on `start.await()`. Collect and inspect every future,
and release/shut down resources in `finally`.
--
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]
