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

rescue_from :all not rescuing all exceptions #1713

Open
Jelkster opened this issue Nov 21, 2017 · 17 comments
Open

rescue_from :all not rescuing all exceptions #1713

Jelkster opened this issue Nov 21, 2017 · 17 comments
Labels

Comments

@Jelkster
Copy link
Contributor

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
Copy link
Member

dblock commented Nov 21, 2017

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?

@dblock dblock added the bug? label Nov 21, 2017
@Jelkster
Copy link
Contributor Author

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
Copy link
Member

dblock commented Nov 22, 2017

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

@dblock
Copy link
Member

dblock commented Jan 1, 2018

Doc updated in #1725. What do we want to do about this issue?

@Jelkster
Copy link
Contributor Author

Jelkster commented Jan 3, 2018

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

@dblock
Copy link
Member

dblock commented Jan 3, 2018

Yes please @Jelkster

@mtsmfm
Copy link

mtsmfm commented Feb 28, 2018

Is it really documentation problem?
I think rescue_from :all should rescue all exceptions as before f038ed5#diff-38c8cda4fd3b2789c88488c6e1e08d0eL28
because it looks unintentional change.

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

@Jelkster
Copy link
Contributor Author

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
Copy link
Member

dblock commented Mar 1, 2018

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
Copy link

mtsmfm commented Mar 1, 2018

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

@Jelkster
Copy link
Contributor Author

Jelkster commented Mar 1, 2018

@mtsmfm go ahead and send it.

@dblock
Copy link
Member

dblock commented Mar 11, 2018

Rebase against #1749.

@fanantoxa
Copy link

fanantoxa commented Mar 15, 2018

@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
Copy link
Member

dblock commented Mar 15, 2018

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

@fanantoxa
Copy link

@dblock Got it. Thank you for fast response.

@dm1try
Copy link
Member

dm1try commented Mar 15, 2018

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
Copy link

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

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

No branches or pull requests

5 participants