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

LOG4J2-2754: LoaderUtil.getClassLoaders may discover additional loaders #328

Conversation

carterkozak
Copy link
Contributor

The utility no longer erroneously returns a result with a null element in some
environments.
Also updated loadClass to avoid internally throwing and catching a null
pointer exception when getThreadContextClassLoader returns null.

The utility no longer erroneously returns a result with a null element in some
environments.
Also updated loadClass to avoid internally throwing and catching a null
pointer exception when getThreadContextClassLoader returns null.
classLoaders.add(current);
final ClassLoader parent = current.getParent();
while (parent != null && !classLoaders.contains(parent)) {
classLoaders.add(parent);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This loop failed to modify the condition

@@ -106,30 +104,31 @@ public ClassLoader run() {
}

public static ClassLoader[] getClassLoaders() {
final List<ClassLoader> classLoaders = new ArrayList<>();
final Collection<ClassLoader> classLoaders = new LinkedHashSet<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

LinkedHashSet is more expensive in environments with few values, but performs better in the worst case without sacrificing order.

@carterkozak
Copy link
Contributor Author

@rgoers I'm not as familiar with the consumers of LoaderUtil.getClassLoaders() as you are, I'd appreciate your feedback before this merges, at your convenience -- I'm not in a rush to merge this, I happened to notice the loop while looking at something else.

Copy link
Member

@rgoers rgoers left a comment

Choose a reason for hiding this comment

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

I have reviewed this and it looks OK to me. Assuming you ran all the unit tests you should be fine to merge this.

@carterkozak
Copy link
Contributor Author

Thanks Ralph, tests all pass, I've merged this commit. It's not a clean cherry-pick to master because fbde9cd was not applied to release-2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants