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

Improve two-way integration of Java and Ruby exceptions #4781

Closed
headius opened this issue Sep 6, 2017 · 6 comments
Closed

Improve two-way integration of Java and Ruby exceptions #4781

headius opened this issue Sep 6, 2017 · 6 comments

Comments

@headius
Copy link
Member

headius commented Sep 6, 2017

For #4699, we removed the automatic initCause call for Java exceptions raised from Ruby. There were a few issues with that logic:

  • Java does not automatically set cause for you, so there seemed to be a bit of a mismatch.
  • The cause can only be set once, so we must check if it has been set already. Attempting to set it again raises an illegal argument exception from Java.
  • The cause field is not volatile so you cannot check and set it safely across threads. It must be set under lock, and even then it is not guaranteed to be visible.
  • Cause was getting set on unrelated exceptions just because they got re-raised with $! in flight.

In light of these problems, I think we should not reinstate automatic cause-setting for Java exceptions, but we do need a way to provide a cause. There's a few ways this might be done...I try to describe some below.

RubyException could hold a reference to its "container" RaiseException, so it can be re-raised without making a new one.

When providing the Java cause, this exception could be used, either through a new JRuby-specific method:

ex.throwable
# or
ex.to_java(Throwable)

...or via some magic trickery during value conversion in JI, when we see a RubyException is being passed for a Throwable.

We have also discussed in the past building a parallel exception hierarchy for RaiseException. In this structure, all exception types that currently exist would still work, but for each Ruby exception there would be an equivalent Java exception of the same name. The two would be inseparable, like the proxy objects we use for JI. This would allow the Ruby exception to behave more naturally as a Java exception, and allow Java code to rescue Ruby exceptions by name (rather than the RaiseException dance we have today).

All this combined would improve our integration between Ruby and Java exceptions.

@headius headius changed the title Provide a way to initCause for a Java exception with a Ruby exception Improve two-way integration of Java and Ruby exceptions Sep 6, 2017
@headius headius added this to the JRuby 9.2.0.0 milestone Sep 6, 2017
headius added a commit that referenced this issue Sep 6, 2017
headius added a commit that referenced this issue Sep 6, 2017
headius added a commit that referenced this issue Sep 6, 2017
I have long wanted to set up a parallel exception class hierarchy
in Java to match the Ruby hierarchy, so you could rescue Ruby
exceptions by type. This is the beginning of that work.

See #4781
@headius
Copy link
Member Author

headius commented Sep 6, 2017

I've started work on the parallel exception hierarchy on the parallel_exceptions branch.

headius added a commit that referenced this issue Feb 27, 2018
This change allow callers to skip the cast when calling toJava
on a Ruby object. It was initially done to assist work on #4781
by allowing a simple "throw rubyException.toJava(Throwable.class).

I tested binary compatibility in two ways:

* A full recompile with all generification in place plus testing
  standard JRuby commands. This confirms that classes recompiled
  against the API but without other use of generics still function
  properly.
* A partial recompile with only the generified classes rebuilt.
  This tests that classes not compiled against the API still
  function properly.

Of course any external JRuby extensions (readline, openssl) will
have been tested by running those standard commands (e.g. irb,
gem install).
@headius
Copy link
Member Author

headius commented Feb 27, 2018

I have abandoned the old branch and have a new (working) branch in exceptions2.

The logic here works like this:

  • Each RubyException knows how to construct its own RaiseException type, and holds a reference to it once created lazily.
  • Throw a RubyException using the form throw ex.getRaiseException().

At the moment this does not introduce any new Throwable types to the system. I need to work on cleaning up initialization, especially wrt backtraces.

headius added a commit that referenced this issue Feb 27, 2018
This work is the start of producing a Java-domain Throwable
hierarchy that matches the Ruby hierarchy, so that Ruby exceptions
can be caught using their actual Throwable type rather than using
RaiseException and checking the exceptions type manually.

A few changes of note:

* RubyException has most logic moved up to new
  AbstractRubyException.
* All RubyException now hold a reference to their RaiseException,
  created lazily once as needed.
  * Throw a Ruby exception using the form
    `throw ex.getRaiseException()`.
  * Provide construction logic specific to a given exception by
    overriding createRaiseException.
* The nativeException parameter is eliminated from all exception
  construction and initialization paths. It was unused.
headius added a commit that referenced this issue Feb 27, 2018
This forms the meat of wiring up the new parallel exception
hierarchy. All public constructors of RaiseException are now
deprecated and unused. RaiseException.from() forms take their
place and know how to properly construct a Ruby exception and
wrap it with an appropriate RaiseException subclass.

Three native exceptions are added, mapping to the Ruby exception
type of the same name: Exception, StandardError, and
SignalException.

One location in the code that caught RaiseException is now able
to catch StandardError (RubyNumeric coerce logic). There are
likely others.

See #4781.
headius added a commit that referenced this issue Feb 28, 2018
This includes everything in standard Ruby plus our additions but
minus the errno classes.

See #4781.
headius added a commit that referenced this issue Feb 28, 2018
@headius
Copy link
Member Author

headius commented Feb 28, 2018

I have landed the parallel exception hierarchy. Will look into making it solve this issue.

@headius
Copy link
Member Author

headius commented Feb 28, 2018

Ok, without any additional modification, we can now do the following to attach a Ruby exception as the cause for a Java exception:

$ jruby -e 'begin; raise "hello"; rescue; raise java.lang.RuntimeException.new($!.to_java); end'
Unhandled Java exception: java.lang.RuntimeException: org.jruby.exceptions.RuntimeError: (RuntimeError) hello
java.lang.RuntimeException: org.jruby.exceptions.RuntimeError: (RuntimeError) hello
         newInstance0 at sun/reflect/NativeConstructorAccessorImpl.java:-2
          newInstance at sun/reflect/NativeConstructorAccessorImpl.java:62
          newInstance at sun/reflect/DelegatingConstructorAccessorImpl.java:45
          newInstance at java/lang/reflect/Constructor.java:423
    newInstanceDirect at org/jruby/javasupport/JavaConstructor.java:278
                 call at org/jruby/java/invokers/ConstructorInvoker.java:87
                 call at org/jruby/java/invokers/ConstructorInvoker.java:176
         cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:355
                 call at org/jruby/runtime/callsite/CachingCallSite.java:180
                 call at org/jruby/java/proxies/ConcreteJavaProxy.java:56
         cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:355
                 call at org/jruby/runtime/callsite/CachingCallSite.java:180
          newInstance at org/jruby/RubyClass.java:1002
                 call at org/jruby/RubyClass$INVOKER$i$newInstance.gen:-1
                 call at org/jruby/internal/runtime/methods/DynamicMethod.java:202
                 call at org/jruby/java/proxies/ConcreteJavaProxy.java:158
         cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:344
                 call at org/jruby/runtime/callsite/CachingCallSite.java:170
     invokeOther6:new at -e:1
               <main> at -e:1
  invokeWithArguments at java/lang/invoke/MethodHandle.java:627
                 load at org/jruby/ir/Compiler.java:94
            runScript at org/jruby/Ruby.java:846
          runNormally at org/jruby/Ruby.java:765
          runNormally at org/jruby/Ruby.java:783
          runFromMain at org/jruby/Ruby.java:596
        doRunFromMain at org/jruby/Main.java:417
          internalRun at org/jruby/Main.java:305
                  run at org/jruby/Main.java:232
                 main at org/jruby/Main.java:204

Caused by:
org.jruby.exceptions.RuntimeError: (RuntimeError) hello
  <main> at -e:1

Remaining question for @kares @enebo: should it work automatically without the to_java call?

We'd need to set up all Exception classes to report Throwable as their "native" Java class, but I think that's all. They'd be eligible for passing to methods that take Throwable, and the toJava logic I added will retrieve the same throwable every time.

@kares
Copy link
Member

kares commented Mar 1, 2018

Remaining question for @kares @enebo: should it work automatically without the to_java call?

tempting but I am not sure if we're able to think through all the consequences in user-land.
... maybe a compromise would be to have a flag (on by default) to "degrade" back? (not that I want it)

@headius
Copy link
Member Author

headius commented Oct 11, 2018

I believe all uncontended changes for this landed in #5065, which now provides real Java exceptions and two-way integration for most Ruby exceptions.

@headius headius closed this as completed Oct 11, 2018
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

3 participants