Conversation
WalkthroughAdds a caching mechanism to ClassLoaderFactory via a static CACHE field and a new record Cache(pluginDir, classLoader), and exposes a synchronized public method forPluginDirFromCache() that returns a cached URLClassLoader and reloads when the pluginDir system property changes. forPluginDir() now delegates to forPluginDirInternal(String), which validates null/blank/path syntax and returns an empty URLClassLoader for invalid input; valid values are handled by forPluginDirWithPath(Path). Jar discovery and error logging are adjusted to debug level and only logged when jars exist. IntegrationsLoader now uses forPluginDirFromCache(). Tests add nested PluginDirInternal and PluginDirFromCache suites to verify validation and cache behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java (1)
121-143: Please exercise the cached path in these tests.These assertions stop at
forPluginDir(), but the production callers now useforPluginDirCached(). A regression test that calls the cached method before and after settingcryptomator.pluginDirwould cover the behavior that actually changed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java` around lines 121 - 143, Update the tests to exercise the cached path by invoking ClassLoaderFactory.forPluginDirCached() in addition to forPluginDir(): call forPluginDirCached() once before modifying the system property and once after setting/clearing cryptomator.pluginDir to assert that the cache behavior is correct; keep existing verifications that ClassLoaderFactory.forPluginDirWithPath(...) is never called and that the returned URLClassLoader is non-null with zero URLs where appropriate, and use the same test methods (testUndefinedSysProp and testBlankSysProp) to perform the cached-call assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java`:
- Around line 24-37: forPluginDirCached() currently memoizes a single
CACHED_CLASSLOADER forever which breaks plugin discovery when the system
property PLUGIN_DIR_KEY changes; update the logic in forPluginDirCached() to
compute the current pluginDir property value (System.getProperty for
PLUGIN_DIR_KEY), and only return a cached URLClassLoader if the cached instance
was created for that same property value (e.g., store the property string
alongside CACHED_CLASSLOADER), otherwise call forPluginDir() to create a new
loader and update the cache; alternatively, if forPluginDir() returns a
fallback/empty loader when the property is blank/invalid, do not cache that
fallback instance so later valid property values will still trigger reloading
(ensure changes apply to CACHED_CLASSLOADER, forPluginDirCached(),
forPluginDir() and interaction points like
IntegrationsLoader.loadSpecific()/loadAll()).
---
Nitpick comments:
In
`@src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java`:
- Around line 121-143: Update the tests to exercise the cached path by invoking
ClassLoaderFactory.forPluginDirCached() in addition to forPluginDir(): call
forPluginDirCached() once before modifying the system property and once after
setting/clearing cryptomator.pluginDir to assert that the cache behavior is
correct; keep existing verifications that
ClassLoaderFactory.forPluginDirWithPath(...) is never called and that the
returned URLClassLoader is non-null with zero URLs where appropriate, and use
the same test methods (testUndefinedSysProp and testBlankSysProp) to perform the
cached-call assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 071f69d4-99aa-435a-ae21-b629b49cba91
📒 Files selected for processing (3)
src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.javasrc/main/java/org/cryptomator/integrations/common/IntegrationsLoader.javasrc/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java
src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java
Outdated
Show resolved
Hide resolved
* rename method to fit better the naming scheme * use cache record to model the connection propertyValue, classloader * adjust unit tests
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java`:
- Around line 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.
- Around line 67-74: Restore tilde expansion for home-relative plugin paths or
explicitly fail: in ClassLoaderFactory, before calling
forPluginDirWithPath(Path.of(val)) check if val startsWith("~/") (or equals "~")
and replace the leading "~" with System.getProperty("user.home") so Path is
built from the expanded path; if expansion/Path.of fails, throw or log an
explicit error instead of silently returning an empty URLClassLoader (update the
catch handling around forPluginDirWithPath to include PLUGIN_DIR_KEY and the
actual path/error in LOG.error), ensuring callers get a clear failure or the
expanded path is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cebc98f7-5759-413d-b411-4803b87d90ac
📒 Files selected for processing (3)
src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.javasrc/main/java/org/cryptomator/integrations/common/IntegrationsLoader.javasrc/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java
| 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; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java | head -100Repository: cryptomator/integrations-api
Length of output: 4342
🏁 Script executed:
grep -n "CACHE\|close\|URLClassLoader" src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.javaRepository: 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 forjar:/file:URLs it closes any files it opened (e.g.,JarFilehandles). Callingclose()more than once is a no-op. [1] - Why this matters (typical “leak” symptom): On platforms like Windows, an unclosed
JarFilehandle commonly shows up as an inability to delete/replace the JAR until the loader (and any streams) are closed. Oracle explicitly documents Java 7’sclose()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 involvinggetResource*/getResourceAsStream), leaving JARs locked even afterclose()—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.URLClassPathleakingJarFileinstances in certain manifestClass-Pathscenarios—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:
- 1: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/net/URLClassLoader.html?utm_source=openai
- 2: https://docs.oracle.com/javase/8/docs/technotes/guides/net/ClassLoader.html?utm_source=openai
- 3: https://bugs.openjdk.org/browse/JDK-7183373?utm_source=openai
- 4: https://mail.openjdk.org/pipermail/core-libs-dev/2024-August/128134.html?utm_source=openai
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.
There was a problem hiding this comment.
The class loader is used in the service loader. Closing it can lead to failures during service discovery.
There was a problem hiding this comment.
@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.
src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java
Show resolved
Hide resolved
1 participant
This PR applies a small refactoring in the plugin loading mechanism.
Noteworthy changes:
cryptomator.PluginDiris not set, no directory is searched for plugins