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

Parallel exception hierarchy for Java #5065

Merged
merged 12 commits into from Feb 28, 2018

Conversation

Projects
None yet
2 participants
@headius
Member

headius commented Feb 27, 2018

This is to address problems related to #4781.

No Ruby exception types have a real exception type in Java, which means in order to capture any of them you must catch all of them (catch (RaiseException)) and inspect or rethrow. This is inefficient and ugly.

This PR introduces a parallel Java exception hierarchy that maps 1:1 to the base Ruby exception hierarchy.

  • All RubyException objects now know how to construct their appropriate Throwable.
  • All base exception types will have their own java.lang.Throwable (subclass of RaiseException).

In addition, this makes it possible for JRuby ext authors to create the same parallel Throwable+RubyException within their libraries to improve exception handling from Java (think catch (ParserError) in the Psych extension).

headius added some commits Feb 27, 2018

Initial pass for parallel exception hierarchy. #4781
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.
Begin propagating exception hierarchy down stack.
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 headius added this to the JRuby 9.2.0.0 milestone Feb 27, 2018

@headius headius requested review from enebo and kares Feb 27, 2018

@headius

This comment has been minimized.

Member

headius commented Feb 27, 2018

More work coming, but mostly just filling out the hierarchy.

Implement Java exceptions for all primary Ruby exception types.
This includes everything in standard Ruby plus our additions but
minus the errno classes.

See #4781.
@headius

This comment has been minimized.

Member

headius commented Feb 28, 2018

I've implemented this for all types of exceptions standard to Ruby, plus our few additions, but I have not done all the errno exceptions. Should I?

I have a minor concern about cluttering up org.jruby with more classes, but I'm not sure where else the new Ruby*Exception types should live. Should we move them somewhere?

All the new Throwables live under org.jruby.exceptions.

This is just about ready, pending questions above and fixing any regressions.

headius added some commits Feb 28, 2018

@kares

This comment has been minimized.

Member

kares commented Feb 28, 2018

💯 very nice (and backwards compatible)!

minor thing: I know we use get-er style naming for conversions but still
public RaiseException getRaiseException() maybe its better (might be subjective) as :
public RaiseException toRaiseException() (or asRaiseException) so the 'conversion' intent is clear?

@headius

This comment has been minimized.

Member

headius commented Feb 28, 2018

minor thing: I know we use get-er style naming for conversions but still
public RaiseException getRaiseException() maybe its better (might be subjective) as :
public RaiseException toRaiseException() (or asRaiseException) so the 'conversion' intent is clear?

Yeah I have also had concerns about some of these names being rather long. Maybe throwable or toThrowable? It would still return RaiseException.

I believe this is green other than things also failing on master.

headius added some commits Feb 28, 2018

@headius headius merged commit 357c171 into master Feb 28, 2018

0 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@headius headius deleted the exceptions2 branch Feb 28, 2018

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