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

[vlsi]

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: vlsi/mat-calcite-plugin#31

[vlsi]

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]

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]

Ping

[vlsi]

It looks like ServiceLoader does not work with most OSGi implementations: jakartaee/websocket#261

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

[vlsi]

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]

Hey Vladimir,
Where do I put these BND bindings?

[vlsi]

--- 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]

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]

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