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

Ruby exceptions passing through Java Integration and back remain wrapped #1839

Closed
headius opened this issue Jul 21, 2014 · 8 comments
Closed

Comments

@headius
Copy link
Member

headius commented Jul 21, 2014

This is either a problem with JI (my area) or IR (@enebo or @subbuss).

It appears that exceptions raised by Ruby code called by Java code called through Java integration (a Ruby exception that passes through a JI call on its way out) remain wrapped and are presented to rescue blocks as the wrapped RaiseException rather than the contained Ruby exception.

 $ jruby -J-classpath test/target/test-classes/ -S rspec spec/java_integration/exceptions/rescue_spec.rb 
.................FF

Failures:

  1) A ruby exception raised through java and back to ruby its class and message is preserved
     Failure/Error: e.message.should == "it comes from ruby"
       expected: "it comes from ruby"
            got: "org.jruby.exceptions.RaiseException: (RuntimeError) it comes from ruby" (using ==)
     # ./spec/java_integration/exceptions/rescue_spec.rb:208:in `(root)'

  2) A ruby exception raised through java and back to ruby via a different thread its class and message is preserved
     Failure/Error: e.message.should == "it comes from ruby"
       expected: "it comes from ruby"
            got: "org.jruby.exceptions.RaiseException: (RuntimeError) it comes from ruby" (using ==)
     # ./spec/java_integration/exceptions/rescue_spec.rb:222:in `(root)'

This happens regardless of JIT status, and since I have not done anything with JI and exceptions recently I suspect this is a flaw in how IR handles unwrapping exceptions for rescue.

@headius headius added this to the JRuby 9000 milestone Jul 21, 2014
@subbuss
Copy link
Contributor

subbuss commented Jul 21, 2014

I think it is because the interpreter (in this debug output below) receives a NativeException and it only unwraps RaiseExceptions. This is debug output from a short test snippet to reproduce that error. We should chat about whether NativeExceptions receive special handling or if this is something that requires changing in JI ...

...
2014-07-20T22:59:46.576-05:00: Interpreter: I: %v_1 = search_const(ExceptionRunner, scope<x.rb>, no-private-consts=false)
2014-07-20T22:59:46.576-05:00: Interpreter: I: %v_2 = call_0o(NORMAL, 'new', %v_1, []){0O}
2014-07-20T22:59:46.576-05:00: Interpreter: I: %v_1 = call(NORMAL, 'do_it_now', %v_2, [], &%self:CLOSURE x.rb_CLOSURE_1[x.rb:5])
2014-07-20T22:59:46.585-05:00: Interpreter: I: thread_poll
2014-07-20T22:59:46.585-05:00: Interpreter: I: line_num(6)
2014-07-20T22:59:46.585-05:00: Interpreter: I: %cl_1_0 = call_1o(FUNCTIONAL, 'raise', %self, ["it comes from ruby"]){1O}
Exception `RuntimeError' at java/lang/Thread.java:1567 - it comes from ruby
2014-07-20T22:59:46.589-05:00: Interpreter: in scope: CLOSURE x.rb_CLOSURE_1[x.rb:5], caught Java throwable: org.jruby.exceptions.RaiseException: (RuntimeError) it comes from ruby; excepting instr: %cl_1_0 = call_1o(FUNCTIONAL, 'raise', %self, ["it comes from ruby"]){1O}
2014-07-20T22:59:46.589-05:00: Interpreter: ipc for rescuer: -1
2014-07-20T22:59:46.591-05:00: Interpreter: in scope: Script: file: x.rbSCRIPT_BODY x.rb[x.rb:0], caught Java throwable: org.jruby.exceptions.RaiseException: (RuntimeError) it comes from ruby; excepting instr: %v_1 = call(NORMAL, 'do_it_now', %v_2, [], &%self:CLOSURE x.rb_CLOSURE_1[x.rb:5])
2014-07-20T22:59:46.591-05:00: Interpreter: ipc for rescuer: 24
2014-07-20T22:59:46.591-05:00: Interpreter: I: %v_1 = recv_ruby_exc
2014-07-20T22:59:46.591-05:00: Interpreter: I: %v_3 = search_const(RuntimeError, scope<x.rb>, no-private-consts=false)
2014-07-20T22:59:46.591-05:00: Interpreter: I: %v_4 = rescue_eqq(Array:[%v_3], %v_1)
2014-07-20T22:59:46.591-05:00: Interpreter: I: b_true(%v_4, LBL_4:29)
2014-07-20T22:59:46.591-05:00: Interpreter: I: %t_e_5 = get_global_var($!)
2014-07-20T22:59:46.591-05:00: Interpreter: I: line_num(9)
2014-07-20T22:59:46.591-05:00: Interpreter: I: store_lvar(%t_e_5, x.rb, e(0:0))
2014-07-20T22:59:46.591-05:00: Interpreter: I: %v_1 = call_1o(FUNCTIONAL, 'p', %self, [%t_e_5]){1O}
#<NativeException: org.jruby.exceptions.RaiseException: (RuntimeError) it comes from ruby>
....

@headius
Copy link
Member Author

headius commented Jul 22, 2014

NativeException should almost never happen. We got rid of those in 1.6 I think. We need to figure out who's creating this NativeException and fix it on that end, I suppose.

@subbuss
Copy link
Contributor

subbuss commented Jul 22, 2014

Oh, it is gone? Then IRRuntimeHelpers has stale code :) ... See https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java#L297 ... and on line 304, it sets $! to that.

@headius
Copy link
Member Author

headius commented Jul 22, 2014

Ok, sorry, it's not completely gone. For backward compatibility, if someone rescues NativeException and we receive a Java exception, then we wrap it before we give it to them. But otherwise we should be propagating the exception as-is and not wrapping it.

@subbuss
Copy link
Contributor

subbuss commented Jul 23, 2014

There seems to be couple things going on.

(1) JI code somewhere is setting $! to a NativeException obj, but the exception obj that enters the interpreter is actually a RaiseException. Since the normal ruby case doesn't set $!, the NativeException obj gets passed along. Whatever passes the correct RaiseException obj should also be setting $! to the same object rather than set a NativeException obj. For the test snippet in question, this is the source of the bug.

(2) In addition, in IRRuntimeHelpers code above, in some scenarios, looks like $! is being updated to a different object type for the JI case. I don't fully understand why this is required and also what the correct wrapping/unwrapping logic is for that. Looking at Helpers.checkJavaException, I see a lot of logic there and it is unwrapped/wrapped differently depending on exception type in the rescue clause .. so, I would think something similar would happen in order to set "$!"

This is probably best fixed by one of you since you understand JI and I haven't delved into that part of JRuby yet, but happy to chat on irc/im about any of this.

@subbuss
Copy link
Contributor

subbuss commented Jul 24, 2014

An update on what is going on based on some debugging.

https://gist.githubusercontent.com/subbuss/d29d501f0cadd8b92a39/raw/993c2a9589bec27b57a192542578bf6b7986e92f/gistfile1.txt shows you what is going on.

When the raise in the ruby code executes, $! is correctly set to the RaiseException object. But, when the exception propagates into the JI code, JI catches the exception and does a stack-trace rewrite before rethrowing it. It uses Helpers.rewriteStackTraceAndThrow for this. Here is the code for that method:

public static IRubyObject rewriteStackTraceAndThrow(Throwable t, Ruby runtime) {
    RaiseException rewriteStackTrace = RaiseException.createNativeRaiseException(runtime, t);
    t.setStackTrace(rewriteStackTrace.getStackTrace());
    throwException(t);

    // not reached
    return null;
}

So, this creates a nativeException -- but RaiseException code unconditionally sets $! - which means $! is now set to be the NativeException. However, the helper code is creating this RaiseException simply to generate a stack trace and discards it before rethrowing the original RubyException. But, in the bargain, it has clobbered $!. That, I think is the bug.

@subbuss
Copy link
Contributor

subbuss commented Nov 6, 2014

Isn't this fixed now?

@headius
Copy link
Member Author

headius commented Jan 15, 2015

This has to be fixed. We'd see a lot more issues with our ruby kernel or the JI specs if it wasn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants