Skip to content
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.

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

Do not search highest classloader #11041

Conversation

jerrinot
Copy link
Contributor

@jerrinot jerrinot commented Aug 4, 2017

--Problem--
Short Version:
Our classloading assumes a hiearchical parent-first delegation model.
This assumption is not always right.

Long Version:
Imagine this classloader hiearchy: childCL -> parentCL
Both childCl and parentCL can access the same JAR. The childCL does
not simply delegate to its parent, it loads (&define) classes on its own.

Now Hazelcast is loaded by the childCL. It's loading hooks with service
definitions when it's starting. It finds META-INF/service/c.h.hook resource
can be loaded by the childCl. However when the findHighestReachableClassLoader()
is used then it will also find the parentCL can load the resource as well (because both classloaders point to the same physical JAR thus they both have a resource with the same URL available) ->
Hazelcast will use the parentCl to load the hook. However the classes loaded
by parentCL are not the same as classes loaded by childCL -> ClassCastException.

Solution:
Do not search for the highest available classloader.

Problem with the solution:
We now might have multiple classloaders loading the same resource
-> Duplicate registration

Solution of the problem:
De-duplication in the ClassIterator.

@jerrinot jerrinot added this to the 3.9 milestone Aug 4, 2017
--Problem--
Short Version:
Our classloading assumes a hiearchical parent-first delegation model.
This assumption is not always right.

Long Version:
Imagine this classloader hiearchy: childCL -> parentCL
Both childCl and parentCL loads from the same JAR. The childCL does
not simply delegate to its parent, it loads (&define) classes on its own.

Now Hazelcast is loaded by the childCL. It's loading hooks with service
definitions when it's starting. It finds META-INF/service/c.h.hook resource
can be loaded by the childCl. However when the findHighestReachableClassLoader()
is used then it will also find the parentCL can load the resource as well ->
Hazelcast will use the parentCl to load hook. However the classes loaded
by parentCL are not the same as classes loaded by childCL.

Solution:
Do not search for the highest available classloader.

Problem with the solution:
We now might have multiple classloaders loading the same resource
->
Duplicate registration

Solution of the problem:
De-duplication in the ClassIterator.

TODO:
Tests
@jerrinot jerrinot force-pushed the experiments/not-not-search-higher-classloader/master branch from e0116b1 to 2f5eec6 Compare August 4, 2017 10:13
@jerrinot jerrinot changed the title [DO NOT MERGE] Do not search highest classloader Do not search highest classloader Aug 4, 2017
Copy link
Contributor

@mmedenjak mmedenjak left a comment

Choose a reason for hiding this comment

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

Looks fine, except for one detail

ClassLoader highestClassLoader = findHighestReachableClassLoader(url, classLoader, resourceName);
if (!highestClassLoader.getClass().getName().equals(IGNORED_GLASSFISH_MAGIC_CLASSLOADER)) {
urlDefinitions.add(new URLDefinition(uri, highestClassLoader));
if (!classLoader.getClass().getName().equals(IGNORED_GLASSFISH_MAGIC_CLASSLOADER)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IntelliJ is warning me this line can cause NPE. I believe it can, since the ArrayList can contain null and it could be added to the list as a result of this:

        ClassLoader tccl = Thread.currentThread().getContextClassLoader();
        if (tccl != classLoader) {
            classLoaders.add(tccl);
        }

Where getContextClassLoader can return null:

     * @return  the context ClassLoader for this Thread, or {@code null}
     *          indicating the system class loader (or, failing that, the
     *          bootstrap class loader)

@@ -400,6 +360,10 @@ private boolean advance() {
return false;
}

private boolean isDuplicate(Class<?> candidate) {
return !alreadyProvidedClasses.add(candidate);
Copy link
Contributor

@mmedenjak mmedenjak Aug 9, 2017

Choose a reason for hiding this comment

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

I was just thinking about this - this would eliminate classes that are equal. That includes the check if the class was loaded by the same classloader. So this check only eliminates same classes loaded by different classloaders? So for example, we had one classloader A loading the service definitions from one JAR and a different classloader B loading the service defintions from a different JAR. If both of them have the same parent and delegate to this parent, this check would eliminate duplicate service definitions from different JARs?

@jerrinot jerrinot merged commit 261c95f into hazelcast:master Aug 10, 2017
@jerrinot jerrinot deleted the experiments/not-not-search-higher-classloader/master branch August 10, 2017 10:05
@mmedenjak mmedenjak added the Source: Internal PR or issue was opened by an employee label Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Source: Internal PR or issue was opened by an employee Team: Core Type: Defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants