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

[core] LanguageRegistry uses default class loader when invoking ServiceLocator #1377

Closed
uhafner opened this Issue Oct 9, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@uhafner

uhafner commented Oct 9, 2018

Affects PMD Version:
all (6.8.0)

Description:

In Jenkins warnings plugin I am using PMD core to read and print the descriptions of the warnings rules. I use the class RuleSetFactory to read the messages. In order to get the resources loaded I would like to provide Jenkins' classloader to the RuleSetFactory. This seems to be possible using the 4-arg constructor. However, this currently does not work since the internal call to LanguageRegistry uses the system default class loader. In order to get this running now, I need to use a "hack": I initialize the rules before Jenkins provides the correct class loader to my plug-in. This might change in the future.

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 9, 2018

@uhafner I'm having a little trouble understanding your scenario.

You seem to be building a Jenkins plugin, which uses PMD. PMD is hence a direct dependency of your plugin and bundled with it (ie: present in the same classloader that loads and runs the plugin).

Jenkins is probably creating a separate classloader to run the plugin (as it's standard practice, to avoid conflicts on different versions of the same dependency), but that classloader probably references the Jenkins classloader as parent classloader to share the Jenkins API classes. Is this assumption correct?

In that scenario, anything in the Jenkins classloader is available to your plugin in the default classloader. And even without the parent classloader, if a classloader loaded your plugin's classes, it MUST have access to the PMD classes (as it's bundled together, and the fact you are not getting a NoClassDefError is proof of that), so the default classloader should simply work, you shouldn't need to specify a classloader yourself. What is exactly going on?

@uhafner

This comment has been minimized.

uhafner commented Oct 9, 2018

If you don't specify a class loader when using Java's ServiceLoader then 'Thread.currentThread().getContextClassLoader()' will be used. This does not work in server applications like Jenkins. See Jenkins Developer Documentation.

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 9, 2018

@uhafner thanks for the info! if I understand correctly, changing:

ServiceLoader<Language> languageLoader = ServiceLoader.load(Language.class);

to:

ServiceLoader<Language> languageLoader = ServiceLoader.load(Language.class, getClass().getClassLoader());

would be enough, as Jenkins loads the classes from the proper classloader, it's simply not the default context one for the current thread, right?

@uhafner

This comment has been minimized.

uhafner commented Oct 9, 2018

I'm not a class loader expert but from my understanding it should work in that way. Kohsuke suggested to expose the class loader as a parameter in a library but this might not be needed.

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 10, 2018

@uhafner ok. I don't think we can easily test that, but if we introduce the change, would you be willing to test against a snapshot build?

@jsotuyod jsotuyod added this to the 6.9.0 milestone Oct 10, 2018

@uhafner

This comment has been minimized.

uhafner commented Oct 10, 2018

Yes, of course. I already have a test case (integration test) that verifies this.

@adangel

This comment has been minimized.

Member

adangel commented Oct 15, 2018

@uhafner The fix has been integrated now. Could you test, whether the problem is gone now?

Simply use PMD version 6.9.0-SNAPSHOT from

        <repository>
            <id>sonatype-nexus-snapshots</id>
            <name>Sonatype Nexus Snapshots</name>
            <url>https://oss.sonatype.org/content/repositories/snapshots</url>
            <releases>
                <enabled>false</enabled>
            </releases>
            <snapshots>
                <enabled>true</enabled>
            </snapshots>
        </repository>

Thanks!

@uhafner

This comment has been minimized.

uhafner commented Oct 17, 2018

I’m in holidays until end of October. As soon as I am online again I will test the fix.

@uhafner

This comment has been minimized.

uhafner commented Oct 30, 2018

I've tested my plug-in with the SNAPSHOT now, it works as expected. Thanks for fixing!

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 30, 2018

@uhafner awesome! this was shipped in 6.9.0 on Sunday, you can use the release version now.

Thanks for testing this!

@uhafner

This comment has been minimized.

uhafner commented Oct 30, 2018

Ah, even better, then I can integrate it right now 👍

uhafner added a commit to jenkinsci/warnings-ng-plugin that referenced this issue Oct 30, 2018

Upgrade to RC12 of analysis-model and beta9 of analysis-model-api.
Upgrade of additional dependency versions.
Removed PMD messages initialization from plugin class,
see pmd/pmd#1377
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment