This project is archived and is in readonly mode.

#896 ✓committed
Matt Palmer

Be a little more discerning about which exceptions to handle

Reported by Matt Palmer | August 25th, 2008 @ 07:43 AM | in 3.x

Throughout the Rails source, it is common to rescue Exception, however this isn't a particularly good idea as various quite fatal exceptions don't get a chance to do their thing (NoMemoryError, for one, although SignalException, SystemExit, and SystemStackError all signify something a little more deadly than something mundane like an ArgumentError).

I've prepared a patch that goes through and "fixes" the occurances of "rescue Exception" to something slightly more reasonable. The fixes were usually either just s/Exception/StandardError/, with the occasional more complicated fix. The few "rescue Exception" blocks I left were ones that just added some useful info then re-raised.

My changes are in the exception_happiness branch of git://

Comments and changes to this ticket

  • Pratik

    Pratik August 30th, 2008 @ 05:55 PM

    That sounds good. Could you please update a .diff file using git-format-patch ?


  • DHH

    DHH September 10th, 2008 @ 05:49 AM

    • State changed from “new” to “incomplete”

    Change to open when patch is attached.

  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • acechase

    acechase May 27th, 2010 @ 07:23 PM

    Hey guys,

    Perhaps I should open another ticket for this, please let me know if that's the case. I have a patch that addresses what is for me the most egregious case of overeager exception handling. In activerecord/lib/active_record/connection_adapters/abstract_adapter.rb the #log method rescues Exception and then re-raises as an ActiveRecord::StatementInvalid exception.

    Some people have hit this problem when they have slow queries that are hanging the system and so I think the response is generally "fix your query". But we're hitting this problem when we try to exit out of our background processes that are just cranking through a whole bunch of data processing.

    In these background processes we have designed them to be tolerant to exceptions and expect the process to die when it's sent a SIGINT or SystemExit. However, we also want those processes to stay alive and continue on to the next job in the queue if any particular job hits an unexpected exception. So we take the approach of wrapping our job invocations in a 'rescue Exception' block and re-raising (allowing the process to die) in the case where the exception is a SystemExit or SignalException. So the AR approach of rescuing and re-raising as a StatementInvalid breaks this approach and leaves us in a situation where we have to do resort to less reliable methods of inspecting the rescued exceptions, like parsing the exception message. It also leads to confusing behavior where you sometimes see the process die cleanly (when it's not invoking an AR statement at the time the SystemExit is received) or continue running (when the exception was caught and re-wrapped).

    I'm guessing there are other folks out there how are experiencing similar problems and so I'd like to offer a patch that applies against the 2-3-stable branch (rather than 3.x).

  • Repository

    Repository May 29th, 2010 @ 05:01 AM

    • State changed from “incomplete” to “committed”

    (from [3d6ed501870f2a790c2d0e288e0f294e8c5f7be3]) Don't rewrap system level exceptions with StatementInvalid

    Signed-off-by: Michael Koziarski
    [#896 state:committed]

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href=""></a>

People watching this ticket



Referenced by