Skip to content

Feature: Refactor plugin loading #85

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

Open
infeo wants to merge 6 commits into
base: develop
Choose a base branch
from
+155 −23
Open

Feature: Refactor plugin loading #85

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
168cd1c
Only search directories, if pluginDir property is set and valid
infeo Mar 10, 2026
8b5b3ce
Cache URLClassLoader if created with system property
infeo Mar 10, 2026
95cff98
improve logging
infeo Mar 10, 2026
3d11f8e
simplify caching
infeo Mar 10, 2026
5d1ddb8
Return new ClassLoader if property changes
infeo Mar 11, 2026
2613fc5
Rework cache impl
infeo Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 45 additions & 10 deletions src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.cryptomator.integrations.common;

import org.jetbrains.annotations.Contract;
import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.VisibleForTesting;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -13,15 +14,37 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Objects;
import java.util.stream.Collectors;

class ClassLoaderFactory {

private static final Logger LOG = LoggerFactory.getLogger(ClassLoaderFactory.class);
private static final String USER_HOME = System.getProperty("user.home");
private static final String PLUGIN_DIR_KEY = "cryptomator.pluginDir";
private static final String JAR_SUFFIX = ".jar";

static Cache CACHE = null;

record Cache(@Nullable String pluginDir, URLClassLoader classLoader) {
}

/**
* Returns the cached class loader instance.
* <p>
* The returned instance does not recheck the pluginDir for updates.
* If no instance is cached or the system property changed, creates a new instance with {@link #forPluginDir()} and caches it.
*
* @return The cached URLClassLoader that is aware of all {@code .jar} files in the plugin dir at the creation time of the instance
*/
@Contract(value = "-> _", pure = false)
public synchronized static URLClassLoader forPluginDirFromCache() {
String currentPluginDir = System.getProperty(PLUGIN_DIR_KEY);
if (CACHE == null || !Objects.equals(CACHE.pluginDir, currentPluginDir)) {
CACHE = new Cache(currentPluginDir, forPluginDirInternal(currentPluginDir));
}
return CACHE.classLoader;
Comment on lines +40 to +45
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java | head -100

Repository: cryptomator/integrations-api

Length of output: 4342

🏁 Script executed:

grep -n "CACHE\|close\|URLClassLoader" src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java

Repository: cryptomator/integrations-api

Length of output: 1069

🌐 Web query:

URLClassLoader close resource leak semantics Java

💡 Result:

  • What URLClassLoader.close() is specified to do: It closes the class loader so it can no longer load new classes/resources, while parent-loader classes/resources remain accessible; and for jar: / file: URLs it closes any files it opened (e.g., JarFile handles). Calling close() more than once is a no-op. [1]
  • Why this matters (typical “leak” symptom): On platforms like Windows, an unclosed JarFile handle commonly shows up as an inability to delete/replace the JAR until the loader (and any streams) are closed. Oracle explicitly documents Java 7’s close() as the mechanism to allow replacing an updated JAR by closing the old loader and creating a new one. [2]
  • Historical/edge-case leak semantics: There have been real JDK bugs where URLClassLoader.close() did not close all underlying JAR/resource handles in some resource-loading paths (notably involving getResource* / getResourceAsStream), leaving JARs locked even after close()—tracked in issues like JDK-7183373 and related bugs. [3]
  • “It should close, but it can still leak” in practice: Even in modern JDKs, fixes continue in the underlying URL/JAR loading infrastructure (e.g., jdk.internal.loader.URLClassPath leaking JarFile instances in certain manifest Class-Path scenarios—JDK-8338445 discussion). [4]

Practical guidance: treat a URLClassLoader as an owned resource—use try-with-resources (or ensure close() in finally), and also close every InputStream you obtain from getResourceAsStream(); if you hit jar-locking despite that, check for known JDK issues in your exact Java version and consider workarounds (e.g., disabling JAR URL caching in the specific code path) based on the affected bug report. [1][3]

Sources:
[1] Java SE 21 URLClassLoader Javadoc (close semantics, closes opened jar/file handles, idempotent) (docs.oracle.com)
[2] Oracle “Closing a URLClassLoader” guide (Java 7 close enables replacing updated JARs; loader invalidated) (docs.oracle.com)
[3] OpenJDK bug JDK-7183373 (close not closing JARs in some resource-loading scenarios; related issues) (bugs.openjdk.org)
[4] OpenJDK core-libs-dev thread on JDK-8338445 (URLClassPath potential JarFile leak) (mail.openjdk.org)

Citations:

Close the previous cached class loader when rotating the cache.

Line 43 overwrites CACHE without closing the old URLClassLoader. Since URLClassLoader manages open JAR file handles, this leaks file handles and can lock plugin JARs after repeated changes to the cryptomator.pluginDir system property. On platforms like Windows, this prevents deletion or replacement of updated plugin files.

♻️ Suggested fix 
 public synchronized static URLClassLoader forPluginDirFromCache() {
 	String currentPluginDir = System.getProperty(PLUGIN_DIR_KEY);
 	if (CACHE == null || !Objects.equals(CACHE.pluginDir, currentPluginDir)) {
-		CACHE = new Cache(currentPluginDir, forPluginDirInternal(currentPluginDir));
+		var previous = CACHE == null ? null : CACHE.classLoader;
+		CACHE = new Cache(currentPluginDir, forPluginDirInternal(currentPluginDir));
+		closeQuietly(previous);
 	}
 	return CACHE.classLoader;
 }
+
+private static void closeQuietly(`@Nullable` URLClassLoader classLoader) {
+	if (classLoader == null) {
+		return;
+	}
+	try {
+		classLoader.close();
+	} catch (IOException e) {
+		LOG.warn("Failed to close previous plugin class loader.", e);
+	}
+}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java`
around lines 40 - 45, The cached URLClassLoader held in CACHE is overwritten
without being closed in forPluginDirFromCache; before assigning a new Cache
(created via forPluginDirInternal), detect the existing CACHE and, if its
classLoader is a URLClassLoader, call close() on it (wrap in try/catch to handle
IOException and avoid throwing from the synchronized method), then replace CACHE
with the new Cache. Update forPluginDirFromCache to perform this close-on-rotate
behavior (referencing CACHE, Cache, forPluginDirFromCache, forPluginDirInternal
and the URLClassLoader close() method) so JAR file handles are released when the
pluginDir changes.

Copy link
Member Author

@infeo infeo Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class loader is used in the service loader. Closing it can lead to failures during service discovery.

Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infeo, that's a fair and important point. URLClassLoader.close() invalidates the loader so it can no longer load new classes or resources, and since ServiceLoader lazily discovers and instantiates providers, closing the old loader while it (or any service it loaded) is still in use could cause ClassNotFoundException or other failures at runtime. Accepting the file handle tradeoff to avoid breaking active service discovery makes sense here.

I'll withdraw the suggestion.

✏️ Learnings added 
Learnt from: infeo
Repo: cryptomator/integrations-api PR: 85
File: src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java:40-45
Timestamp: 2026-03-11T14:15:42.977Z
Learning: In `src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java` (cryptomator/integrations-api), the cached URLClassLoader returned by `forPluginDirFromCache()` must NOT be closed when rotating the cache (i.e., when `cryptomator.pluginDir` changes), because the classloader is actively used by `ServiceLoader` in `IntegrationsLoader`. Closing it prematurely can cause failures during service discovery since ServiceLoader lazily loads providers.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: infeo
Repo: cryptomator/integrations-api PR: 85
File: src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java:67-74
Timestamp: 2026-03-11T14:13:16.792Z
Learning: In `src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java` (cryptomator/integrations-api), the removal of `~/` home-directory expansion in `forPluginDirInternal()` is intentional. The downstream project sets the `cryptomator.pluginDir` system property to an explicit absolute path, so tilde expansion is not required.

}

/**
* Attempts to find {@code .jar} files in the path specified in {@value #PLUGIN_DIR_KEY} system property.
* A new class loader instance is returned that loads classes from the given classes.
Expand All @@ -30,21 +53,33 @@ class ClassLoaderFactory {
*/
@Contract(value = "-> new", pure = true)
public static URLClassLoader forPluginDir() {
String val = System.getProperty(PLUGIN_DIR_KEY, "");
final Path p;
if (val.startsWith("~/")) {
p = Path.of(USER_HOME).resolve(val.substring(2));
} else {
p = Path.of(val);
String val = System.getProperty(PLUGIN_DIR_KEY);
return forPluginDirInternal(val);
}

@VisibleForTesting
@Contract(value = "_ -> new", pure = true)
static URLClassLoader forPluginDirInternal(@Nullable String val) {
if (val == null) {
return URLClassLoader.newInstance(new URL[0]);
}

try {
if (val.isBlank()) {
throw new IllegalArgumentException("Plugin dir path is blank");
}
return forPluginDirWithPath(Path.of(val)); //Path.of might throw InvalidPathException
} catch (IllegalArgumentException e) {
LOG.debug("{} contains illegal value. Skipping plugin directory.", PLUGIN_DIR_KEY, e);
return URLClassLoader.newInstance(new URL[0]);
}
return forPluginDirWithPath(p);
}

@VisibleForTesting
@Contract(value = "_ -> new", pure = true)
static URLClassLoader forPluginDirWithPath(Path path) throws UncheckedIOException {
var jars = findJars(path);
if (LOG.isDebugEnabled()) {
if (LOG.isDebugEnabled() && jars.length != 0) {
String jarList = Arrays.stream(jars).map(URL::getPath).collect(Collectors.joining(", "));
LOG.debug("Found jars in cryptomator.pluginDir: {}", jarList);
}
Expand All @@ -56,7 +91,7 @@ static URL[] findJars(Path path) {
try (var stream = Files.walk(path)) {
return stream.filter(ClassLoaderFactory::isJarFile).map(ClassLoaderFactory::toUrl).toArray(URL[]::new);
} catch (IOException | UncheckedIOException e) {
// unable to locate any jars // TODO: log a warning?
LOG.debug("Failed to read plugin dir {}", path, e);
return new URL[0];
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public static <T> Optional<T> load(Class<T> clazz) {
* @param <T> Type of the service
*/
public static <T> Optional<T> loadSpecific(Class<T> clazz, String implementationClassName) {
return ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDir()).stream()
return ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDirFromCache()).stream()
.filter(provider -> provider.type().getName().equals(implementationClassName))
.map(ServiceLoader.Provider::get)
.findAny();
Expand All @@ -61,7 +61,7 @@ public static <T> Optional<T> loadSpecific(Class<T> clazz, String implementation
* @return An ordered stream of all suited service providers
*/
public static <T> Stream<T> loadAll(Class<T> clazz) {
return loadAll(ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDir()), clazz);
return loadAll(ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDirFromCache()), clazz);
}

/**
Expand Down
119 changes: 108 additions & 11 deletions src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
package org.cryptomator.integrations.common;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EmptySource;
import org.junit.jupiter.params.provider.NullSource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.MockedStatic;
import org.mockito.Mockito;

import java.io.ByteArrayInputStream;
Expand All @@ -17,6 +23,9 @@
import java.util.Arrays;
import java.util.Comparator;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.never;

public class ClassLoaderFactoryTest {

@Nested
Expand Down Expand Up @@ -82,6 +91,7 @@ public void testReadPluginDirFromSysProp() {
var absPath = "/there/will/be/plugins";
try (var mockedClass = Mockito.mockStatic(ClassLoaderFactory.class)) {
mockedClass.when(() -> ClassLoaderFactory.forPluginDir()).thenCallRealMethod();
mockedClass.when(() -> ClassLoaderFactory.forPluginDirInternal(any())).thenCallRealMethod();
mockedClass.when(() -> ClassLoaderFactory.forPluginDirWithPath(Path.of(absPath))).thenReturn(ucl);

System.setProperty("cryptomator.pluginDir", absPath);
Expand All @@ -91,21 +101,108 @@ public void testReadPluginDirFromSysProp() {
}
}

@Test
@DisplayName("read path from cryptomator.pluginDir and replace ~/ with user.home")
public void testReadPluginDirFromSysPropAndReplaceHome() {
var ucl = Mockito.mock(URLClassLoader.class, "ucl");
var relPath = "~/there/will/be/plugins";
var absPath = Path.of(System.getProperty("user.home")).resolve("there/will/be/plugins");
try (var mockedClass = Mockito.mockStatic(ClassLoaderFactory.class)) {
mockedClass.when(() -> ClassLoaderFactory.forPluginDir()).thenCallRealMethod();
mockedClass.when(() -> ClassLoaderFactory.forPluginDirWithPath(absPath)).thenReturn(ucl);
@Nested
@DisplayName("Method pluginDirInternal")
public class PluginDirInternal {
MockedStatic<ClassLoaderFactory> mockedClass;

System.setProperty("cryptomator.pluginDir", relPath);
var result = ClassLoaderFactory.forPluginDir();
@BeforeEach
public void beforeEach() {
mockedClass = Mockito.mockStatic(ClassLoaderFactory.class);
mockedClass.when(() -> ClassLoaderFactory.forPluginDirInternal(any())).thenCallRealMethod();
}

@AfterEach
public void afterEach() {
mockedClass.close();
}

@Test
@DisplayName("Valid path string calls forPluginDirWithPath")
public void testValidPathString() {
var ucl = Mockito.mock(URLClassLoader.class, "ucl");
mockedClass.when(() -> ClassLoaderFactory.forPluginDirWithPath(any())).thenReturn(ucl);

var result = ClassLoaderFactory.forPluginDirInternal("some/string");
mockedClass.verify(() -> ClassLoaderFactory.forPluginDirWithPath(any()));
Assertions.assertSame(ucl, result);
}

@ParameterizedTest
@DisplayName("Invalid or null path strings do not call forPluginDirWithPath")
@EmptySource
@ValueSource(strings = {"\t\t", " "})
@NullSource
public void testInvalidPathString(String propValue) {
var result = ClassLoaderFactory.forPluginDirInternal(propValue);

mockedClass.verify(() -> ClassLoaderFactory.forPluginDirWithPath(any()), never());
Assertions.assertNotNull(result);
Assertions.assertEquals(0, result.getURLs().length);
}

}

@Nested
@DisplayName("Method pluginDirFromCache")
public class PluginDirFromCache {

MockedStatic<ClassLoaderFactory> mockedClass;

@BeforeEach
public void beforeEach() {
ClassLoaderFactory.CACHE = null;
System.clearProperty("cryptomator.pluginDir");
mockedClass = Mockito.mockStatic(ClassLoaderFactory.class);
mockedClass.when(() -> ClassLoaderFactory.forPluginDirFromCache()).thenCallRealMethod();
}

@AfterEach
public void afterEach() {
mockedClass.close();
ClassLoaderFactory.CACHE = null;
System.clearProperty("cryptomator.pluginDir");
}

@Test
@DisplayName("returns cached classloader on subsequent calls with same property")
public void testReturnsCachedInstance() {
var ucl = Mockito.mock(URLClassLoader.class, "ucl");
mockedClass.when(() -> ClassLoaderFactory.forPluginDirInternal(any())).thenReturn(ucl);

System.setProperty("cryptomator.pluginDir", "/some/path");
var first = ClassLoaderFactory.forPluginDirFromCache();
var second = ClassLoaderFactory.forPluginDirFromCache();

Assertions.assertSame(first, second);
mockedClass.verify(() -> ClassLoaderFactory.forPluginDirInternal("/some/path"), Mockito.times(1));
}

@Test
@DisplayName("creates new classloader when property changes")
public void testInvalidatesCacheOnPropertyChange() {
var ucl1 = Mockito.mock(URLClassLoader.class, "ucl1");
var ucl2 = Mockito.mock(URLClassLoader.class, "ucl2");
var ucl3 = Mockito.mock(URLClassLoader.class, "ucl3");
mockedClass.when(() -> ClassLoaderFactory.forPluginDirInternal(any())).thenReturn(ucl1, ucl2, ucl3);

System.setProperty("cryptomator.pluginDir", "/path/one");
var first = ClassLoaderFactory.forPluginDirFromCache();

System.setProperty("cryptomator.pluginDir", "/path/two");
var second = ClassLoaderFactory.forPluginDirFromCache();

System.clearProperty("cryptomator.pluginDir");
var third = ClassLoaderFactory.forPluginDirFromCache();

Assertions.assertSame(ucl1, first);
Assertions.assertSame(ucl2, second);
Assertions.assertSame(ucl3, third);
Assertions.assertNotSame(first, second);
Assertions.assertNotSame(second, third);
Assertions.assertNotSame(first, third);
mockedClass.verify(() -> ClassLoaderFactory.forPluginDirInternal(any()), Mockito.times(3));
}
}

@Test
Expand Down
Loading