Skip to content
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

$CLASSPATH changes regressed spec:ji #2276

Closed
headius opened this issue Dec 5, 2014 · 4 comments
Closed

$CLASSPATH changes regressed spec:ji #2276

headius opened this issue Dec 5, 2014 · 4 comments

Comments

@headius
Copy link
Member

@headius headius commented Dec 5, 2014

I don't have an explanation for it, but the commits 76a514e and/or 83044c2 caused spec:ji to regressed in spectacular fashion. The errors indicated some terrific classloader problem leading to multiple versions of JRuby core classes to collide. This only appears to happen on master; jruby-1_7 passes spec:ji.

This is definitely a big concern with the recent classloader changes. I have reverted these two commits on master and we need to investigate why they caused such problems.

cc @mkristian

@headius headius added the core label Dec 5, 2014
@headius headius added this to the JRuby 9.0.0.0 milestone Dec 5, 2014
@mkristian
Copy link
Member

@mkristian mkristian commented Dec 5, 2014

smile, the reason is that the command line execution sets the
runtime.getJRubyClassLoader as Thread.currentThread.contextClassLoader (CCL)

first this CCL has jruby.jar in its parent, then you add jruby.jar to CCL
(which is runtime.getJRubyClassLoader) and now you load all new classes
from the CCL.

this does not happen when you run the same code inside a java app with
ScriptingContainer since there CCL is left alone, some is true for case
where jruby does not have anything to do with CCL like OSGi containers.

the reason why it is not showing up, is that relative path are resolved
within the classloader in the java way using System property 'user.dir' as
current directory. i.e. the $CLASSPATH entry with jruby.jar has NO effect !
with my patch it is working now ;)

I consider it as edge case that people add jruby.jar to $CLASSPATH. just
use any other jar without jruby classes and probably test whether you can
really load them !

@headius
Copy link
Member Author

@headius headius commented Dec 5, 2014

Ahh, so the problem was that one spec pushed jruby.jar into $CLASSPATH and as a result started seeing double loads of JRuby classes in other specs. In particular, any JI use of JRuby classes would cause another instance to get loaded.

I'm glad you found the spec triggering this, but I must confess it still worries me a bit. You're right -- adding jruby.jar to the current runtime's $CLASSPATH does seem like an edge case, but I wonder about Ruby libraries that use jruby-jars to pin logic to a specific version of JRuby. Of course, they'd have trouble without the classloader changes since some class would come from jruby-jars and some would come from the parent JRuby...so maybe I worry too much.

In any case, we have not seen this occur on other libraries people test against 9k, so perhaps it's really ok.

Thank you for looking into it!

@mkristian
Copy link
Member

@mkristian mkristian commented Dec 5, 2014

with jruby-1_7 requiring those jars from jruby-jars will have no effect,
they will just not be used. so I really doubt that is an use-case to hot
replace jruby by another version of itself.

and the readme from jruby-jars suggest to fork:

require 'jruby-jars'
  exec("java", "-cp", JRubyJars.core_jar_path, "org.jruby.Main")

which is totally OK.

PS I understand the general worries due to bad experience with classloaders
and some of my "features"

@mkristian
Copy link
Member

@mkristian mkristian commented Dec 6, 2014

@headius I was thinking a bit more about this issue and with this patch

diff --git a/core/src/main/java/org/jruby/util/OneShotClassLoader.java b/core/src/main/java/org/jruby/util/OneShotClassLoader.java
index e2209d6..9813295 100644
--- a/core/src/main/java/org/jruby/util/OneShotClassLoader.java
+++ b/core/src/main/java/org/jruby/util/OneShotClassLoader.java
@@ -6,7 +6,7 @@ package org.jruby.util;
 public class OneShotClassLoader extends ClassLoader implements ClassDefiningClassLoader {

     public OneShotClassLoader(JRubyClassLoader parent) {
-        super(parent);
+        super(parent.getParent());
     }

     public Class<?> defineClass(String name, byte[] bytes) {

at least the errors look more moderate in the case someone does $CLASSPATH << 'jruby.jar'. it still fails for case like:
https://github.com/jruby/jruby/blob/master/core/src/main/ruby/jruby/jruby.rb#L77

where a ruby script uses a class which is now part of JRubyClassLoader and which can NOT be coerced to the class from JRubyClassLoader.getParent classloader.

I feel the patch is safe assuming that AOT and JIT compilation takes place in the same context, i.e. in the main classloader of jruby (and not JRubyClassLoader which is empty for AOT). for me it is a valid patch since it keeps a classloader which is not used out of the game and makes classloader issue less likely.

@headius headius removed this from the JRuby 9.0.0.0 milestone Jan 16, 2015
@headius headius added this to the JRuby 9.0.0.0-pre1 milestone Jan 16, 2015
@headius headius added this to the JRuby 9.0.0.0-pre1 milestone Jan 16, 2015
@headius headius removed this from the JRuby 9.0.0.0 milestone Jan 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants