snehashisp commented on code in PR #18403:
URL: https://github.com/apache/kafka/pull/18403#discussion_r1906560819
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/ReflectionScanner.java:
##########
@@ -76,8 +78,21 @@ private static <T> String versionFor(Class<? extends T>
pluginKlass) throws Refl
@Override
protected PluginScanResult scanPlugins(PluginSource source) {
+ // By default, java and classgraph uses parent first classloading,
hence if a plugin is loaded by the classpath
+ // loader, and then by an isolated plugin loader, the default
precedence will always load the classpath version.
+ // This breaks isolation and hence connect uses isolated plugin
loaders, which are child first classloaders.
+ // Therefore, we override the classloader order to be child first, so
that the isolated plugin loader is used first.
+ // In addition, we need to explicitly specify the full classloader
order, as classgraph only scans the classes available
+ // in the classloaders and not the entire parent chain. Due to this
reason if a plugin is extending a class present
+ // in classpath/application it will not be able to find the parent
class unless we explicitly specify the classloader order.
+ List<ClassLoader> classLoaderOrder = new ArrayList<>();
+ ClassLoader cl = source.loader();
+ while (cl != null) {
+ classLoaderOrder.add(cl);
+ cl = cl.getParent();
+ }
Review Comment:
I did some more analysis on this. My statement is somewhat incorrect.
Classgraph does try to find and scan the classes from parent but it fails to do
so. Elaborating more.
Classgraph scanning works by computing a list of class path URLs from the
provided class loaders and then manually scans the `.class` files under the
URLs to retrieve information about those classes. Unless we instruct
classloading explicitly (which we do
[here](https://github.com/apache/kafka/blob/2d41315aeaa1570af6840e0501878cbc02463c96/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/ReflectionScanner.java#L144))
the classes are not loaded using Java's classloading. If the desired class is
not found in the scanning our code never tries to load the class. The list of
class path URLs are ordered based on the ordering of the provided classloaders.
All the logic is in the constructor of
[ClasspathFinder](https://github.com/classgraph/classgraph/blob/latest/src/main/java/nonapi/io/github/classgraph/classpath/ClasspathFinder.java).
ClasspathFinder uses various
[ClassLoaderHandlers](https://github.com/classgraph/classgraph/tree/latest/src/main/java/nonapi/io/github/classgraph/classloaderhandler)
to obtain the set of URLs for a classloader. Connects PluginClassLoader uses
[URLClassLoaderHandler](https://github.com/classgraph/classgraph/blob/6f9012f2a193ebfefe4a4384e7642820e7aab0f5/src/main/java/nonapi/io/github/classgraph/classloaderhandler/URLClassLoaderHandler.java#L86)
which works fine and gets the list of jars in a plugin path. But when it comes
to the application classloader and platform classloader which has the URLs for
classpath it uses
[JPMSClassLoaderHelper](https://github.com/classgraph/classgraph/blob/6f9012f2a193ebfefe4a4384e7642820e7aab0f5/src/main/java/nonapi/io/github/classgraph/classloaderhandler/JPMSClassLoaderHandler.java#L89)
and tries to get the URLs through a illegal reflections access on a private
field, which will fail on modern Java (throws an IllegalAccessException, can be
mitigated u
sing `--illegal-access=permit` but this is not present since Java 17 and not
really recommended). Even though the classloader chain is computed the, URLs in
classpath are not obtained because of this. This is why some of the tests were
failing where `SubclassOfClasspathConverter` which extens `ByteArrayConverter`
was not computed to be an implementation of a converter since
`ByteArrayConverter` is in classpath.
To force classpath URL scanning explicitly we need ClasspathFinder to
execute
[this](https://github.com/classgraph/classgraph/blob/6f9012f2a193ebfefe4a4384e7642820e7aab0f5/src/main/java/nonapi/io/github/classgraph/classpath/ClasspathFinder.java#L299-L314)
part of code, which is only possible with classloader overrides if one of the
provided classloader is application/platform classloader. Passing the
classloader chain for the PluginClassLoader achieves this.
`ignoreParentClassLoader` is tied to `overrideClassloader == null` check, hence
is of no use with classLoader overrides.
--
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]
