search

janino-compiler / janino

Janino is a super-small, super-fast Java™ compiler.
http://janino-compiler.github.io/janino
Other
1.25k stars 208 forks source link

Use Java's ServiceLoader instead of org.codehaus.commons.compiler.properties #209

Open vlsi opened 10 months ago

vlsi commented 10 months ago

I'm trying to run janino in OSGi environment (Eclipse plugin), and I am facing <<No implementation of org.codehaus.commons.compiler could be loaded. Typically, you'd have "janino.jar", or "commons-compiler-jdk.jar", or both on the classpath, and use the "ClassLoader.getSystemClassLoader" to load them>>

Here's the code that calls getDefaultCompilerFactory: https://github.com/apache/calcite/blob/816edd1fa1a1d6af7e72416d791eb01d8c66b6ea/core/src/main/java/org/apache/calcite/rel/metadata/JaninoRelMetadataProvider.java#L151-L154

Frankly speaking I have no idea why janino uses its own properties resource file to select the implementation, however, it looks like Java's ServiceLoader would do exactly that in a more standard way. ServiceLoader is supported for Java's Modules, and it is supported by OSGi, so I guess it should be less hassle for everybody if janino used ServiceLoader.

WDYT?

Caused by: java.lang.IllegalStateException: Unable to instantiate java compiler
    at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.compile(JaninoRelMetadataProvider.java:156)
    at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.generateCompileAndInstantiate(JaninoRelMetadataProvider.java:138)
    at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.lambda$static$0(JaninoRelMetadataProvider.java:72)
    at com.google.common.cache.CacheLoader$FunctionToCacheLoader.load(CacheLoader.java:169)
    at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3576)
    at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2318)
    at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2191)
    at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2081)
    at com.google.common.cache.LocalCache.get(LocalCache.java:4019)
    at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4042)
    at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5024)
    at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.revise(JaninoRelMetadataProvider.java:189)
    at org.apache.calcite.rel.metadata.RelMetadataQueryBase.revise(RelMetadataQueryBase.java:118)
    at org.apache.calcite.rel.metadata.RelMetadataQuery.collations(RelMetadataQuery.java:605)
    at org.apache.calcite.rel.metadata.RelMdCollation.project(RelMdCollation.java:302)
    at org.apache.calcite.rel.logical.LogicalProject.lambda$create$0(LogicalProject.java:167)
    at org.apache.calcite.plan.RelTraitSet.replaceIfs(RelTraitSet.java:244)
    at org.apache.calcite.rel.logical.LogicalProject.create(LogicalProject.java:166)
    at org.apache.calcite.rel.logical.LogicalProject.create(LogicalProject.java:144)
    at org.apache.calcite.rel.core.RelFactories$ProjectFactoryImpl.createProject(RelFactories.java:204)
    at org.apache.calcite.tools.RelBuilder.project_(RelBuilder.java:2125)
    at org.apache.calcite.tools.RelBuilder.project(RelBuilder.java:1900)
    at org.apache.calcite.tools.RelBuilder.project(RelBuilder.java:1883)
    at org.apache.calcite.tools.RelBuilder.project(RelBuilder.java:1855)
    at org.apache.calcite.tools.RelBuilder.project(RelBuilder.java:1844)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertUnnest(SqlToRelConverter.java:2479)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertFrom(SqlToRelConverter.java:2436)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertFrom(SqlToRelConverter.java:2334)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertFrom(SqlToRelConverter.java:2296)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertJoin(SqlToRelConverter.java:3182)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertFrom(SqlToRelConverter.java:2417)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertFrom(SqlToRelConverter.java:2296)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertSelectImpl(SqlToRelConverter.java:709)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertSelect(SqlToRelConverter.java:690)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertQueryRecursive(SqlToRelConverter.java:3769)
    at org.apache.calcite.sql2rel.SqlToRelConverter.convertQuery(SqlToRelConverter.java:610)
    at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:257)
    at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:220)
    at org.apache.calcite.prepare.CalcitePrepareImpl.prepare2_(CalcitePrepareImpl.java:666)
    at org.apache.calcite.prepare.CalcitePrepareImpl.prepare_(CalcitePrepareImpl.java:519)
    at org.apache.calcite.prepare.CalcitePrepareImpl.prepareSql(CalcitePrepareImpl.java:487)
    at org.apache.calcite.jdbc.CalciteConnectionImpl.parseQuery(CalciteConnectionImpl.java:236)
    at org.apache.calcite.jdbc.CalciteConnectionImpl.prepareStatement_(CalciteConnectionImpl.java:216)
    ... 63 more
Caused by: java.lang.ClassNotFoundException: No implementation of org.codehaus.commons.compiler could be loaded. Typically, you'd have  "janino.jar", or "commons-compiler-jdk.jar", or both on the classpath, and use the "ClassLoader.getSystemClassLoader" to load them.
    at org.codehaus.commons.compiler.CompilerFactoryFactory.getDefaultCompilerFactory(CompilerFactoryFactory.java:83)
    at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.compile(JaninoRelMetadataProvider.java:154)
    ... 105 more

Here's the reproducer: https://github.com/vlsi/mat-calcite-plugin/pull/31

vlsi commented 10 months ago

The workaround is to call org.codehaus.commons.compiler.CompilerFactoryFactory.getDefaultCompilerFactory(org.codehaus.janino.CompilerFactory.class.getClassLoader()))

It would CompilerFactoryFactory initialize with a classloader that contains org.codehaus.commons.compiler.properties (the one with janino.jar). The factory is cached in a static field, so subsequent requests get a workable instance.

aunkrig commented 10 months ago

Hey Vladimir,

I replaced "org.codehaus.commons.compiler.properties" with Java's ServiceLoader as you proposed. ServiceLoader is available since Java 1.6, and Janino started with Java 1.2 -- that's why. But today, Janino requires at least Java 1.7, so it was no problem to switch to ServiceLoader, which is actually 80% identical with Janino's old mechanism.

However, for exactly that reason I'm afraid that the change won't fix your issue - please test!

aunkrig commented 9 months ago

Ping

vlsi commented 9 months ago

It looks like ServiceLoader does not work with most OSGi implementations: https://github.com/jakartaee/websocket/issues/261

For now, I rely on CompilerFactoryFactory.getDefaultCompilerFactory(CompilerFactory.class.getClassLoader()) workaround.

vlsi commented 9 months ago

In any case, it is probably worth adding the following bnd instructions so janino.jar does expose services via OSGi mediators (if mediator is available at all)

<Provide-Capability>osgi.serviceloader;osgi.serviceloader="org.codehaus.commons.compiler.ICompilerFactory"</Provide-Capability>
<Require-Capability>osgi.extender;filter:="(&amp;(osgi.extender=osgi.serviceloader.registrar)(version&gt;=1.0)(!(version&gt;=2.0)))"</Require-Capability>
aunkrig commented 9 months ago

Hey Vladimir, Where do I put these BND bindings?

vlsi commented 9 months ago 
--- a/janino/pom.xml
+++ b/janino/pom.xml
@@ -65,6 +65,8 @@
           <instructions>
                        <Export-Package>org.codehaus.janino, org.codehaus.janino.samples, org.codehaus.janino.tools, org.codehaus.janino.util, org.codehaus.janino.util.resource</Export-Package>
             <Require-Bundle>org.codehaus.janino.commons-compiler</Require-Bundle>
+            <Provide-Capability>osgi.serviceloader;osgi.serviceloader="org.codehaus.commons.compiler.ICompilerFactory"</Provide-Capability>
+            <Require-Capability>osgi.extender;filter:="(&amp;(osgi.extender=osgi.serviceloader.registrar)(version&gt;=1.0)(!(version&gt;=2.0)))"</Require-Capability>
           </instructions>
         </configuration>
       </plugin>

It adds the relevant headers to the manifest, however, I can't test it. My current use case is Eclipse Equinox (I am maintaining a plugin for Eclise Memory Analyzer), and it turns out Equinox does not have serviceloader mediators.

aunkrig commented 9 months ago

Ok, I just merged your untested patch, hoping that it doesn't break anybody's code.

Is there really no way you can test it?

vlsi commented 9 months ago

I think pax-exam can help with testing. I test pgjdbc as follows: https://github.com/pgjdbc/pgjdbc/tree/master/pgjdbc-osgi-test/src/test