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

Try to load class from all of packages as classpath scanning doesn't work in Eclipse runtime environment #4916

Closed
lkoe opened this Issue Aug 8, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@lkoe
Member

lkoe commented Aug 8, 2017

I am the maintainer of the Checkstyle Eclipse plugin (http://eclipse-cs.sourceforge.net/).
This is a followup to #4885, indeed the error message was misleading.

After some in-depth debugging I came to the conclusion that the classpath scanning, introduced with #3607 just does not work in more complex classloading environments such as Eclipse - which is an OSGi runtime platform.
Eclipse/OSGI requires all functionality to be provided as OSGi bundle/plugin. Classloading for each bundle is isolated by specialized bundle classloaders which encapsulate which classes a plugin/bundle "sees". This requires both Checkstyle core and potential Checkstyle extension to be provided as plugin/bundle.

The classpath scanning uses the Classpath facility from Guava, where documentation clearly states that it only supports URLClassloaders. However OSGi bundle classloading is not based on URLClassloaders, thus the classpath scanning is not able to find any custom checks in Checkstyle extensions.
Checkstyle core still works because of hardwired lookup tables maintained in code.

Would you be open to discuss the current approach and a potential return the the "old brute-force" extension classloading?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 8, 2017

Member

Would you be open to discuss

We are always open to discussions.

There is no way to give a specialized ClassLoader to Checkstyle to help in identifying these external classes?

"old brute-force"
is not able to find any custom checks in Checkstyle extensions

You are saying Class.for finds the classes? So it breaks out of Eclipse's encapsulation?
If you define the full classpath in the Checkstyle configuration, it loads the 3rd party module without issue, right? It is just short names it fails?

Also, is there some way to observe this behavior in a standalone environment that we can reproduce and see?

Member

rnveach commented Aug 8, 2017

Would you be open to discuss

We are always open to discussions.

There is no way to give a specialized ClassLoader to Checkstyle to help in identifying these external classes?

"old brute-force"
is not able to find any custom checks in Checkstyle extensions

You are saying Class.for finds the classes? So it breaks out of Eclipse's encapsulation?
If you define the full classpath in the Checkstyle configuration, it loads the 3rd party module without issue, right? It is just short names it fails?

Also, is there some way to observe this behavior in a standalone environment that we can reproduce and see?

@lkoe

This comment has been minimized.

Show comment
Hide comment
@lkoe

lkoe Aug 9, 2017

Member

There is no way to give a specialized ClassLoader to Checkstyle to help in identifying these external classes?

Of course there is, eclipse-cs passes a specially crafted Classloader to Checkstyle core to use as module classloader (https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/util/ExtensionClassLoader.java). It is "just" the case that this classloader is not (an cannot be based) on URLClassloader, thus the classpath scanning doesn't work.

You are saying Class.for finds the classes? So it breaks out of Eclipse's encapsulation?
If you define the full classpath in the Checkstyle configuration, it loads the 3rd party module without issue, right? It is just short names it fails?

Traditionally Checkstyle used moduleClassloader.loadClass(String) to load check/module classes. Using the above referenced classloader implementation this worked perfectly well within the boundaries of the OSGi runtime. And, yes, it's "just" the check discovery based on short/simple names where it fails, although that's likely the 99% usecase.

Also, is there some way to observe this behavior in a standalone environment that we can reproduce and see?

No, I supposed there is no easy way to observe this in a standalone environment.

The "old" implementation in Checkstyle core was based on 2 very simple assuptions:

  • integrators of the Checkstyle library provide a classloader, which is able to load both Checkstyle core and extension modules
  • module classnames are constructed by convention: <packagename>+<simplename>+ "Check" or <packagename>+<simplename>
    This simple convention worked well for both checks and other modules (Treewalker, *Holder).

The new implementation on the other side is oddly more complex:

  • the module classloader provided must be based on URLClassloader specifically (otherwise guava Classpath falls over) - an assumption which is not true for all integration scenarios
  • a registry of Checkstyle core checks is maintained in code (violation of DRI principle)

Personally I find the new classpath scanning (even if it worked in all cases) to provide no tangible benefits over the old mechanism. Reading #3607 also provides no real rationale behind this change.

Member

lkoe commented Aug 9, 2017

There is no way to give a specialized ClassLoader to Checkstyle to help in identifying these external classes?

Of course there is, eclipse-cs passes a specially crafted Classloader to Checkstyle core to use as module classloader (https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/util/ExtensionClassLoader.java). It is "just" the case that this classloader is not (an cannot be based) on URLClassloader, thus the classpath scanning doesn't work.

You are saying Class.for finds the classes? So it breaks out of Eclipse's encapsulation?
If you define the full classpath in the Checkstyle configuration, it loads the 3rd party module without issue, right? It is just short names it fails?

Traditionally Checkstyle used moduleClassloader.loadClass(String) to load check/module classes. Using the above referenced classloader implementation this worked perfectly well within the boundaries of the OSGi runtime. And, yes, it's "just" the check discovery based on short/simple names where it fails, although that's likely the 99% usecase.

Also, is there some way to observe this behavior in a standalone environment that we can reproduce and see?

No, I supposed there is no easy way to observe this in a standalone environment.

The "old" implementation in Checkstyle core was based on 2 very simple assuptions:

  • integrators of the Checkstyle library provide a classloader, which is able to load both Checkstyle core and extension modules
  • module classnames are constructed by convention: <packagename>+<simplename>+ "Check" or <packagename>+<simplename>
    This simple convention worked well for both checks and other modules (Treewalker, *Holder).

The new implementation on the other side is oddly more complex:

  • the module classloader provided must be based on URLClassloader specifically (otherwise guava Classpath falls over) - an assumption which is not true for all integration scenarios
  • a registry of Checkstyle core checks is maintained in code (violation of DRI principle)

Personally I find the new classpath scanning (even if it worked in all cases) to provide no tangible benefits over the old mechanism. Reading #3607 also provides no real rationale behind this change.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 9, 2017

Member

eclipse-cs passes a specially crafted Classloader to Checkstyle core to use as module classloader

Issue #3773
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/CheckerFactory.java#l290

So I assume this issue would break eclipse-cs more even if the current problem didn't as you need to give us a special classloader to find these classes/resources?
If this is true, please also make a post on that issue.

Personally I find the new classpath scanning (even if it worked in all cases) to provide no tangible benefits over the old mechanism.

We considered old approach slightly ugly as we were testing all theories until something hit. New approach also validated the class structure to verify it was considered a module by us.
Although there is no new benefit right now, it could allow us to display all modules defined in the classpath if we ever implement a feature notifying users of new modules not listed in their configurations and such.

I want to look into this issue more, I remember URLClassloader was a special case for reflection hacks, but I don't know all details and would like to see if there was another workaround as I don't think @romani will like going back to old approach but I don't know another way at this point.
If you have any other ideas, please share.

Member

rnveach commented Aug 9, 2017

eclipse-cs passes a specially crafted Classloader to Checkstyle core to use as module classloader

Issue #3773
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/CheckerFactory.java#l290

So I assume this issue would break eclipse-cs more even if the current problem didn't as you need to give us a special classloader to find these classes/resources?
If this is true, please also make a post on that issue.

Personally I find the new classpath scanning (even if it worked in all cases) to provide no tangible benefits over the old mechanism.

We considered old approach slightly ugly as we were testing all theories until something hit. New approach also validated the class structure to verify it was considered a module by us.
Although there is no new benefit right now, it could allow us to display all modules defined in the classpath if we ever implement a feature notifying users of new modules not listed in their configurations and such.

I want to look into this issue more, I remember URLClassloader was a special case for reflection hacks, but I don't know all details and would like to see if there was another workaround as I don't think @romani will like going back to old approach but I don't know another way at this point.
If you have any other ideas, please share.

@lkoe

This comment has been minimized.

Show comment
Hide comment
@lkoe

lkoe Aug 9, 2017

Member

#3773 should be no problem, since that classloader and the module classloader (Checker.setModuleClassloader) are different ones, see
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/CheckerFactory.java#l275

The classloader used referenced in #3773 is needed to load project classes - as opposed to Checkstyle/extension classes loaded by the module classloader.

We considered old approach slightly ugly...

I may be perceived ugly, but is was also much simpler implementation wise, more robust and made little assumptions about the integration enviroment. Loading classes via classloaders has been a Java standard mechanism since Java 1.0 - while classpath scanning up until today is not.

Member

lkoe commented Aug 9, 2017

#3773 should be no problem, since that classloader and the module classloader (Checker.setModuleClassloader) are different ones, see
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/CheckerFactory.java#l275

The classloader used referenced in #3773 is needed to load project classes - as opposed to Checkstyle/extension classes loaded by the module classloader.

We considered old approach slightly ugly...

I may be perceived ugly, but is was also much simpler implementation wise, more robust and made little assumptions about the integration enviroment. Loading classes via classloaders has been a Java standard mechanism since Java 1.0 - while classpath scanning up until today is not.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 9, 2017

Member

Some info as I am looking.

The classpath scanning uses the Classpath facility from Guava, where documentation clearly states that it only supports URLClassloaders.

https://github.com/google/guava/blob/6290373588ef914111f0896b8d0c86a15b7f0f6b/guava/src/com/google/common/reflect/ClassPath.java#L58-L59

Warning: Currently only {@link URLClassLoader} and only {@code file://} urls are supported.

https://github.com/google/guava/blob/6290373588ef914111f0896b8d0c86a15b7f0f6b/guava/src/com/google/common/reflect/ClassPath.java#L435
https://github.com/google/guava/blob/6290373588ef914111f0896b8d0c86a15b7f0f6b/guava/src/com/google/common/reflect/ClassPath.java#L438
Unless it is a URLClassLoader (or one of it's parent) and it's URLs point to a file, it just ignores the classloader and returns an empty list.

https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/CheckerFactory.java#l275
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/util/ExtensionClassLoader.java#l51
Eclipse-cs class loader has no defined parent, so it uses ClassLoader.getSystemClassLoader as the parent.

https://docs.oracle.com/javase/7/docs/api/java/lang/ClassLoader.html
https://docs.oracle.com/javase/7/docs/api/java/net/URLClassLoader.html
The JRE only has URLClassLoader as an implementation.

https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/util/ExtensionClassLoader.java#l43
It looks like Eclipse-cs uses bundles to load classes.
https://stackoverflow.com/a/37895588/1016482
https://stackoverflow.com/a/22800118/1016482
You can scan a bundle for all classes and load them this way and they are returned as URLs.
@lkoe Could you make a custom URLClassLoader from these bundles and their returned URLs?
I will try later tonight to setup a workspace for eclipse-cs.

Member

rnveach commented Aug 9, 2017

Some info as I am looking.

The classpath scanning uses the Classpath facility from Guava, where documentation clearly states that it only supports URLClassloaders.

https://github.com/google/guava/blob/6290373588ef914111f0896b8d0c86a15b7f0f6b/guava/src/com/google/common/reflect/ClassPath.java#L58-L59

Warning: Currently only {@link URLClassLoader} and only {@code file://} urls are supported.

https://github.com/google/guava/blob/6290373588ef914111f0896b8d0c86a15b7f0f6b/guava/src/com/google/common/reflect/ClassPath.java#L435
https://github.com/google/guava/blob/6290373588ef914111f0896b8d0c86a15b7f0f6b/guava/src/com/google/common/reflect/ClassPath.java#L438
Unless it is a URLClassLoader (or one of it's parent) and it's URLs point to a file, it just ignores the classloader and returns an empty list.

https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/CheckerFactory.java#l275
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/util/ExtensionClassLoader.java#l51
Eclipse-cs class loader has no defined parent, so it uses ClassLoader.getSystemClassLoader as the parent.

https://docs.oracle.com/javase/7/docs/api/java/lang/ClassLoader.html
https://docs.oracle.com/javase/7/docs/api/java/net/URLClassLoader.html
The JRE only has URLClassLoader as an implementation.

https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/util/ExtensionClassLoader.java#l43
It looks like Eclipse-cs uses bundles to load classes.
https://stackoverflow.com/a/37895588/1016482
https://stackoverflow.com/a/22800118/1016482
You can scan a bundle for all classes and load them this way and they are returned as URLs.
@lkoe Could you make a custom URLClassLoader from these bundles and their returned URLs?
I will try later tonight to setup a workspace for eclipse-cs.

@lkoe

This comment has been minimized.

Show comment
Hide comment
@lkoe

lkoe Aug 9, 2017

Member

The correct resources from eclipse-cs are those:
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/util/ExtensionClassLoader.java
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/CheckerFactory.java#l275

Please don't mix up ProjectClassloader and ExtensionClassloader - the first is used to load project classes used by certain checks (e.g. JavadocMethod), the latter is used to load Checkstyle module classes.

Member

lkoe commented Aug 9, 2017

The correct resources from eclipse-cs are those:
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/util/ExtensionClassLoader.java
https://sourceforge.net/p/eclipse-cs/git/ci/master/tree/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/CheckerFactory.java#l275

Please don't mix up ProjectClassloader and ExtensionClassloader - the first is used to load project classes used by certain checks (e.g. JavadocMethod), the latter is used to load Checkstyle module classes.

@lkoe

This comment has been minimized.

Show comment
Hide comment
@lkoe

lkoe Aug 9, 2017

Member

@lkoe Could you make a custom URLClassLoader from these bundles and their returned URLs?

No it just doesn't work this way in OSGi, a bundle classpath simply does not consist of a mere list of jars - but is constructed meticulously to isolate bundles from each other. A bundles manifest states which packages are to be imported by a bundle and only those packages are visible to the bundle.

The is no notion of a "jar" in a bundle classpath - it just doesn't work that way. Hence it is impossible to artifically construct a URLClassloader in an OSGi runtime.

Member

lkoe commented Aug 9, 2017

@lkoe Could you make a custom URLClassLoader from these bundles and their returned URLs?

No it just doesn't work this way in OSGi, a bundle classpath simply does not consist of a mere list of jars - but is constructed meticulously to isolate bundles from each other. A bundles manifest states which packages are to be imported by a bundle and only those packages are visible to the bundle.

The is no notion of a "jar" in a bundle classpath - it just doesn't work that way. Hence it is impossible to artifically construct a URLClassloader in an OSGi runtime.

@mickaelistria

This comment has been minimized.

Show comment
Hide comment
@mickaelistria

mickaelistria Aug 31, 2017

The is no notion of a "jar" in a bundle classpath - it just doesn't work that way. Hence it is impossible to artifically construct a URLClassloader in an OSGi runtime.

It would be dirty, and not very conform to OSGi, and error-prone as it wouldn't handle the case of multiple versions of the same class properly, but you can use somethjing like new URLClassLoader(Arrays.stream(someBundle.getBundleContext().getBundles()).map(Bundle::getLocation).filter(loc -> loc.startsWith("reference:file:").map(loc -> loc.substring("resference:file:".length()).map(File::new).map(File::toURL).toArray()) to get a classloader that would be a bit like the one used at runtime by the bundles.
But in any case, it would definitely be better to have checkstyle not relying on all classloaders to be URLClassLoader nor on anything else that assumes that. Simply because it's so wrong!

mickaelistria commented Aug 31, 2017

The is no notion of a "jar" in a bundle classpath - it just doesn't work that way. Hence it is impossible to artifically construct a URLClassloader in an OSGi runtime.

It would be dirty, and not very conform to OSGi, and error-prone as it wouldn't handle the case of multiple versions of the same class properly, but you can use somethjing like new URLClassLoader(Arrays.stream(someBundle.getBundleContext().getBundles()).map(Bundle::getLocation).filter(loc -> loc.startsWith("reference:file:").map(loc -> loc.substring("resference:file:".length()).map(File::new).map(File::toURL).toArray()) to get a classloader that would be a bit like the one used at runtime by the bundles.
But in any case, it would definitely be better to have checkstyle not relying on all classloaders to be URLClassLoader nor on anything else that assumes that. Simply because it's so wrong!

@InnateDistress

This comment has been minimized.

Show comment
Hide comment
@InnateDistress

InnateDistress Oct 27, 2017

Is there an estimate for when this issue will be resolved?

InnateDistress commented Oct 27, 2017

Is there an estimate for when this issue will be resolved?

kazachka added a commit to kazachka/checkstyle that referenced this issue Oct 30, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Nov 12, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Nov 14, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Nov 14, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Nov 24, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Nov 24, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Nov 25, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Nov 25, 2017

rnveach added a commit that referenced this issue Nov 26, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 26, 2017

Member

Fix was merged.

@lkoe eclipse-cs must create custom PackageObjectFactory and pass it ModuleLoadOption.TRY_IN_ALL_REGISTERED_PACKAGES to use old brute force.

Member

rnveach commented Nov 26, 2017

Fix was merged.

@lkoe eclipse-cs must create custom PackageObjectFactory and pass it ModuleLoadOption.TRY_IN_ALL_REGISTERED_PACKAGES to use old brute force.

@rnveach rnveach closed this Nov 26, 2017

@rnveach rnveach added this to the 8.5 milestone Nov 26, 2017

@romani romani added the new feature label Nov 26, 2017

@romani romani changed the title from Classpath scanning doesn't work in Eclipse runtime environment to Try to load class from all of packages as classpath scanning doesn't work in Eclipse runtime environment Nov 26, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani
Member

romani commented Nov 26, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

@lkoe

This comment has been minimized.

Show comment
Hide comment
@lkoe

lkoe Dec 20, 2017

Member

Thanks, eclipse-cs 8.5.0 based on Checkstyle 8.5 has been released. Custom checks should be working again (tried it with my sample extension).

Member

lkoe commented Dec 20, 2017

Thanks, eclipse-cs 8.5.0 based on Checkstyle 8.5 has been released. Custom checks should be working again (tried it with my sample extension).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment