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

cleanup NativeException #3773

Merged
merged 4 commits into from
Apr 27, 2016
Merged

cleanup NativeException #3773

merged 4 commits into from
Apr 27, 2016

Conversation

kares
Copy link
Member

@kares kares commented Apr 1, 2016

wasn't able to find a test/spec or a reason why RaiseException did (for NativeException) :

-    private static String buildMessage(Throwable exception) {
-        StringBuilder sb = new StringBuilder();
-        StringWriter stackTrace = new StringWriter();
-        exception.printStackTrace(new PrintWriter(stackTrace));
-
-        sb.append("Native Exception: '").append(exception.getClass()).append("'; ");
-        sb.append("Message: ").append(exception.getMessage()).append("; ");
-        sb.append("StackTrace: ").append(stackTrace.getBuffer());
-
-        return sb.toString();
-    }

9.1 sounds like a good place to break away from that message building - which would actually only be seen very rarely (effectively almost never without hacks) even without the patch as RaiseException does re-set the providedMessage field in case of the NativeException constructor path.

another annoyance with NativeException is the cause.stackTrace + backtrace join-ing - which shouldn't be necessary at all ... for now I've kept it for cases where the trace heads do not point to the same location. that means 99% cases it won't show up. except when the head would be filtered away as a JRuby internal. there seem to have been some special NativeException filtering in place previously but with filtering going on elsewhere I did not want to filter twice esp as it seemed unnecessary.

tests on Java as well as Ruby side to cover functionality.

p.s. RubyException got a getMessageAsJavaString (would love to have been able to just change getMessage to return String :) which is used by RaiseException - this way at least for NativeException the String -> RubyString -> String message conversion can be avoided 🎉

kares added 4 commits April 1, 2016 15:49
…e building

- add getMessageAsJavaString to potentially avoid String -> RubyString -> String
- deprecated direct RubyException.message (unfortunately public) field access
- protected RubyException allows a null message to be handled by sub-classes
@kares kares added this to the JRuby 9.1.0.0 milestone Apr 1, 2016
@headius
Copy link
Member

headius commented Apr 27, 2016

👍 This stuff is all old and should be removed whenever we see it. We have completely different notions of how exceptions work now, and NativeException is not a part of it.

@kares kares merged commit 8e0a058 into master Apr 27, 2016
@kares kares deleted the cleanup-native-exception branch April 27, 2016 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants