This seems like an appropriate time to make this change. My immediate concern was about not properly setting $! every time we raise a Ruby exception from Java, but such cases should be using Kernel#raise logic in general.

I'll do a review with more specific notes. This could potentially go into 10.1.1.0 if you are happy with it, @kares.

Rails verifications failed due to a raise event hook suddenly getting nil values. This resulted in the server not starting, and the verification script is not smart enough to fail so it ran forever. I've killed it but that definitely seems like it's related to this PR.

Perhaps we should let this one bake and merge in 10.1.2.0.

Here's a patch that might be appropriate. It would be nice to try to match up the exact sequence of events and annotate the calls we make with the equivalent functions in CRuby, because our setup_exception is currently preRaise and it gets called regardless of whether raise is called. CRuby appears to defer exception setup longer, until it's actually about to be raised.

diff --git a/core/src/main/java/org/jruby/RubyKernel.java b/core/src/main/java/org/jruby/RubyKernel.java
index 33c1bcd6b9..0267466cd0 100644
--- a/core/src/main/java/org/jruby/RubyKernel.java
+++ b/core/src/main/java/org/jruby/RubyKernel.java
@@ -1110,7 +1110,12 @@ public class RubyKernel {
     private static IRubyObject raiseException(ThreadContext context, RubyException exception) {
         printDebugException(context, exception);
 
-        context.setErrorInfo(exception); // set $! as part of the raise flow (like MRI's setup_exception)
+        // set $! as part of the raise flow (like MRI's setup_exception)
+        context.setErrorInfo(exception);
+
+        // trace the raise immediately before throwing
+        IRRuntimeHelpers.traceRaise(context);
+
         return Helpers.throwExceptionT(exception.toThrowable());
     }
 
diff --git a/core/src/main/java/org/jruby/exceptions/RaiseException.java b/core/src/main/java/org/jruby/exceptions/RaiseException.java
index 095ebb9107..ce31f76291 100644
--- a/core/src/main/java/org/jruby/exceptions/RaiseException.java
+++ b/core/src/main/java/org/jruby/exceptions/RaiseException.java
@@ -223,8 +223,6 @@ public class RaiseException extends JumpException {
             }
             setStackTraceFromException();
         }
-
-        IRRuntimeHelpers.traceRaise(context);
     }
 
     private void setStackTraceFromException() {

I also discovered an old bug in our trace logic: we are not clearing $! during the hook body as CRuby does. The following spec tests this, and fails on all versions of JRuby I tested.

diff --git a/spec/ruby/core/tracepoint/trace_spec.rb b/spec/ruby/core/tracepoint/trace_spec.rb
index 167f594bb9..c1bf364002 100644
--- a/spec/ruby/core/tracepoint/trace_spec.rb
+++ b/spec/ruby/core/tracepoint/trace_spec.rb
@@ -7,4 +7,18 @@ describe 'TracePoint.trace' do
     trace.should.enabled?
     trace.disable
   end
+
+  it 'clears $! when invoking the trace block' do
+    exception_in_trace = nil
+    trace = TracePoint.trace(:raise) do |tp|
+      exception_in_trace = $!
+    end
+    begin
+      raise
+    rescue => e
+      exception_in_trace.should == nil
+      $!.should == e
+    end
+    trace.disable
+  end
 end

This does not appear to be tested in any Ruby spec. I did not investigate CRuby tests.

@eregon Unsure where to put the spec above. It is testing behavior of the hook block passed to TracePoint.new or TracePoint.trace but it is not really a behavior of either of those methods. I went with TracePoint.trace specs for simplicity, but the behavior applies to all forms of tracing.

That's good. Especially TracePoint tends to cause some overhead so it's good to keep specs using TracePoint under core/tracepoint.

applies to all forms of tracing.

BTW what other forms are there? set_trace_func? Anything else?

BTW what other forms are there? set_trace_func? Anything else?

@eregon Those are the two Ruby-facing APIs. I believe both should probably have this specification. Even though they both are implemented with the same event hook logic in CRuby, that might not be the case on all implementations (for example, JRuby might in the future use Java debug APIs to hook up some of these events, like raising).

The C-facing APIs probably should also have specs, but I am not equipped to write or test them.