Crystal: Re-raise an exception while keeping a previous callstack

[straight-shoota]

I was looking for a solution for re-raising an exception while keeping the same callstack of the original exception.

The use case is to add to the exception some additional info wich is not available where the exception is created. This is how I tried to implement this:

begin
  do_some_stuff_might_raise
rescue ex : OrigException
  ex.context = some_context_data
  raise ex
end

Currently this is not usable because raise always overwrites ex.callstack = Callstack.new, so the backtrace points to where the exception is enhanced with context data, but the original source is lost.

My workaround is a wrapper exception with the original exception as cause, which delegates everything to the original exception or could be unwrapped where the error handling happens.
But it doesn't feel right to it this way and such a WrapperException will not be rescued by a rescue block for OrigException. Generics might help, but it's still not the real deal...

We discussed this in Gitter and @akzhan initially suggested to set the callstack of an exception to the callstack of the cause if one is provided:

if ex.cause.nil?
  ex.callstack = CallStack.new
else
  ex.callstack = cause.callstack
end

But you might want to keep the callstacks of both the cause and the new exception. And for the use case it would help a bit, but still be weird to set the exception as cause for itself (or create a new exception with the original as cause).

A workable suggestion to have raise only set the callstack if it is nil:

ex.callstack ||= Callstack.new

The callstack attribute is probably not set directly anywhere else - it wouldn't have mattered anyway because raise overwrites it - and is only set by raise. So an exception with callstack != nil will always have been raised somewhere.
This change would break behaviour if an exception is supposed to be re-raised with the current callstack. This seems quite unlikely, though, because it wouldn't make much sense to raise an existing exception with a new callstack. (If this was desired, a simple ex.callstack = nil would provide that behaviour)

An example implementation: https://gist.github.com/akzhan/13771b16dd12c01496a42a7153bcf72a

[faultyserver]

This is a somewhat orthogonal solution, but I ended up messing around with this for a while in Ruby and discovered that raise without an argument re-raises the previous exception, meaning you can capture an exception, modify it, and then re-raise it directly:

def foo
  raise Exception.new("what")
rescue Exception => ex
  # do something to `ex`
  raise
end

foo

$ ruby main.rb 
main.cr:2:in `foo': what (Exception)
        from main.cr:7:in `<main>'
$ 

The Crystal equivalent fails to compile, because raise expects an argument:

def foo
  raise Exception.new("what")
rescue ex : Exception
  raise
end

foo

in main.cr:4: wrong number of arguments for 'raise' (given 0, expected 1)
Overloads are:
 - raise(ex : Exception)
 - raise(message : String)

With that, adopting the behavior where raise without an argument re-raises the last exception wouldn't break any existing code (code that it would break would have failed to compile before now), and seems at least somewhat intuitive to me (though reraise would definitely be a better keyword for it).

This would behave like your example of rescue ex ... raise ex, but would explicitly _not_ modify the exception being raised in any way (callstack or otherwise).

[refi64]

FWIW Python has it as raise new_ex from previous_ex.

[akzhan]

Anyway we should add raise without arguments to rethrow exceptions (it should be compiled and worked only in rescue block).

[asterite]

@straight-shoota Exceptions have a cause property that you can pass when creating an exception, to wrap another one. I suggest to raise a new exception that wraps the old exception. That way you can preserve the inner exception call stack. I see no reason to do something other than that, and less if there's no compelling use case (maybe you can mention a reason to want to preserve the call stack like that).

[asterite]

And no, we won't implement argless raise: https://github.com/crystal-lang/crystal/issues/3181

[straight-shoota]

@asterite It would make no sense to wrap the original exception in a new one with cause, because the new exception would be exactly the same class, have identical message and other payload data.

begin
  do_some_stuff_might_raise
rescue ex : OrigException
  new_ex = OrigException.new(ex.message, cause: ex)
  # ... copy other payload data
  new_ex.context = some_context_data
  raise new_ex
end

This just feels not right and gets tedious for multiple exception classes.

/edit Ok, it would be possible to just duplicate ex and set one of the duplicates as cause for the other. But it still smells.

It's not that the kind of exception would change in any way, only relating to a previous exception (this would be a valid case for cause). The original exception could also be let through without any modification and still be 100% usable. Its just an augmentation that helps to diagnose the error cause and needs to be added to the exception while it is propagated upwards through the stack trace.

To detail my use case: I'm working on an implementation of the Jinja2 template engine. Exceptions are usually related to some location in the source code and while the parser knows the source string and can add it to any exception directly, the interpreter does not. So the source information needs to be added to an exception at a higher level, where it is known. It's no solution to pass the source info down into every part of the runtime evaluation just to be able to create an appropriate exception there.

[asterite]

But the source info is from the template source, which is not crystal code, right?

[straight-shoota]

Yeah, it's the template source code.

[straight-shoota]

Another use case where this would be handy might be filtering exceptions based on their message or other properties instead of only their type. For example if you're working with some status codes from an API or something. Lets say maybe HTTP status codes. It's not feasible to create an exception for every code, but you want to rescue only some specific status codes (maybe a 401 unauthorized to retry with authorization) but exceptions with other codes should be re-raised and handled elsewhere.

/edit I've just encountered an example where a configurable value determines if certain exceptions should be rescued and silenced without an error message. This use case can be addressed by switching between a branch with rescue and one without but this bloats up the code. This is much easier:

begin
  do_some_stuff_might_raise
rescue ex : Exception
  raise ex unless ignore_errors
end

[akzhan]

@asterite raise without arguments has another semantics: rescue block doesn't handle exception using its logic and need to rethrow original exception.

[ysbaddaden]

From a purely technical point of view, Exception#callstack is a nilable, so we could set the callstack only if it hasn't been already.

From a language design perspective, the question is whether raise should always reset the callstack, so it represents the last place the exception was raised (the current implementation), or if the callstack should only be set once and for all (this proposal).

For my personal use cases, maybe I'd set the callstack once (no strong opinion). I either catch then re-raise an exception based on some conditions (as examplified above by @straight-shoota) without modifying the exception at all (no need to reset and re-unwind the stack), or I raise a new exception, for example a MyLib::ConnectionError instead of a Socket::Error, discarding/hiding the previous exception and its callstack (implementation detail).

[asterite]

I'm seeing in Ruby that when you re-raise an exception the callstack remains the same. I believe this behaviour is correct, because catching and re-raising is simply a way to forward an exception while doing something in the middle. So I agree with some comments here, callstack should only be set by raise if it's not already set.

[asterite]

(and this probably works this way in other languages like Java and C#, when you catch and re-raise an exception)

[straight-shoota]

C# has throw without arguments for that. But Java works like this, there Throwable.fillInStackTrace() is called from the constructor of the exception and the stack trace is not influenced by any throw statement.

[asterite]

raise in Crystal is just a regular method, it's not a keyword like in C#. There's no way to access the currently rescued exception, so 👎 for me. Simply doing raise ex should be enough.

[refi64]

FWIW in Python, an exception can have two (or more) tracebacks: one from the original exception, and one from the re-raising point. It seems excessive, but it can really help when debugging weird issues.

[akzhan]

@kirbyfan64

Looks like we need to extend backtrace method with inner cause.backtrace.

[ysbaddaden]

@akzhan Exception#cause is an Exception? so ex.cause.backtrace already works. No need to hack on the root ex.backtrace.

[straight-shoota]

If you need separated tracebacks, you should probably use the original exception as cause of a new one.