BUGFIX: Fix bug where 'inspect_with_backtrace' raises an IndexError i…

[marksiemers]

This is a bug in crystal maybe?

[faustinoaq]

@marksiemers @eliasjpr Does this happen only on macOS?

I'm using Manjaro Linux and I haven't had this issue yet 😅

[elorest]

Both times my tests have failed because of it I have been using my living room iMac. It never used to fail before December though...

[marksiemers]

@faustinoaq @elorest - Did it ever fail on travis?

@faustinoaq - either way, I think having a private method and a begin ... rescue is worth it.

In the case of a 500 error the overhead of this shouldn't really matter, response time should be a second priority to catching exception and error logging.

Additionally, for different environments (e.g. production) we probably do not want to render backtrace, so the private method here can expand to consider things like that.

I can and will change the puts statement to something else.

[marksiemers]

As far as crystal goes, it looks like it has to do with here:
https://github.com/crystal-lang/crystal/blob/v0.24.1/src/exception.cr#L35

And therefore here:
https://github.com/crystal-lang/crystal/blob/351596855d635141deb204f7c572ed615f415ce5/src/callstack.cr#L157

And in callstack, it does do something different for macOS vs linux:
https://github.com/crystal-lang/crystal/blob/351596855d635141deb204f7c572ed615f415ce5/src/callstack.cr#L8

[marksiemers]

Please don't delete this branch, unless you update the issue with crystal: crystal-lang/crystal#5678

Currently, this branch is referenced for reproducing a bug.

[elorest]

@faustinoaq @elorest - Did it ever fail on travis?@faustinoaq - either way, I think having a private method and a begin ... rescue is worth it.

I don't think it ever failed on travis (for this reason), but we obviously need the specs to pass locally for people running OSX as well. Also development will be weird if the message isn't returned. I would recommend returning the message from the rescue if it errors on inspect_with_backtrace, that way in development people will still see the error that was caused.

Similar to #626

[marksiemers]

@elorest - The message from the original exception is returned in the case that inspect_with_backtrace raises an IndexError

The first priority is still @ex.inspect_with_backtrace and if that doesn't work, the second priority is @ex.message

line 39 runs in either case.

[eliasjpr]

Will this not work?

private def internal_server_error_message
  message = @ex.inspect_with_backtrace
rescue ex : IndexError
  # ERROR: Any of these will cause another IndexError (at least on macOS)
  # ex.inspect_with_backtrace # or @ex.inspect_with_backtrace
  # ex.backtrace? # or @ex.backtrace?
  @ex.message
end

Also it might be worth to use the flags just like in the crystal source code flag?(:freebsd) || flag?(:linux) || flag?(:openbsd) instead of rescueing

[marksiemers]

Yeah, this should work:

private def internal_server_error_message
  @ex.inspect_with_backtrace
rescue ex : IndexError
  @ex.message
end

I prefer not to count on flags in this case. It is a bug in crystal, which has been resolved in master:
crystal-lang/crystal#5224
crystal-lang/crystal@5eecd57

[eliasjpr]

I dont have a lot of context here. But left a comment based on my understanding.

[eliasjpr]

Will this not work?

private def internal_server_error_message
  message = @ex.inspect_with_backtrace
rescue ex : IndexError
  # ERROR: Any of these will cause another IndexError (at least on macOS)
  # ex.inspect_with_backtrace # or @ex.inspect_with_backtrace
  # ex.backtrace? # or @ex.backtrace?
  @ex.message
end

Also it might be worth to use the flags just like in the crystal source code flag?(:freebsd) || flag?(:linux) || flag?(:openbsd) instead of rescueing

[marksiemers]

It looks like a fix was merged for this issue: 055579f

Do we want this code with the private method, or leave it as is?
@elorest @eliasjpr

I like the idea of a private method because I think we will end up needing one anyway (to produce different messages based on the environment).

[eliasjpr]

I am fine with the private method

[marksiemers]

@elorest @drujensen

[elorest]

Have a couple comments but then lets merge this.

[elorest]

I think we should leave these comments here or else someone will refactor it again because they won't understand why there's added complexity here.

[elorest]

Rather than capturing wild card error and printing "An unknown error occured." we should capture wild card and then print both @ex.message and new ex.mesage. Unknown error message is confusing and hides the fact that there is an actual error.

[elorest]

I'm fine with moving this into a private method especially since this only gets called if there's an error.

Personally I think we abstract code into methods too much though. In a lot of cases it's just as readable either way and slows down the code minutely every branch we add to the AST.

[marksiemers]

Updates made.

We should revisit this code though. It needs to do different things based on environment. Right now, in production, it will return the stack trace or exception messages to the browser. It should do something more like this:

• In development, do what it is currently doing
• In production, log the stack trace or exception messages with logger.error (or whatever is appropriate), and return a more user-friendly error message to the browser.

stale