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

Cannot create nested exception #1542

Closed
mojavelinux opened this issue Jan 22, 2019 · 11 comments
Closed

Cannot create nested exception #1542

mojavelinux opened this issue Jan 22, 2019 · 11 comments
Assignees
Milestone

Comments

@mojavelinux
Copy link

It should be possible to create a nested exception using the exception and set_backtrace methods on an exception.

For example:

begin
  begin
    raise 'nested'
  rescue => nested
    root_ex = nested.exception 'root'
    root_ex.set_backtrace nested.backtrace
    raise root_ex
  end
rescue => e
  puts e.message
  p e.cause
end

This should print:

root
#<RuntimeError: nested>

However, what we see instead is:

nested
nil

It appears that truffleruby does not support creating nested exceptions.

@chrisseaton
Copy link
Collaborator

We do support nested exceptions in general, and the single spec for cause passes.

begin
  begin
    raise 'a'
  rescue => a
    raise 'b'
  end
rescue => b
  p b
  p b.cause
end

If we modify your code, this works as expected.

begin
  begin
    raise 'nested'
  rescue => nested
    #root_ex = nested.exception 'root'
    #root_ex.set_backtrace nested.backtrace
    raise 'foo'
  end
rescue => e
  puts e.message
  p e.cause
end

I think the error is more in Exception#exception, although there's quite a few specs for that and they all pass. I think these two tests show two separate issues.

begin
  begin
    raise 'a'
  rescue => a
    raise a.exception('b')
  end
rescue => b
  p b.message # a should be b
end
begin
  begin
    raise 'a'
  rescue => a
    raise a.exception('b')
  end
rescue => b
  p b.cause # nil, should be a
end

@chrisseaton
Copy link
Collaborator

chrisseaton commented Jan 22, 2019

Wow the problem's more weird than that!

begin
  begin
    raise 'a'
  rescue => a
    p a.object_id
    b = a.exception('b')
    p b.object_id
    raise b
  end
rescue => c
  p c.object_id
end
$ mri test.rb 
70236888579520
70236888579160
70236888579160
$ truffleruby test.rb
528
544
528

So the old exception is either being raised or caught instead of the new one!

@chrisseaton
Copy link
Collaborator

This is the spec I'm going with.

  it "when raised will be rescued as the new exception" do
    begin
      begin
        raised_first = StandardError.new('first')
        raise raised_first
      rescue => caught_first
        # raised_second = StandardError.new('second')   this would work fine
        raised_second = raised_first.exception('second')
        raise raised_second
      end
    rescue => caught_second
    end

    raised_first.should == caught_first
    raised_second.should == caught_second
  end

But this is a real puzzler how this can be going wrong!

@chrisseaton
Copy link
Collaborator

This is the problem

if (reRaiseProfile.profile(backtrace != null && backtrace.getTruffleException() != null)) {
// We need to rethrow the existing TruffleException, otherwise we would lose the
// TruffleStackTrace stored in it.
throw (RaiseException) backtrace.getTruffleException();

The new exception is cloned, and gets the same backtrace. The backtrace stores the original exception. For complicated reasons we re-use the same exception when raising an exception that's already been raised once and we do that by getting it from the backtrace.

CC @eregon.

@chrisseaton
Copy link
Collaborator

If I clone the backtrace and remove the reference to the TruffleException when cloning an exception, then Exception#dup does copy the backtrace fails because, as the comment above says, we lose part of the stack trace.

        @Specialization(guards = { "self != from", "isRubyException(from)" })
        public Object initializeCopy(DynamicObject self, DynamicObject from) {
            final Backtrace backtrace = Layouts.EXCEPTION.getBacktrace(from);
            Layouts.EXCEPTION.setBacktrace(self, backtrace == null ? null : backtrace.copyWithoutTruffleException());
            Layouts.EXCEPTION.setFormatter(self, Layouts.EXCEPTION.getFormatter(from));
            Layouts.EXCEPTION.setMessage(self, Layouts.EXCEPTION.getMessage(from));
            return self;
        }

@chrisseaton
Copy link
Collaborator

Potentially fixed as

                // We need to rethrow the existing TruffleException, otherwise we would lose the
                // TruffleStackTrace stored in it.
                final RaiseException raiseException = (RaiseException) backtrace.getTruffleException();
                raiseException.repurpose(exception, internal);
                throw raiseException;

@mojavelinux
Copy link
Author

Fascinating.

I'd like to offer some motivation as to why I think this is such an important change (since it's proving to be quite a puzzler to solve).

If I catch an exception from another program (or a facade), I want to make sure I can still offer all the details of the original exception while still being able to decorate it with a message to provides context. If I were to create a new exception, I might end up dropping some of the data from the original. So what I'm after is a pure wrapped exception. As I understand it, that's the point of the exception + set_backtrace methods (though I wish the exception method took a second argument to preserve the backtrace).

Here's where I use this in context: https://github.com/asciidoctor/asciidoctor/blob/41da20a47a8da96966ef3ec1c2f509e07e7920e3/lib/asciidoctor.rb#L1322-L1338

@eregon eregon self-assigned this Jan 23, 2019
@eregon
Copy link
Member

eregon commented Jan 23, 2019

@mojavelinux I'm working on it, the fix should land soon.

About

        # The original message must be explicitly preserved when wrapping a Ruby exception
        wrapped_ex = ex.exception %(#{context} - #{ex.message})
        # JRuby automatically sets backtrace, but not MRI
        wrapped_ex.set_backtrace ex.backtrace

That's probably a bug of Exception#exception worth reporting to MRI, if it wasn't already.
Exception#clone does copy the backtrace for all supported versions of MRI.

About the use case, would just raising a new exception in the rescue already achieve that?
Then the old exception would be the Exception#cause of the new one, and still contain a backtrace.

def internals
  raise ArgumentError, "foo"
end

def main
  begin
    internals
  rescue
    raise "wrapped"
  end
end

main

The problem is: MRI didn't show the cause at all up until 2.6.0 (I filed the bug for it 3 years ago, https://bugs.ruby-lang.org/issues/9918).
So 2.5.3:

Traceback (most recent call last):
	2: from e2.rb:13:in `<main>'
	1: from e2.rb:5:in `main'
e2.rb:9:in `rescue in main': wrapped (RuntimeError)

And 2.6.0:

Traceback (most recent call last):
	2: from e2.rb:13:in `<main>'
	1: from e2.rb:7:in `main'
e2.rb:2:in `internals': foo (ArgumentError)
	2: from e2.rb:13:in `<main>'
	1: from e2.rb:5:in `main'
e2.rb:9:in `rescue in main': wrapped (RuntimeError)

Of course, if there is a rescue on top properly printing the cause, then this doesn't matter, but as a library it's difficult to ensure that.

@eregon
Copy link
Member

eregon commented Jan 23, 2019

I filed https://bugs.ruby-lang.org/issues/15558 to ask what's the correct behavior for Exception#exception about copying the backtrace. Currently TruffleRuby copies it (might change), like JRuby, but unlike MRI.

@eregon eregon added this to the 1.0.0-rc12 milestone Jan 23, 2019
@eregon
Copy link
Member

eregon commented Jan 23, 2019

bb637c9 should fix this issue, and it passes the reported example.
@mojavelinux Thank you for the report!
The fix will be in the next release.

@eregon eregon closed this as completed Jan 23, 2019
@mojavelinux
Copy link
Author

That's great news, @eregon. Nice work!

would just raising a new exception in the rescue already achieve that?

Now that you point that out, I need to revisit that code. It may have been that way to workaround a deficiency in a version of Ruby that Asciidoctor no longer supports. Simply raising another exception might be sufficient.

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

No branches or pull requests

3 participants