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鈥檒l 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

Conversation

Projects
None yet
2 participants
@kares
Member

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 some commits Apr 1, 2016

simplify NativeException exc.message handling - avoid provided messag鈥
鈥 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

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 27, 2016

Member

馃憤 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.

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

0 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details

@kares kares deleted the cleanup-native-exception branch Apr 27, 2016

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