#383 Make classloader used by PackageObjectFactory configurable

release_4.0
closed
Oliver Burn
5
2012-10-10
2005-08-26
No

According to the discussion on the developer mailing list
I need to change the classloader used by the
PackageObjectFactory.

Currently the classloader with which the
PackageObjectFactory class itself was loaded is used
as well to load the modules.
I want to provide a feature for the eclipse-cs plugin that
allows the user to just drop in his custom jar in a specific
location.
The plugin will then automatically discover these custom
libraries - so no addtional configuration inside the plugin
needs to be done.

The easiest solution occurring to me was if the
PackageObjectFactory would use the contextclassloader
of the current thread (Thread.currentThread().
getContextClassLoader) to load the modules.
This way I only need to change this context classloader
before configuring a checker - and all custom modules
will be found.

No additional API would be needed with this aproach.

Discussion

  • Logged In: YES
    user_id=1238882

    I appended a patch that includes the suggested solution.
    Additional it contains a similar patch for the
    LocalizedMessage class.
    The problem there was too, that the class is using the 'wrong'
    classloader to find the message bundle, which led to the
    following bug:
    http://sourceforge.net/tracker/index.php?
    func=detail&aid=1292831&group_id=80344&atid=559494

    Since I see currently no way of working around the second
    problem I would be really happy if the patch (or a similar
    solution) could make it into Checkstyle 4.0.

    Regards,
    Lars Kdderitzsch

     
  • Oliver Burn
    Oliver Burn
    2005-09-21

    Logged In: YES
    user_id=218824

    I will work on this over the next few days. It is actually a
    non-trivial problem and requires we use class loaders
    consistently in all the code.

     
  • Ralf
    Ralf
    2005-09-25

    Logged In: YES
    user_id=383599

    Hello, I work in a similar project as Lars. Here are my ideas:

    1) PackageObjectFactory:
    As Lars mentioned the classloader should be setable. I also
    would like a feature that additional checkstyle_package.xml
    could be parsed.
    I achieved that by implementing my own PackageObjectFactory.

    2) LocalizedMessage
    The constructor gets the Class of the check that is emitting
    that message. Just use the classloader of this class to load
    the ResourceBundle. Either load it in the constructor or
    store a reference to the class to get the classname and the
    classloader later.

    Regards
    Ralf

     
  • Oliver Burn
    Oliver Burn
    2005-09-25

    Logged In: YES
    user_id=218824

    I have checked in a change where the Class Loader used to
    load infrastructure classes and resources comes from
    Thread.currentThread().getContextClassLoader(). The
    expection is that if plug-in developers want to change where
    the class loader used, then they will set the context class
    loader.

    Lars, can you please verify that this change works for you.

     
  • Logged In: YES
    user_id=1238882

    Great! Thank you very much, Oliver!
    Sourceforge CVS is currently crapping out on me - I will
    confirm as soon as possible.

     
  • Ralf
    Ralf
    2005-09-25

    Logged In: YES
    user_id=383599

    Oliver, Lars,

    I tested the solution with the Checkclipse plugin and my
    custom checks are now loaded successfully. Unfortunally the
    messages
    bundles are not loaded (they also would need to use the
    contextClassLoader).

    Drawback: You have to set the contextClassLoader, else
    Checkstyle doesn't work. This will also affect plugins for
    other environments than eclipse!

    I propose to postpone this RFE to post 4.0, use a custom
    PackageObjectFactory for now and fix the LocalizedMessage as
    I mentioned below.

    Lars, I can provide code for a custom PackageObjectFactory.

    Regards
    Ralf

     
  • Logged In: YES
    user_id=1238882

    Ralf is right with the message bundles not being loaded,
    because the classloader of the LocalizedMessage class is
    being used.
    There are two valid solutions I see:
    1. Like Ralf suggested use the classloader from the actual
    Check class to load the bundle - this would ensure that the
    classloader which loaded the check class is also used to load
    the bundle
    2. Use the context classloader of the current thread to load
    the bundle as suggested with the attached patch.

    For the mentioned drawback:
    I don't think that the changes will affect other plugins as it is
    not mandatory to set the context classloader. In case the
    context classloader is not set the original classloader is used
    as before.

     
  • Ralf
    Ralf
    2005-09-26

    Logged In: YES
    user_id=383599

    Ok, I checked again.

    Till now Checkclipse never used setContextClassLoader().
    If I use the current Checkstyle code from CVS I get a
    CheckstyleException stating:

    unable to parse
    C:\Workspaces\eclipse3-WS\runtime-workspace\XMsgPlugin\checkstyle.xml
    - Unable to load internal dtd
    com/puppycrawl/tools/checkstyle/configuration_1_1.dtd

    Looks like the classes are loaded, but the resources are
    not. By setting a ContextClassLoader this problem goes away.

    Strange ...

    Ralf

     
  • Logged In: YES
    user_id=1238882

    Strange indeed, because if you don't set a special context
    classloader the standard EclipseClassloader (which loads
    from the plugin classpath) is the actual context classloader.

    Source of the problem could be if the checkstyle jar(s) are not
    on your plugin classpath.

     
  • Ralf
    Ralf
    2005-09-26

    Logged In: YES
    user_id=383599

    Lars, I get the exception on the following line:

    ConfigurationLoader.loadConfiguration(configFileName, new
    PropertiesExpander(properties));

    The ConfigurationLoader and the PropertiesExpander are
    classes from the Checkstyle Jars. This is something that
    really confuses me.

    BTW: setting the ContextClassLoader to the loader of the
    current class also solves the problem.

    Thread.currentThread().setContextClassLoader(this.getClass().getClassLoader());

    I looked at the default ContextClassLoader and I didn't find
    anything pointing to my Plugin or to Eclipse. I don't know
    the classloader internals, but for me it looks like the
    ClassLoader used to start Eclipse.

    Ralf

     
  • Ralf
    Ralf
    2005-09-26

    Logged In: YES
    user_id=383599

    Lars, I get the exception on the following line:

    ConfigurationLoader.loadConfiguration(configFileName, new
    PropertiesExpander(properties));

    The ConfigurationLoader and the PropertiesExpander are
    classes from the Checkstyle Jars. This is something that
    really confuses me.

    BTW: setting the ContextClassLoader to the loader of the
    current class also solves the problem.

    Thread.currentThread().setContextClassLoader(this.getClass().getClassLoader());

    I looked at the default ContextClassLoader and I didn't find
    anything pointing to my Plugin or to Eclipse. I don't know
    the classloader internals, but for me it looks like the
    ClassLoader used to start Eclipse.

    Ralf

     
  • Logged In: YES
    user_id=1238882

    Ralf, you're right with the default contextclassloader, my
    mistake.
    So its true that plugin providers need to set the context
    classloader. This needs to be documented - so they won't
    miss this point.

    Nevertheless, I think the 4.0 would be a good point to
    introduce such a change - because this certainly will not be
    possible in the following minor releases.

     
  • Ralf
    Ralf
    2005-09-26

    Logged In: YES
    user_id=383599

    Ok, I agree that the ContextClassLoader fixes our problem
    for now, but I'm not convinced that it is the final solution
    to this problem.

    IMHO there is more to come, as Checkstyle should be able to
    read multiple checkstyle_package.xml files. This would make
    the job easier for developers writing own checks. See my RFE
    1304528.

    The ContextClassLoader seems like a temporary fix for our
    two plugins, but perhapse it is better to use the workaround
    I outlined below for now and rethink the entire area.

    In the end it's the choice of the Checkstyle team.

    Ralf

     
  • Ralf
    Ralf
    2005-09-26

    Logged In: YES
    user_id=383599

    I just saw that this problem is also addressed by eclipse
    itself (at least for V 3.1).

    See Eclipse Bugzilla Bug 87775 "Overcoming classloading
    issues with third party libraries".

    Description:
    This bug is an umbrella for gathering solutions to
    integrating various third party libraries (e.g., hibernate,
    log4j, prevayler, ...) into Eclipse. Various of these use
    the context classloader or reflection to load classes.
    These are sometimes at odds with the Eclipse/OSGi component
    model's desire to structure classloading. While there are
    typically ways around the issues, they are sometimes not
    obvious to newcomers and so represent a barrier to adoption.
    Further, in the 3.1 timeframe we are actively looking at
    ways for the runtime to make life easier.

     
  • Ralf
    Ralf
    2005-09-27

    Logged In: YES
    user_id=383599

    Forgot the following:

    Eclipse bug 87775 "Overcoming classloading issues with third
    party libraries" is marked as fixed for Eclipse release 3.1.

    The introduced solution is called "Buddy Loader".

    Haven't tried it as I'm still using Eclipse 3.0.1

    Ralf

     
  • Logged In: YES
    user_id=1238882

    Buddy classloading in Eclipse is only useful if a user would
    like to provide a plugin that extends the eclipse checkstyle
    plugin (whichever) to integrate a custom jar.
    In this case the checkstyle plugin needs to load the check
    classes from the extending plugin - through the use of the
    buddy mechanism.

    However, this does not apply with the eclipse-cs plugin as the
    suggested ways to provide custom checks are:
    a) drop you jar in a special location inside the plugin dir
    b) provide a fragment to the plugin containing your custom jar

    One suggestion:
    I think going so deep into the eclipse magic of classloading is
    probably not so interesting for the Checkstyle folks.
    If there is more to discuss I would like to invite you to continue
    this specific discussion on the eclipse-cs mailing list or
    forums.

     
  • Logged In: YES
    user_id=1238882

    Thank you, Ralf and Oliver, for providing and applying the
    patch to LocalizedMessage.

    From my point of view this RFE can be closed as fixed, but
    considering all comments there might be different opinions on
    that.

     
  • Oliver Burn
    Oliver Burn
    2005-10-06

    Logged In: YES
    user_id=218824

    Thanks I believe this is also fixed. Ralf, please let me
    know if I am missing something.