Skip to content

Show that ArgumentError from encoding error is recursive#4117

Closed
nightscape wants to merge 1 commit intojruby:masterfrom
crealytics:recursive_argument_error
Closed

Show that ArgumentError from encoding error is recursive#4117
nightscape wants to merge 1 commit intojruby:masterfrom
crealytics:recursive_argument_error

Conversation

@nightscape
Copy link
Contributor

In general Java exceptions should not have themselves as cause and JRuby in most cases handles this correctly.
We just ran into a problem where an ArgumentError raised due to an encoding problem does have itself as cause which leads to a problem in an ActiveSupport method.
The ActiveSupport method is a little dangerous/questionable by itself, but the recursive cause should be prevented in JRuby nonetheless and there is also a test that checks this for RuntimeErrors.
We're not sure if this is the most general reproduction, so feel free to ignore this PR if you find a more general test.

@nightscape
Copy link
Contributor Author

Issue #4024 might be related to this.

@headius
Copy link
Member

headius commented Sep 30, 2016

I think this is simply us being too eager to use the in-flight exception when re-raising, not realizing we're stuffing the same exception into itself again. Looking.

@headius
Copy link
Member

headius commented Sep 30, 2016

This only appears to affect re-raising using the raise ex form, and I have a fix coming.

$ jruby -e 'begin; "hi \255".split; rescue => e; p e.cause; raise e; end rescue p $!.cause'
nil
#<ArgumentError: invalid byte sequence in UTF-8>

@headius
Copy link
Member

headius commented Sep 30, 2016

I've fixed the issue, but the test would be better going into ruby/spec (spec/ruby/core/exception or spec/ruby/core/kernel/raise_spec.rb).

@headius headius closed this Sep 30, 2016
@headius headius added this to the JRuby 9.1.6.0 milestone Sep 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants