Copilot commented on code in PR #39:
URL:
https://github.com/apache/accumulo-classloaders/pull/39#discussion_r2688547098
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
+
+ // a pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor for this proxy
+ // object's reachability, but then the clean action can clean up the real
URLClassLoader when
+ // the proxy object becomes phantom-reachable (this won't work)
+ var proxiedCl = (URLClassLoader) Proxy.newProxyInstance(parent,
+ getInterfaces(URLClassLoader.class).toArray(new Class<?>[0]), (obj,
method, args) -> {
+ try {
+ return method.invoke(cl, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
+ });
+
+ CLEANER.register(proxiedCl, () -> {
+ try {
+ cl.close();
+ } catch (IOException e) {
+ LOG.debug("Problem closing phantom-reachable classloader instance {}",
name);
+ }
+ });
+
if (LOG.isTraceEnabled()) {
LOG.trace("New classloader created for {} from URLs: {}", name,
Arrays.asList(urls));
}
- return cl;
+ return proxiedCl;
}
+ private static Set<Class<?>> getInterfaces(Class<?> clazz) {
+ var set = new HashSet<Class<?>>();
+ if (clazz != null) {
+ set.addAll(getInterfaces(clazz.getSuperclass()));
+ for (Class<?> interfaze : clazz.getInterfaces()) {
+ set.add(interfaze);
Review Comment:
The variable name 'interfaze' is a misspelling workaround to avoid the
'interface' keyword. Consider using a more conventional name like 'iface',
'intf', or 'interfaceClass' which are more commonly used in Java code.
```suggestion
for (Class<?> iface : clazz.getInterfaces()) {
set.add(iface);
```
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
+
+ // a pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor for this proxy
+ // object's reachability, but then the clean action can clean up the real
URLClassLoader when
+ // the proxy object becomes phantom-reachable (this won't work)
+ var proxiedCl = (URLClassLoader) Proxy.newProxyInstance(parent,
+ getInterfaces(URLClassLoader.class).toArray(new Class<?>[0]), (obj,
method, args) -> {
+ try {
+ return method.invoke(cl, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
+ });
+
+ CLEANER.register(proxiedCl, () -> {
+ try {
+ cl.close();
+ } catch (IOException e) {
+ LOG.debug("Problem closing phantom-reachable classloader instance {}",
name);
+ }
+ });
Review Comment:
The cleaner action captures a strong reference to the URLClassLoader
instance 'cl' in the lambda, which prevents the cleaner from ever triggering.
According to the Cleaner API documentation, the cleaning action must not hold
strong references to the object being monitored (proxiedCl), but it also must
not hold references that would prevent the monitored object from becoming
phantom-reachable. Since loaded classes hold references to the actual
classloader 'cl', and 'cl' is referenced in the cleaning action, this creates a
reference chain that prevents cleanup.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
Review Comment:
The parent classloader reference in LccUtils.class.getClassLoader() has
changed from the original
LocalCachingContextClassLoaderFactory.class.getClassLoader(). This change in
parent classloader could affect class resolution behavior and which classes are
visible to the created URLClassLoader, potentially breaking existing
functionality.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
+
+ // a pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor for this proxy
+ // object's reachability, but then the clean action can clean up the real
URLClassLoader when
+ // the proxy object becomes phantom-reachable (this won't work)
+ var proxiedCl = (URLClassLoader) Proxy.newProxyInstance(parent,
+ getInterfaces(URLClassLoader.class).toArray(new Class<?>[0]), (obj,
method, args) -> {
+ try {
+ return method.invoke(cl, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
+ });
+
+ CLEANER.register(proxiedCl, () -> {
+ try {
+ cl.close();
+ } catch (IOException e) {
+ LOG.debug("Problem closing phantom-reachable classloader instance {}",
name);
+ }
+ });
+
if (LOG.isTraceEnabled()) {
LOG.trace("New classloader created for {} from URLs: {}", name,
Arrays.asList(urls));
}
- return cl;
+ return proxiedCl;
}
+ private static Set<Class<?>> getInterfaces(Class<?> clazz) {
Review Comment:
The method name getInterfaces is ambiguous because it returns all interfaces
from the entire class hierarchy, not just the immediate interfaces of the
provided class. Consider renaming to getAllInterfacesRecursive or
collectAllInterfaces to better reflect what the method does.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
+
+ // a pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor for this proxy
+ // object's reachability, but then the clean action can clean up the real
URLClassLoader when
+ // the proxy object becomes phantom-reachable (this won't work)
Review Comment:
The comment explicitly acknowledges that this approach won't work, but the
code is being implemented anyway. This creates confusion and technical debt. If
this is experimental code, it should be clearly marked as such or moved to a
feature branch. Alternatively, if this is known to be non-functional, consider
documenting why it's being committed and what the next steps are.
```suggestion
// A pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor this proxy
// object's reachability and, when it becomes phantom-reachable, attempt
to clean up the real
// URLClassLoader. This relies on GC and Cleaner semantics and therefore
provides best-effort,
// not guaranteed, timely cleanup.
```
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
+
+ // a pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor for this proxy
+ // object's reachability, but then the clean action can clean up the real
URLClassLoader when
+ // the proxy object becomes phantom-reachable (this won't work)
+ var proxiedCl = (URLClassLoader) Proxy.newProxyInstance(parent,
+ getInterfaces(URLClassLoader.class).toArray(new Class<?>[0]), (obj,
method, args) -> {
+ try {
+ return method.invoke(cl, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
+ });
Review Comment:
The proxy creation may fail at runtime because URLClassLoader is a concrete
class, not an interface. The Proxy.newProxyInstance method only works with
interfaces. Since URLClassLoader extends ClassLoader (a class), attempting to
cast the proxy result to URLClassLoader will throw a ClassCastException. The
proxy can only implement interfaces, not extend classes.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
+
+ // a pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor for this proxy
+ // object's reachability, but then the clean action can clean up the real
URLClassLoader when
+ // the proxy object becomes phantom-reachable (this won't work)
+ var proxiedCl = (URLClassLoader) Proxy.newProxyInstance(parent,
+ getInterfaces(URLClassLoader.class).toArray(new Class<?>[0]), (obj,
method, args) -> {
+ try {
+ return method.invoke(cl, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
Review Comment:
The InvocationTargetException unwrapping may not preserve the original
exception type properly. When throwing e.getCause(), if the cause is a checked
exception, it may violate the method signature's declared exceptions. Consider
checking if the cause is an Error or RuntimeException before unwrapping, or
handling checked exceptions appropriately.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
+
+ // a pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor for this proxy
+ // object's reachability, but then the clean action can clean up the real
URLClassLoader when
+ // the proxy object becomes phantom-reachable (this won't work)
+ var proxiedCl = (URLClassLoader) Proxy.newProxyInstance(parent,
+ getInterfaces(URLClassLoader.class).toArray(new Class<?>[0]), (obj,
method, args) -> {
+ try {
+ return method.invoke(cl, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
+ });
+
+ CLEANER.register(proxiedCl, () -> {
+ try {
+ cl.close();
+ } catch (IOException e) {
+ LOG.debug("Problem closing phantom-reachable classloader instance {}",
name);
+ }
+ });
+
if (LOG.isTraceEnabled()) {
LOG.trace("New classloader created for {} from URLs: {}", name,
Arrays.asList(urls));
}
- return cl;
+ return proxiedCl;
}
+ private static Set<Class<?>> getInterfaces(Class<?> clazz) {
+ var set = new HashSet<Class<?>>();
+ if (clazz != null) {
+ set.addAll(getInterfaces(clazz.getSuperclass()));
+ for (Class<?> interfaze : clazz.getInterfaces()) {
+ set.add(interfaze);
+ }
Review Comment:
The getInterfaces method has inefficient recursion that processes the same
interfaces multiple times. When collecting interfaces from a class hierarchy,
the same interface can be encountered multiple times through different paths,
leading to redundant recursive calls. Consider using iterative traversal or
memoization to avoid processing the same class multiple times.
```suggestion
Class<?> current = clazz;
while (current != null) {
for (Class<?> interfaze : current.getInterfaces()) {
set.add(interfaze);
}
current = current.getSuperclass();
```
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/LccUtils.java:
##########
@@ -34,15 +39,48 @@ public class LccUtils {
public static final DigestUtils DIGESTER = new
DigestUtils(DigestUtils.getSha256Digest());
+ private static final Cleaner CLEANER = Cleaner.create();
+
@SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
public static URLClassLoader createClassLoader(String name, URL[] urls) {
- final var cl = new URLClassLoader(name, urls,
- LocalCachingContextClassLoaderFactory.class.getClassLoader());
+ final var parent = LccUtils.class.getClassLoader();
+ final var cl = new URLClassLoader(name, urls, parent);
+
+ // a pass-through wrapper for the URLClassLoader, so the Cleaner can
monitor for this proxy
+ // object's reachability, but then the clean action can clean up the real
URLClassLoader when
+ // the proxy object becomes phantom-reachable (this won't work)
+ var proxiedCl = (URLClassLoader) Proxy.newProxyInstance(parent,
+ getInterfaces(URLClassLoader.class).toArray(new Class<?>[0]), (obj,
method, args) -> {
+ try {
+ return method.invoke(cl, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
+ });
+
+ CLEANER.register(proxiedCl, () -> {
+ try {
+ cl.close();
+ } catch (IOException e) {
+ LOG.debug("Problem closing phantom-reachable classloader instance {}",
name);
Review Comment:
The error message does not include the exception details, making it
difficult to diagnose issues when the close operation fails. Consider logging
the exception or including it in the log message to aid in troubleshooting.
```suggestion
LOG.debug("Problem closing phantom-reachable classloader instance
{}", name, e);
```
--
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]
