When is it legit to rescue Exception?

ruby-talk
1

Hi everyone,

In a current project I ended up writing the code below (relevant part is
the last rescue statement):

def perform_single_request(conn = init_connection)

  self.last_status = 'started'
  begin
    response = http_call_with_retries(conn)
    if validate_response(response)
      handle_response_success(response, self)
    else
      handle_response_error(response)
    end
  # Error types that we're allowed to **believe** they're transient. We
want
  # to save current request state, so that the next time it runs,
  # it will continue from where it was.
  rescue Faraday::Error::ClientError, SyncEngine::Error::RateLimitExceeded
=> e
    errback(e)
    save!
  rescue SyncEngine::Error::Base => e
    errback(e)
    clear_pagination
    save!

* rescue Exception => e # We don't want another exception to be raised.
Here we will # *try* to finish the execution as clean as possible, but
in any case # what we want is the original exception to be re-raised,
not a different # one. begin errback(e) clear_pagination
  save! rescue Exception end raise e end*
end

Do you think this is a legit use of rescue Exception? I'm a aware of some
of the issues with it, but now I'm wondering if there may be cases that
could make such code critical or dangerous.

What would be legit uses of rescue Exception?

Thanks
Sam B.

2

First of all it seems these two lines should go in an "ensure" clause
and be removed from all "rescue" clauses:

clear_pagination
save!

As far as I can see these are executed in all branches.

Then the question remains whether you need errback(e). Since I have no
idea what you are doing there I cannot really help you with that. If
that is not needed, you can remove "rescue Exception => e".

Generally the pattern with the resource acquisition after "begin"
looks suspicious. Usually the approach is (simplified example, of
course you would normally use the block form of File.open for this):

io = File.open(file_name) # acquire resource
begin
  puts ">>>"
  io.each_line do |l|
    puts l
  end
  puts "<<<"
ensure
  io.close # release resource
end

Since we do not know the details of your solution it's difficult to
come up with more advice - for me at least.

Note also that you can reraise the last exception by simply only
writing "raise".

Kind regards

robert

PS: See also my ancient blog post:
http://blog.rubybestpractices.com/posts/rklemme/002_Writing_Block_Methods.html

···

On Mon, Jan 11, 2016 at 1:09 PM, Samuel Brandão <gb.samuel@gmail.com> wrote:

Hi everyone,

In a current project I ended up writing the code below (relevant part is the
last rescue statement):

def perform_single_request(conn = init_connection)
  self.last_status = 'started'
  begin
    response = http_call_with_retries(conn)
    if validate_response(response)
      handle_response_success(response, self)
    else
      handle_response_error(response)
    end
  # Error types that we're allowed to **believe** they're transient. We
want
  # to save current request state, so that the next time it runs,
  # it will continue from where it was.
  rescue Faraday::Error::ClientError, SyncEngine::Error::RateLimitExceeded
=> e
    errback(e)
    save!
  rescue SyncEngine::Error::Base => e
    errback(e)
    clear_pagination
    save!
  rescue Exception => e
    # We don't want another exception to be raised. Here we will
    # *try* to finish the execution as clean as possible, but in any case
    # what we want is the original exception to be re-raised, not a
different
    # one.
    begin
      errback(e)
      clear_pagination
      save!
    rescue Exception
    end
    raise e
  end
end

Do you think this is a legit use of rescue Exception? I'm a aware of some of
the issues with it, but now I'm wondering if there may be cases that could
make such code critical or dangerous.

What would be legit uses of rescue Exception?

--
[guy, jim, charlie].each {|him| remember.him do |as, often| as.you_can
- without end}
http://blog.rubybestpractices.com/

3

imc, i log everything. so when and exception comes up, i log then raise again.
thats all.

best regards --botp

···

On Mon, Jan 11, 2016 at 8:09 PM, Samuel Brandão <gb.samuel@gmail.com> wrote:

What would be legit uses of rescue Exception?

4

Thanks for your reply, Robert!

First of all it seems these two lines should go in an "ensure" clause

and be removed from all "rescue" clauses:
clear_pagination
save!
As far as I can see these are executed in all branches.

We're not using ensure as there is one case we do not want this to happen
(rescue Faraday::Error ...). On the other hand, the last two rescues also
have a small difference, as only the last one re-raise the exception. I
definitely agree we could benefit of some DRYing there, but I have no clue
on how to proceed while keeping the small differences across the different
rescue clauses and keeping readability high. Suggestions? :smiley:

Then the question remains whether you need errback(e). Since I have no

idea what you are doing there I cannot really help you with that. If
that is not needed, you can remove "rescue Exception => e".

You're right, the => e is definitely not needed on the last statement.
errback(e) implements app-specific logic, mainly stuff regarding error
logging since this code is executed as a background job w/ resque on the
server, and we need to keep track of what did not go well.

My main concern is about rescuing or not Exception, is it dangerous?
Simplifying and purging out app specific context, this is the core issue:

begin
   raise SomeError.new("Bu!") # hardware failures, strange interrupts,
application errors... SomeError can be a descendant of StandardError or
not, who knows?
rescue StandardError => e
   # handle application-level errors
rescue Exception => e
    # If we get here, we're likely not talking about app-level errors.
    begin
      # Try performing some cleanup, error logging, etc. If this fails we
don't care, at least we tried.
    rescue Exception
    end
    raise e
  end

Since we're rescuing Exception, it may be the case we have some condition
that won't even allow this cleanup to happen properly, so we risk another
exception to be raised. If that happen, I don't want this second exception
to be the one that will be raised, but the original one, that's why I'm
using a rescue instead of an ensure. Does it make sense? I'm assuming here
that the first exception raised has more important information for
debugging purposes, and when I re-raise it, I expect to see it in Airbrake
to diagnose it later. What do you think?

And thanks for pointing out your blog post! I will read it again when I get
home later today :wink:

Thanks
Sam

5

Personally I'd avoid Exception altogether and use StandardError. Tangentially related, but consider this example:
$ ruby -e "begin; raise Exception; rescue; puts 'rescued'; end"
-e:1:in `<main>': Exception (Exception)
$ ruby -e "begin; raise Exception; rescue => e; puts 'rescued'; end"
-e:1:in `<main>': Exception (Exception)
$ ruby -e "begin; raise Exception; rescue Exception; puts 'rescued'; end"
rescued
This is really more of a "don't raise Exception" recommendation, but it shows how it exhibits some potentially surprising behavior

Josh

···

From: Samuel Brandão <gb.samuel@gmail.com>
To: Ruby users <ruby-talk@ruby-lang.org>
Sent: Monday, January 11, 2016 11:38 AM
Subject: Re: When is it legit to rescue Exception?
   
Thanks for your reply, Robert!

First of all it seems these two lines should go in an "ensure" clause
and be removed from all "rescue" clauses:
clear_pagination
save!
As far as I can see these are executed in all branches.

We're not using ensure as there is one case we do not want this to happen (rescue Faraday::Error ...). On the other hand, the last two rescues also have a small difference, as only the last one re-raise the exception. I definitely agree we could benefit of some DRYing there, but I have no clue on how to proceed while keeping the small differences across the different rescue clauses and keeping readability high. Suggestions? :smiley:

Then the question remains whether you need errback(e). Since I have no
idea what you are doing there I cannot really help you with that. If
that is not needed, you can remove "rescue Exception => e".

You're right, the => e is definitely not needed on the last statement. errback(e) implements app-specific logic, mainly stuff regarding error logging since this code is executed as a background job w/ resque on the server, and we need to keep track of what did not go well.
My main concern is about rescuing or not Exception, is it dangerous? Simplifying and purging out app specific context, this is the core issue:
begin
raise SomeError.new("Bu!") # hardware failures, strange interrupts, application errors... SomeError can be a descendant of StandardError or not, who knows?
rescue StandardError => e
# handle application-level errors
rescue Exception => e
# If we get here, we're likely not talking about app-level errors.
begin
# Try performing some cleanup, error logging, etc. If this fails we don't care, at least we tried.
rescue Exception
end
raise e
end

Since we're rescuing Exception, it may be the case we have some condition that won't even allow this cleanup to happen properly, so we risk another exception to be raised. If that happen, I don't want this second exception to be the one that will be raised, but the original one, that's why I'm using a rescue instead of an ensure. Does it make sense? I'm assuming here that the first exception raised has more important information for debugging purposes, and when I re-raise it, I expect to see it in Airbrake to diagnose it later. What do you think?

And thanks for pointing out your blog post! I will read it again when I get home later today :wink:
ThanksSam

6

Thanks for your reply, Robert!

You're welcome!

We're not using ensure as there is one case we do not want this to happen
(rescue Faraday::Error ...). On the other hand, the last two rescues also
have a small difference, as only the last one re-raise the exception. I
definitely agree we could benefit of some DRYing there, but I have no clue
on how to proceed while keeping the small differences across the different
rescue clauses and keeping readability high. Suggestions? :smiley:

See at end.

Then the question remains whether you need errback(e). Since I have no
idea what you are doing there I cannot really help you with that. If
that is not needed, you can remove "rescue Exception => e".

You're right, the => e is definitely not needed on the last statement.
errback(e) implements app-specific logic, mainly stuff regarding error
logging since this code is executed as a background job w/ resque on the
server, and we need to keep track of what did not go well.

I actually meant the complete rescue clause, not only the "=> e".

My main concern is about rescuing or not Exception, is it dangerous?

Well, with the information I have, the answer is: it depends. :wink:

Put differently: I don't think there is a general answer to that,
which seems to be what you are hoping for. The matter and situations
are far too complex to be adequately captured with a simple rule like
"Do not catch Exception" or "Catching Exception is OK".

Since we're rescuing Exception, it may be the case we have some condition
that won't even allow this cleanup to happen properly, so we risk another
exception to be raised. If that happen, I don't want this second exception
to be the one that will be raised, but the original one, that's why I'm
using a rescue instead of an ensure. Does it make sense? I'm assuming here
that the first exception raised has more important information for debugging
purposes, and when I re-raise it, I expect to see it in Airbrake to diagnose
it later. What do you think?

I think there are far too many different exceptions in play. It may
point to a less than optimal structure of the code. Maybe you need to
separate concerns better by different layering of functionality. I
suppose that would also reduce the complexity of error handling and
the need for these many rescue clauses etc. But, as I said, that is
purely speculative with the little information I have.

Kind regards

robert

···

On Mon, Jan 11, 2016 at 5:38 PM, Samuel Brandão <gb.samuel@gmail.com> wrote:

--
[guy, jim, charlie].each {|him| remember.him do |as, often| as.you_can
- without end}
http://blog.rubybestpractices.com/