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.

Sign up for GitHub

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

Jump to bottom

Stop wrapping Java exceptions with LoadError #5135

Closed
headius opened this issue Apr 12, 2018 · 8 comments
Closed

Stop wrapping Java exceptions with LoadError #5135

headius opened this issue Apr 12, 2018 · 8 comments
Milestone
Won't Fix

Comments

@headius
Copy link
Member

headius commented Apr 12, 2018

In looking at #5115, I realized we are still wrapping Java exceptions that bubble through a load or require with a new LoadError. The only way to see the original exception is to pass -d to JRuby and capture stderr so you can see the actual exception's stack trace.

$ cat blah.rb
raise java.lang.NullPointerException.new

$ jruby -e 'require "./blah.rb"'
LoadError: load error: ./blah -- java.lang.NullPointerException: null
  require at org/jruby/RubyKernel.java:956
  require at /Users/headius/projects/jruby/lib/ruby/stdlib/rubygems/core_ext/kernel_require.rb:55
   <main> at -e:1

We need to do a better job reporting these exceptions, since they usually will indicate a load-time bug in some called library.

One option would be to set the cause of the LoadError to the Java exception, and make sure that exception dumping is outputting cause correctly. This should naturally then output the LoadError trace followed by the original Java exception trace.

After thinking about it for a while, I do not believe we can stop wrapping them since the typical pattern in Ruby is that rescue LoadError covers a multitude of sins (library loading, library IO, other system exceptions during load). Any current libraries that depend on this behavior will continue to work as before. However, the logged output of those LoadError will now make it more obvious there's a problem to be fixed in the library itself.
@headius headius added this to the JRuby 9.2.0.0 milestone Apr 12, 2018
@kares
Copy link
Member

kares commented Apr 12, 2018

+1 have been using the native exception Java cause wrapping in RaiseException in a few places already.
mostly out of JRuby itself as well - AR-JDBC, JOSSL. maybe designing some explicit API could be handy ...

@enebo
Copy link
Member

enebo commented Apr 12, 2018

Yeah adding a cause to LoadError seems like a great way of preserving semantics of Ruby being able to deal with a failed load (a Java exception is just another novel one for us but not MRI), but still see the underlying problem. I would go so far to say an unhandled LoadError should display cause also.

@headius
Copy link
Member Author

headius commented Apr 13, 2018

Well I can certainly add the cause, but it does not appear to be default behavior in CRuby to print the cause in the top-level exception printout. It's a start, but so far I am not seeing much default logging behavior associated with Exception#cause.

@kares
Copy link
Member

kares commented Apr 13, 2018

how about just using the Java initCause and having a flag to "long" print cause traces or simply do it with
-Xexception.style=raw ... just an idea. most users probably do not want to see Java traces by default ...

@kares
Copy link
Member

kares commented Apr 13, 2018

(since initCause is used in external exts - it would allow traces to "switch" using the feature on as well)

@enebo
Copy link
Member

enebo commented Apr 13, 2018

@headius @kares So using cause may interfere with MRI future plans but I bet we could submit it since it sort of sucks to not know why something did not load. As far as not wanting to see this...It is quite rare an ordinary user would see this in the case of a Java Exception being the cause. Perhaps we can just display in that case. I suspect 99% of the time it is an ext developer or us. Most requires work or are a simple syntax error (which would be a Ruby exception not a Java one).

@headius
Copy link
Member Author

headius commented Apr 13, 2018

Both CRuby and JRuby actually the actual exception for syntax errors and any regular Ruby errors that occur during load time. I'm not sure how to cause internal load errors in CRuby, so I can only test what happens when it can't find or load a file, which obviously leads to load errors.

I should be clear...I'd like to just get rid of the wrapper altogether. If the Ruby code loads, everything that happens after that should just raise as a normal error. But I'm worried about libraries that use LoadError as a catch all around JRuby extensions or Java library calls, as an indication to fall back to pure-Ruby or a different version of the library.

@enebo enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018
@headius
Copy link
Member Author

headius commented Oct 11, 2018

We will leave this unfixed for now. LoadError only wraps Java exceptions, allowing others to bubble out properly. This aligns (in my opinion) with MRI testing runtime-level failures in loading a library as more LoadError.

@headius headius closed this as completed Oct 11, 2018
@headius headius modified the milestones: JRuby 9.2.1.0, Won't Fix Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Won't Fix
Development

No branches or pull requests
3 participants
@headius @enebo @kares