twosigma / beakerx

Beaker Extensions for Jupyter Notebook
http://BeakerX.com
Apache License 2.0
2.8k stars 382 forks source link

ServiceLoader does not find any services when using dynamic classpath ( Java / Groovy ) #2276

Closed dkobylarz closed 8 years ago

dkobylarz commented 9 years ago

Using libraries that load some services via SPI (like Lucene codecs) is impossible when using Class path option, both java and groovy language. Since beakernotebook uses modified JCL code I think this is releated to the issue I have just commented on: https://github.com/kamranzafar/JCL/issues/32 Lucene library works ok when the jar is added to the beakernotebook boot classpath. Is there any way we can use dynamic jars with SPI ?

ildipo commented 9 years ago

Hi, thanks for your report. I have to look at this in details.

I think the issue you highlighted with JCL it is related. We do use the modified JCL loader to allow for dynamic class re-loading and (of course) this has issues when you want to use other class loaders.

Right now I cannot think how to circumvent this - I'll try to look at this issue as soon as I can

ildipo commented 9 years ago

Do you have an example notebook that demonstrates this issue? I did fix a class loading issue for scala and I'm wondering if here we can use the same solution.

dkobylarz commented 9 years ago

Here's the sample code that runs in beakernotebook, just add lucene-4.7.0.jar to the classpath, available here: http://central.maven.org/maven2/org/apache/lucene/lucene-core/4.7.0/lucene-core-4.7.0.jar

Java snippet:

import java.util.Iterator;
import java.util.ServiceLoader;

Class<?> codecClass = Class.forName("org.apache.lucene.codecs.Codec");

ServiceLoader<?> loader = ServiceLoader.load(codecClass);

int c = 0;

for( Iterator<?> iterator = loader.iterator(); iterator.hasNext(); ) {

  System.out.println("Codec: " + iterator.next());

  c++;

}

System.out.println("Codecs count: " + c);

and the stack trace:

java.lang.ExceptionInInitializerError

at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:264)
at com.twosigma.beaker.javash.bkr09dbc233.Foo.beakerRun(Foo.java:11)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at com.twosigma.beaker.javash.utils.JavaEvaluator$workerThread$MyRunnable.run(JavaEvaluator.java:378)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.IllegalArgumentException: A SPI class of type org.apache.lucene.codecs.Codec with name 'Lucene46' does not exist. You need to add the corresponding JAR file supporting this SPI to your classpath.The current classpath supports the following names: []
at org.apache.lucene.util.NamedSPILoader.lookup(NamedSPILoader.java:109)
at org.apache.lucene.codecs.Codec.forName(Codec.java:95)
at org.apache.lucene.codecs.Codec.(Codec.java:122)
... 13 more
ildipo commented 9 years ago

This works now in Java but in groovy I get

Caused by: java.lang.ClassCastException: class org.apache.lucene.codecs.lucene40.Lucene40PostingsFormat

dkobylarz commented 9 years ago

Thanks for the update, I confirm SPI works in java now and there's this exception in groovy. I think it occurs when same class is loaded by two different classloaders and a class from one hierarchy is being cast to another class hierarchy - this is what usually happens. Here's a shorter snippet for groovy tests:

def codecClass = Class.forName("org.apache.lucene.codecs.Codec")

def loader = ServiceLoader.load(codecClass)

int c = 0

for( Iterator<?> iterator = loader.iterator(); iterator.hasNext(); ) {

  println("Codec: " + iterator.next())

  c++

}

println("Codecs count: $c")
hadfield commented 9 years ago

any ETA on fixing the groovy issue?

hadfield commented 9 years ago

according to this: https://github.com/kamranzafar/JCL/issues/32 the underlying JCL issue was resolved.

hadfield commented 8 years ago

JCL is not used anymore, but the problem still persists.

A fix is available here: https://github.com/vital-ai/beaker-notebook/commit/1c8dc931054f03869093c7f0104e1fbdeb6cc914

This replaces groovyshell with groovy compiling the code.

scottdraves commented 8 years ago

that's for the tip, we'll try it out. is that something you could contribute?

hadfield commented 8 years ago

we don't have time to put together a PR at the moment, but you are welcome to incorporate the code change.

scottdraves commented 8 years ago

cool thanks don't worry about the PR but if the code is nontrivial we would need a contributor license agreement.

hadfield commented 8 years ago

it's pretty small, but happy to provide contributor license as needed.


Marc C. Hadfield marc@vital.ai @MarcHadfield 917-991-9685

On Tue, Dec 8, 2015 at 3:49 PM, Scott Draves notifications@github.com wrote:

cool thanks don't worry about the PR but if the code is nontrivial we would need a contributor license agreement.

— Reply to this email directly or view it on GitHub https://github.com/twosigma/beaker-notebook/issues/2276#issuecomment-163013721 .

scottdraves commented 8 years ago

didn't seem to work: https://github.com/twosigma/beaker-notebook/pull/3073

hadfield commented 8 years ago

what is the issue?

scottdraves commented 8 years ago

the class loader exception persisted. i would be happy to take a closer look this week.

hadfield commented 8 years ago

ok, great. we're using a variant of the same code successfully internally. ( https://github.com/vital-ai/beaker-notebook/commit/1c8dc931054f03869093c7f0104e1fbdeb6cc914 )

dkobylarz commented 8 years ago

fyi, I have just created a new pull request with our latest GroovyEvaluator version: #3428

hadfield commented 8 years ago

thanks derek!

scott, let us know if that fixes your issue.

hadfield commented 8 years ago

any progress getting this PR merged in?

scottdraves commented 8 years ago

commented on the PR

scottdraves commented 8 years ago

Marc, with the merge of https://github.com/twosigma/beaker-notebook/pull/3428 this should be fixed, right? can you please confirm?

hadfield commented 8 years ago

hi @scottdraves , yes, derek confirmed that the merge fixes the issue with serviceloader.

this merged PR also allows environmental variables to be used as per the PR notes, but using them is dependent on the PR: https://github.com/twosigma/beaker-notebook/pull/3416 which allows setting such environmental variables.

scottdraves commented 8 years ago

xlnt, thanks again for the patch. will comment on the other PR.