Grape: rescue_from :all not rescuing all exceptions

[Jelkster]

According to the README, grape can rescue from all exceptions with the following:

rescue_from :all

I recently stumbled across a case where the wrong method accidentally raised a NotImplementedError and caused the web request to hang as a result. I was surprised to see this because I had the rescue_from :all included in the API. I suspected that maybe NotImplementedError was not a StandardError and maybe that somehow had something to do with it. Sure enough, I found that it was a ScriptError.

I consulted the documentation again to see if I missed something, and actually saw an example of rescue_from NotImplementedError in the README. So it appeared as though it was perfectly valid at some point in time. That being said, I also saw the following message below an example: _"In this case UserDefinedError must be inherited from StandardError"_.

After taking a look at error.rb it appears as though only StandardError will get rescued. I modified the source and replaced StandardError with Exception to see if that would resolve the issue and it did!

The last thing I did was inspect the history of the error.rb file to see if it was ever rescuing Exception before, and sure enough it was. It appears Rubocop recommended the change.

[dblock]

I think we definitely need to update the documentation first, make sure the specs cover all the scenarios, and possibly add a rescue_from :exception that will behave differently? Please contribute?

[Jelkster]

I'm willing to try and help. Regarding the documentation, would you like me to remove the NotImplementedError example only, or do we also need to address each mention of "...all exceptions..."?

[dblock]

Whatever you think is appropriate, try something and we can look at it.

[dblock]

Doc updated in https://github.com/ruby-grape/grape/pull/1725. What do we want to do about this issue?

[Jelkster]

I think your rescue_from :exception proposal is good. Want me to take a stab at that?

[dblock]

Yes please @Jelkster

[mtsmfm]

Is it really documentation problem?
I think rescue_from :all should rescue all exceptions as before https://github.com/ruby-grape/grape/commit/f038ed5457cef803b0275e4ad7a99674e61e4394#diff-38c8cda4fd3b2789c88488c6e1e08d0eL28
because it looks unintentional change.

What do you think about replacing this line with rescue Exception => e like Rails?

[Jelkster]

That would make my life easier, as I'm currently in the process of implementing a separate rescue_from :exception option that does what you'd expect :all to do.

[dblock]

I am willing to look at any PR, but this is a pretty major change and it will affect lots of people, so there be dragons.

[mtsmfm]

@Jelkster Can I send a PR which rescue truly all exceptions? Are you implementing now?

[Jelkster]

@mtsmfm go ahead and send it.

[dblock]

Rebase against https://github.com/ruby-grape/grape/pull/1749.

[fanantoxa]

@dblock Hi. I have a similar issue.
I'm using mandrill-api gem and grape 1.0.2 doesn't handle exception inherited from Exception class.

I've tried to add

rescue_from :all
rescue_from Exception
rescue_from Mandrill::Error
rescue_from InvalidKeyError

Any of this doesn't work.

I've update Gemfile to use last master commit and started partly work
So next is working now:

rescue_from Mandrill::Error
rescue_from InvalidKeyError

But

rescue_from :all

still doesn't care about exceptions like this.

I saw from docs that it handle StandartException, but is that all?
I meant https://ruby-doc.org/core-2.5.0/Exception.html say that StandartException it's just a subclass of Exception so maybe better to handle them all or this is madnrill issue?

Here is the example of mandrill code:
https://bitbucket.org/mailchimp/mandrill-api-ruby/src/044396ea8f760a617a1b62e28f69b77833d82d55/lib/mandrill/errors.rb?at=master&fileviewer=file-view-default

[dblock]

These mandrill errors should have been derived from StandardError, but yes this is the same problem. Still taking PRs, see discussion above.

[fanantoxa]

@dblock Got it. Thank you for fast response.

[dm1try]

I've update Gemfile to use last master commit and started partly work
So next is working now:

rescue_from Mandrill::Error
rescue_from InvalidKeyError

rescue_from Exception should also work on master, feel free to provide a failing spec if it does not work

[fanantoxa]

@dm1try Yes, it is. It's still mandrill issue