asciidoctor / asciidoctorj

:coffee: Java bindings for Asciidoctor. Asciidoctor on the JVM!
http://asciidoctor.org
Apache License 2.0
624 stars 173 forks source link

Verify resource loading with JRuby 9.0.0.0 #362

Open robertpanzer opened 9 years ago

robertpanzer commented 9 years ago

JRuby 1.7 by default uses the TCCL to load resources from Jars like the asciidoctor gem. As converter and extension registry service implementation are also found via the TCCL we changed our recommendation to setup the Asciidoctor instance without passing a ClassLoader to Asciidoctor.Factory.create() but set the TCCL appropriately before invocation of the create() method.

JRuby 9.0.0.0 changed this default as you can see in https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/RubyInstanceConfig.java#L1397-L1407 Instead of the TCCL the default loader is the ClassLoader that loaded the jruby.jar or the class RubyInstanceConfig respectively. The TCCL is only a fallback if this class is loaded via the boot class loader, which would mean to place the jruby.jar into <jre-home>/lib. This change in behavior also becomes apparent in the test WhenClassloaderIsRequired which fails with JRuby 9.0.0.0.

So tools like the ant task or the SBT plugin would have to pass a class loader again, what we just removed. (And I think it's quite strange to pass in the class loader that loaded Asciidoctor to the create method, Asciidoctor.Factory.create() should know which Classloader loaded it;-) )

As a solution I would recommend to let AsciidoctorJ pass the TCCL to RubyInstanceConfig.setLoader() if no other class loader is passed to Asciidoctor.Factory.create(). What do you think?

mojavelinux commented 9 years ago

That seems like a reasonable change to me, but I'd rather hear from others who know better than I do about this topic.

robertpanzer commented 9 years ago

??? Who should that be? :) Besides @lordofthejars I could only imagine discuss this with Mr. JRuby @headius himself about this.

Am 19.07.2015 um 21:35 schrieb Dan Allen notifications@github.com:

That seems like a reasonable change to me, but I'd rather hear from others who know better than I do about this topic.

— Reply to this email directly or view it on GitHub https://github.com/asciidoctor/asciidoctorj/issues/362#issuecomment-122697312.

mojavelinux commented 9 years ago

...anyone else who's watching us. :cold_sweat:

LightGuard commented 9 years ago

:+1: sounds like the right thing to do

On Sunday, July 19, 2015, Dan Allen notifications@github.com wrote:

That seems like a reasonable change to me, but I'd rather hear from others who know better than I do about this topic.

— Reply to this email directly or view it on GitHub https://github.com/asciidoctor/asciidoctorj/issues/362#issuecomment-122697312 .

Sent from Gmail Mobile

headius commented 9 years ago

I think it's the right thing to do as well. JRuby will automatically set TCCL only when run from the command line, in order to support libraries that need it. You'll need to set it under other scenarios.

headius commented 9 years ago

Copying @mkristian as he may have some input here too.

mojavelinux commented 9 years ago

Thanks @headius!

mkristian commented 9 years ago

JRuby always uses the classloader which loaded jruby itself. something like Ruby.class.getClassLoader() in some cases (CLI) this is null then we set it to Thread.currentThread.getContectClassLoader() - this is the 9k setup and I think with javax.script.ScriptEngine as well on 1.7.x

so if you do not set the loader on the config means to let jruby pick the right loader on 9k but in some cases it is the wrong one on 1.7.x. the loader always needs to be the loader which contains jruby itself. defaulting on TCCL is only right when you jruby can be loaded from there.

in CLI case jruby sets the TCCL to be the JRuby.runtime.jruby_class_loader which has the 'loader' from the config as parent. in the CLI case we also assume that jruby can be loaded from TCCL (which is the case for almost all cases via CLI)

just need to mention that inside an OSGi container jruby will not startup when you set its loader to TCCL ! jruby and the majority of gems do work with OSGi containers when you pack them right. in general JRuby9k just works finewhen loaded from any classloader - just add the jruby-complete.jar into a URLClassloader and can have JRuby running isolated from any of your actual application.

mojavelinux commented 9 years ago

Thanks for this incredibly valuable insight, @mkristian!

robertpanzer commented 9 years ago

Thanks for the valuable input! But it doesn't make my life much easier ;-)

I think that I can easily create a scenario with the approach of JRuby 9.0.0.0 on top of Java EE where JRuby will not find the asciidoctor gem. On the other hand, as far as I have seen on the felix container, OSGi does not set a TCCL. That's where we got the NPEs in JRuby 1.7.

So I think the current approach of my PR would match most cases:

  1. Set the TCCL on RubyInstanceConfig.setLoader() if it is != null
  2. Don't call RubyInstanceConfig.setLoader() if TCCL is null

In the end the user still has the possibility to pass a classloader to Asciidoctor.Factory.create() that overrides the behavior mentioned above.

mkristian commented 9 years ago

@robertpanzer regarding the jruby 9k and J2EE and JRuby not finding the gem is something I want to see. I want at least understand why it is not working and maybe fix JRuby to work with this scenario out of the box as well. but NOT finding a gem is a different topic from the classloader where JRuby is located.

for jruby-9K the ScriptingContainer or ScriptEngine or IsolatedScriptingContainer just work out of the box. if not it is a bug on JRuby! actually the IsolatedScriptingContainer does mimic the JRuby-9k behaviour on jruby-1.7.x. and it is used by the JRuby's integration test for OSGi and J2EE for both 9K as well 1.7.x. it makes sure that JRuby can loads its kernel as well all the stdlib including all embedded gems for OSGi as well J2EE setups. again if you have a case where IsolatedScriptingContainer is not doing it right then please file a bug on JRuby

being able to set the classloader on 9K is not needed unless you ran into a bug.

not finding a gem on J2EE or OSGi needs some extra work on packing them "right" - there is at least a maven and a gradle plugin helping here.

mkristian commented 9 years ago

I need to mention (since I did not check the ascidoctorj code) that the IsolatedScriptingContainer on jruby-1.7.x still has a few short comings regarding what you can configure after you created on instance of it.

robertpanzer commented 9 years ago

Hi Christian,

thanks for your feedback! I am so happy that we can discuss this as there seem to be multiple ways to reach our goal and we have no real proof yet that we are on the best track yet.

I constructed a simple JavaEE application that shows the diffs between JRuby 1.7 and 9.0.0.0. It is an ear where JRuby is in the ear/lib and the asciidoctor.jar is in a war module in that ear:

adtestear.ear
 +- adtestwar.war
 |   +- WEB-INF/classes/org/asciidoctor/AsciidoctorServlet.class
 |   +- WEB-INF/lib/asciidoctorj-1.5.2.jar // The asciidoctor gem is in here!!!
 +- lib/jruby-complete-xyz.jar

Without setting a loader the example works with JRuby 1.7 because it takes the TCCL which is the classloader for the war, but fails with 9.0.0.0 which takes the classloader that loaded Ruby, and that's the ear loader, not the war loader. If you like you can try it out yourself, I pushed the example with a little README to GitHub: https://github.com/robertpanzer/asciidoctorj-jruby-test

Thanks for the hint to the IsolatedScriptingContainer, I didn't know that one yet. At the moment we create new Ruby runtimes via JavaEmbedUtils.initialize(). I don't feel that IsolatedScriptingContainer would be the right thing for us. It extends ScriptingContainer and if I understood correctly that one per default holds a singleton Ruby runtime or you can configure to manage a Runtime per thread. But both options are not really what we want. We need full control over the Ruby runtimes as we need one per AsciidoctorJ instance.

mkristian commented 9 years ago

@robertpanzer thanx for all the input and the repo

the j2ee repo boils down to:

which is very similar to an osgi setup. with the j2ee example of yours the asciidoctorj classloader is the TCCL and setting this will help in the j2ee setup.

with OSGi the TCCL is not used in general but can be used by web-archive-bundles or some bundles do set it to use libraries which depends on TCCL.

the whole JavaEmbedUtils class was under my radar :)

with your example and what the previous work to improve OSGi support made me come up with this:

https://github.com/jruby/jruby/blob/test-add-classloader-to-instance-config/core/src/main/java/org/jruby/RubyInstanceConfig.java#L708

so instead of replacing the classloader of JRuby core/stdlib you can add the classloader of asciidoctorj jar to jruby. this adding is done in a way that embedded gems and ruby scripts can be found on the given classloader.

I did not use your ear-example to build an integration test for jruby but I will when my time permits it. in the meanwhile there are a few tests using this new feature:

https://github.com/jruby/jruby/blob/test-add-classloader-to-instance-config/maven/jruby-complete/src/templates/osgi_many_bundles_with_embedded_gems/test/src/test/java/org/jruby/embed/osgi/test/JRubyOsgiEmbedTest.java#L84

where the addClassLoader method on container uses the addLoader on the underlying instance config.

or https://github.com/jruby/jruby/blob/test-add-classloader-to-instance-config/core/src/test/java/org/jruby/javasupport/JavaEmbedUtilsTest.java

for asciidoctorj this means the right thing to do would be (9.0.1.0-SNAPSHOT):

config.addLoader(getClass().getClassLoader());

this would make the jruby setup J2EE and OSGi usable.

in case you want to give this approach a trial I uploaded a 9.0.1.0-SNAPSHOT to https://oss.sonatype.org/content/repositories/snapshots/ as I did not test it on your repo.

any feedback is welcome and the j2ee example of yours was very helpful indeed ;)

robertpanzer commented 9 years ago

Wow, thank you, Christian! This looks really good, I will try as soon as I find the time for it.

I absolutely like the idea: Being able to simply add classloaders greatly simplifies things.

Sanne commented 7 years ago

Hi all, I would love to see a solution for this merged, as it is a pre-requisite for #515 Thanks!