[core] Isolate classloaders for runtime and auxclasspath #680

jsotuyod · 2017-10-23T17:32:00Z

We will now always load classes from auxclasspath during
typeresolution, even if we have a class with the same name in PMD's
classpath
This prevents conflicts when using different versions of the same
dependencies

- We will now always load classes from auxclasspath during typeresolution, even if we have a class with the same name in PMD's classpath - This prevents conflicts when using different versions of the same dependencies

jsotuyod · 2017-10-23T17:33:23Z

pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java

@@ -543,7 +543,7 @@ private void parseSingleRuleNode(RuleSetReferenceId ruleSetReferenceId, RuleSetB
        if (attribute == null || "".equals(attribute)) {
            throw new IllegalArgumentException("The 'class' field of rule can't be null, nor empty.");
        }
-        Rule rule = (Rule) classLoader.loadClass(attribute).newInstance();
+        Rule rule = (Rule) RuleSetFactory.class.getClassLoader().loadClass(attribute).newInstance();


This change in particular means users can no longer write custom Java rules within their project's codebase / auxclasspath and apply them. This feature was undocumented and unsupported.

Custom rules can be written, but must be part of PMD's execution classpath.

adangel · 2017-10-23T19:33:07Z

@jsotuyod Is this a fix for the build problem described in #679?

Custom rules can be written, but must be part of PMD's execution classpath.

This sounds totally correct - I wouldn't have expected to be implemented otherwise. But it was... With this PR, the classes need to be on the execution classpath, but the rulesets still can be on the auxclasspath, which is wrong, I think.

I don't know, why we use the auxclasspath in the RuleSetFactory at all. It looks like, it was a wrong fix some time ago, for the problem, that PMD didn't find a rule/ruleset (because it's easier to change the auxclasspath -> command line option, than to change the execution class path -> shell script).

One point to the classloading: The maven plugin configures the Thread's context classloader - which might make a difference - not sure.

I'm not sure, whether we really need the change for the parent-last strategy. It would only make a difference, if the same class is multiple times on the auxclasspath...

jsotuyod · 2017-10-23T20:03:56Z

@adangel yes, this addresses the issue on #679, I was finally able to reproduce it even from outside Maven.

You are right, the ruleset can be in the auxclasspath. I believe the reason why we give a classloader to the rule set factory is here:

https://github.com/pmd/pmd/blob/master/pmd-core/src/main/java/net/sourceforge/pmd/ant/internal/PMDTaskImpl.java#L240-L253

It can probably be worked out more elegantly.

I'm not sure, whether we really need the change for the parent-last strategy. It would only make a difference, if the same class is multiple times on the auxclasspath...

If a project uses any dependency we use, but a different version, they will have a conflict. Suppose someone is using commons-io 2.6. Since we use version 2.4, and the default is parent-first, we will always try to load classes from the execution claspath before checking the auxclasspath. Therefore, all classes will be loaded from 2.4, meaning typeresolution can fail (incompatible types, missing methods, etc.). We want to use 2.4 for running PMD, but 2.6 for typeresolution.

adangel · 2017-10-23T20:13:19Z

@jsotuyod Ok, understood the issue why parent-last.

But: Should the execution classpath ever end up in the auxclasspath, like it currently always does? https://github.com/pmd/pmd/blob/master/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java#L191-L196 (either classpath is null or set via the method you mentioned for ant)

We probably should only have on the auxclasspath, whatever is given as the auxclasspath configuration option + the standard java base classpath (java.lang.*, etc.) / bootclasspath.

jsotuyod · 2017-10-23T20:20:36Z

We probably should only have on the auxclasspath, whatever is given as the auxclasspath configuration option + the standard java base classpath (java.lang.*, etc.) / bootclasspath.

@adangel I thought about it, but I don't know if we can get a hold of the same "boot classloader" as the one being used to run PMD. Otherwise I see 2 issues:

we would end up loading twice all JRE classes, meaning more memory / CPU usage
rules would be harder to write. We can't check against stuff such as instanceof String anymore, as that String would be loaded from a different classloader than the one stored in the node's type, and therefore not match.

Code such as this will fail the exact same way:

https://github.com/pmd/pmd/blob/master/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symboltable/TypeSet.java#L402-L409

I'm uncertain if setting the parent classloader to getSystemClassLoader() will work for this. Wouldn't this produce the same behavior we currently have when running from CLI (java -cp allpmdclassesanddeps)?

gibarsin · 2017-10-23T20:22:27Z

pmd-core/src/main/java/net/sourceforge/pmd/util/ClasspathClassLoader.java

+        // First, check if the class has already been loaded
+        Class<?> c = findLoadedClass(name);
+        if (c == null) {
+            if (c == null) {


Nothing harmful, but the null check is done twice.

Thanks! Bad copy-paste on my part

jsotuyod · 2017-10-24T01:20:19Z

@adangel I confirm, getSystemClassLoader() returns the same classloader used to start the app, and contains the whole classpath.

We could, request that classloader's parent, and that one DOES only resolve JRE classes, but I am not sure how reliable that is... I can't find documentation saying this behavior is standard.

That parent classloader is a ExtClassLoader (at least under Oracle JVMs), which loads anything from jars within $JAVA_HOME/jre/lib/ext/ or anything the property java.ext.dirs points at which could be tampered to include other things, but that's more unlikely. I don't personally fell comfortable using this unless we can find proper documentation on how cross-JVMs this behavior actually is.

- ResourceLoader is now instantiable, and we can tell which classloader to use to get resources - We will always use the execution classloader, or just add the paths added by ant, but *never* the auxclasspath - The classpath added by Ant won't get into the auxclasspath either

jsotuyod · 2017-10-24T21:13:24Z

@adangel I reworked how we load rulesets. In doing so we now enforce both rules and rulesets are only in the execution classpath.

As for typeresolution, we keep the newly introduced parent-last approach to avoid issues.

I'm no longer adding classpath entries defined in Ant to the auxclasspath (although they can still be used to locate rulesets, not rules!). Users should add those entries to the auxclasspath explicitly if they want them.

Please review it, and provide feedback on the open issue on which parent classloader to use for typeresolution.

adangel · 2017-10-28T17:26:57Z

@jsotuyod

... We can't check against stuff such as instanceof String anymore

Completely agree - this would be a no-go.

We could, request that classloader's parent, and that one DOES only resolve JRE classes, but I am not sure how reliable that is... I can't find documentation saying this behavior is standard.

Probably not reliable :) At least, the documentation about the getSystemClassloader talks about implementation-dependency...

So, let's stay with the parent-last approach for resolving classing during typeresolution.

I like, how you reworked ruleset loading with using the resource loader! Nice!

[core] Support parent-last classloading

96bb1f4

- We will now always load classes from auxclasspath during typeresolution, even if we have a class with the same name in PMD's classpath - This prevents conflicts when using different versions of the same dependencies

jsotuyod added a:bug PMD crashes or fails to analyse a file. in:pmd-internals Affects PMD's internals labels Oct 23, 2017

jsotuyod added this to the 6.0.0 milestone Oct 23, 2017

jsotuyod assigned adangel Oct 23, 2017

jsotuyod commented Oct 23, 2017

View reviewed changes

gibarsin reviewed Oct 23, 2017

View reviewed changes

Avoid duplicate code

42d9658

jsotuyod changed the title ~~[core] Support parent-last classloading~~ [core] Isolate classloaders for runtime and auxclasspath Oct 24, 2017

jsotuyod mentioned this pull request Oct 25, 2017

[core] Token scheme generalization #679

Merged

adangel added a commit to adangel/pmd that referenced this pull request Oct 28, 2017

Update release notes, refs pmd#680

20f1676

adangel merged commit 20093c8 into pmd:master Oct 28, 2017

jsotuyod deleted the parent-last-classloading branch October 28, 2017 19:06

adangel mentioned this pull request Nov 11, 2017

[java] LinkageError with aux classpath #732

Closed

jsotuyod mentioned this pull request Jan 12, 2018

[core] build problem: org.apache.maven.plugins:maven-pmd-plugin:3.8:pmd: java.lang.IncompatibleClassChangeError #843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Isolate classloaders for runtime and auxclasspath #680

[core] Isolate classloaders for runtime and auxclasspath #680

jsotuyod commented Oct 23, 2017

jsotuyod Oct 23, 2017

adangel commented Oct 23, 2017

jsotuyod commented Oct 23, 2017

adangel commented Oct 23, 2017

jsotuyod commented Oct 23, 2017 •

edited

Loading

gibarsin Oct 23, 2017

jsotuyod Oct 23, 2017

jsotuyod commented Oct 24, 2017 •

edited

Loading

jsotuyod commented Oct 24, 2017 •

edited

Loading

adangel commented Oct 28, 2017 •

edited

Loading

[core] Isolate classloaders for runtime and auxclasspath #680

[core] Isolate classloaders for runtime and auxclasspath #680

Conversation

jsotuyod commented Oct 23, 2017

jsotuyod Oct 23, 2017

Choose a reason for hiding this comment

adangel commented Oct 23, 2017

jsotuyod commented Oct 23, 2017

adangel commented Oct 23, 2017

jsotuyod commented Oct 23, 2017 • edited Loading

gibarsin Oct 23, 2017

Choose a reason for hiding this comment

jsotuyod Oct 23, 2017

Choose a reason for hiding this comment

jsotuyod commented Oct 24, 2017 • edited Loading

jsotuyod commented Oct 24, 2017 • edited Loading

adangel commented Oct 28, 2017 • edited Loading

jsotuyod commented Oct 23, 2017 •

edited

Loading

jsotuyod commented Oct 24, 2017 •

edited

Loading

jsotuyod commented Oct 24, 2017 •

edited

Loading

adangel commented Oct 28, 2017 •

edited

Loading