Defect #31387

Don't rescue Exception class

Added by Pavel Rosický 7 months ago. Updated 7 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Code cleanup/refactoring
Target version:4.1.0
Resolution:Fixed Affected version:

Description

https://stackoverflow.com/questions/10048173/why-is-it-bad-style-to-rescue-exception-e-in-ruby

https://thoughtbot.com/blog/rescue-standarderror-not-exception

Exception is the root of Ruby's exception hierarchy, so when you rescue Exception you rescue from everything, including subclasses such as SyntaxError, LoadError, and Interrupt.

Rescuing Interrupt prevents the user from using CTRL-C to exit the program.

I've seen a recent addition to redmine

def destroy
  IssueStatus.find(params[:id]).destroy
  redirect_to issue_statuses_path
rescue Exception => e
  flash[:error] = l(:error_unable_delete_issue_status, ERB::Util.h(e.message))
  redirect_to issue_statuses_path
end

https://github.com/redmine/redmine/blob/aed2d197a0e897e8a83013384b95353e110bd2f8/app/controllers/issue_statuses_controller.rb#L77

please replace these "safe" exceptions. StandardError is usually more than sufficient.

0001-Custom-Exception-classes-should-inherit-StandardErro.patch Magnifier (3.78 KB) Go MAEDA, 2019-05-20 13:44

0002-Don-t-rescue-Exception.patch Magnifier (19.3 KB) Go MAEDA, 2019-05-20 14:24


Related issues

Related to Redmine - Feature #31361: Include a reason in the error message when an issue statu... Closed
Related to Redmine - Patch #29441: Remove code related to JRuby and unsupported Ruby versions Closed

Associated revisions

Revision 18187
Added by Go MAEDA 7 months ago

Custom Exception classes should inherit StandardError instead of Exception (#31387).

Patch by Go MAEDA.

Revision 18197
Added by Go MAEDA 7 months ago

Don't rescue Exception class (#31387).

Patch by Go MAEDA and Pavel Rosický.

History

#1 Updated by Go MAEDA 7 months ago

Thank you for pointing it out.

Maybe we should also check the following lines, shouldn't we?

$ egrep -rn 'rescue\s+Exception' app config lib extra
app/models/mailer.rb:705:    rescue Exception => e
app/models/repository/git.rb:87:  rescue Exception => e
app/models/repository.rb:385:    rescue Exception => e
app/models/repository.rb:395:    rescue Exception => e
app/models/repository.rb:405:    rescue Exception => e
app/models/mail_handler.rb:56:  rescue Exception => e
app/models/import.rb:58:      rescue Exception => e
app/models/import.rb:262:      rescue Exception => e
app/controllers/auth_sources_controller.rb:63:    rescue Exception => e
app/controllers/admin_controller.rb:57:      rescue Exception => e
app/controllers/admin_controller.rb:68:    rescue Exception => e
config/routes.rb:358:      rescue Exception => e
lib/redmine/plugin.rb:421:        rescue Exception => e
lib/redmine/plugin.rb:432:        rescue Exception => e
lib/redmine/plugin.rb:443:        rescue Exception => e
lib/redmine/scm/adapters/mercurial_adapter.rb:95:        rescue Exception => e
lib/redmine/scm/adapters/subversion_adapter.rb:118:            rescue Exception => e
lib/redmine/scm/adapters/abstract_adapter.rb:254:          rescue Exception => e
lib/redmine/scm/adapters/abstract_adapter.rb:285:          rescue Exception => err
lib/tasks/email.rake:167:      rescue Exception => e
lib/tasks/migrate_from_trac.rake:617:      rescue Exception => e
lib/tasks/migrate_from_trac.rake:632:      rescue Exception => e
lib/tasks/locales.rake:171:        rescue Exception => e1
lib/tasks/locales.rake:176:    rescue Exception => e
extra/svn/reposman.rb:57:  rescue Exception => e
extra/mail_handler/rdm-mailhandler.rb:207:    rescue Exception => e

#2 Updated by Go MAEDA 7 months ago

  • Related to Feature #31361: Include a reason in the error message when an issue status cannot be deleted added

#3 Updated by Go MAEDA 7 months ago

Here are patches to fix not to raise/rescue Exception. I hope someone to review these patches.

#4 Updated by Pavel Rosický 7 months ago

thanks, there's a typo in config/routes.rb

it should be rescue SyntaxError, StandardError => e

#5 Updated by Go MAEDA 7 months ago

Pavel Rosický wrote:

it should be rescue SyntaxError, StandardError => e

Thanks, updated the patch. Could you tell me why it caches SyntaxError? To detect syntax error in plugins' routes.rb?

#6 Updated by Go MAEDA 7 months ago

  • File deleted (0002-Don-t-rescue-Exception.patch)

#7 Updated by Pavel Rosický 7 months ago

exceptions have a hierarchy

Exception
 NoMemoryError
 ScriptError
   LoadError
   NotImplementedError
   SyntaxError
 SignalException
   Interrupt
 StandardError
   ArgumentError
   IOError
     EOFError
   IndexError
   LocalJumpError
   NameError
     NoMethodError
   RangeError
     FloatDomainError
   RegexpError
   RuntimeError
   SecurityError
   SystemCallError
   SystemStackError
   ThreadError
   TypeError
   ZeroDivisionError
 SystemExit

as you can see SyntaxError doesn't inherit from StandardError. It's a common mistake and the original intention was to show a better message that there's an error in a plugin instead of a long and confusing backtrace that Ruby would usually print.

#8 Updated by Go MAEDA 7 months ago

  • Target version changed from Candidate for next minor release to 4.1.0

Setting the target version to 4.1.0.

#9 Updated by Go MAEDA 7 months ago

  • Assignee set to Go MAEDA

#10 Updated by Go MAEDA 7 months ago

Pavel Rosický wrote:

as you can see SyntaxError doesn't inherit from StandardError. It's a common mistake and the original intention was to show a better message that there's an error in a plugin instead of a long and confusing backtrace that Ruby would usually print.

You are right.

With catching SyntaxError:

An error occurred while loading the routes definition of redmine_message_customize plugin (/Users/maeda/redmines/redmine-trunk/plugins/redmine_message_customize/config/routes.rb): (eval):7: syntax error, unexpected end-of-input, expecting '}'.

Without catching SyntaxError:

#11 Updated by Go MAEDA 7 months ago

According to r6230, it seems that we have to catch Exception in lib/redmine/scm/adapters/abstract_adapter.rb unless we drop the support for JRuby.

Index: lib/redmine/scm/adapters/abstract_adapter.rb
===================================================================
--- lib/redmine/scm/adapters/abstract_adapter.rb    (リビジョン 6229)
+++ lib/redmine/scm/adapters/abstract_adapter.rb    (リビジョン 6230)
@@ -224,7 +224,11 @@
               io.close_write
               block.call(io) if block_given?
             end
-          rescue Errno::ENOENT => e
+          ## If scm command does not exist,
+          ## Linux JRuby 1.6.2 (ruby-1.8.7-p330) raises java.io.IOException
+          ## in production environment.
+          # rescue Errno::ENOENT => e
+          rescue Exception => e
             msg = strip_credential(e.message)
             # The command failed, log it and re-raise
             logmsg = "SCM command failed, " 

#12 Updated by Go MAEDA 7 months ago

  • Related to Patch #29441: Remove code related to JRuby and unsupported Ruby versions added

#13 Updated by Pavel Rosický 7 months ago

This was a fix for a very, very old jruby version. It should match MRI's behaviour now.

#14 Updated by Go MAEDA 7 months ago

  • Subject changed from don't rescue Exception to Don't rescue Exception class
  • Status changed from New to Closed
  • Resolution set to Fixed

Committed the patch.

Pavel Rosický wrote:

This was a fix for a very, very old jruby version. It should match MRI's behaviour now.

Thank you for the advice. I committed the following code.

Index: lib/redmine/scm/adapters/abstract_adapter.rb
===================================================================
--- lib/redmine/scm/adapters/abstract_adapter.rb    (リビジョン 18196)
+++ lib/redmine/scm/adapters/abstract_adapter.rb    (リビジョン 18197)
@@ -247,11 +247,7 @@
               io.close_write unless options[:write_stdin]
               block.call(io) if block_given?
             end
-          ## If scm command does not exist,
-          ## Linux JRuby 1.6.2 (ruby-1.8.7-p330) raises java.io.IOException
-          ## in production environment.
-          # rescue Errno::ENOENT => e
-          rescue Exception => e
+          rescue => e
             msg = strip_credential(e.message)
             # The command failed, log it and re-raise
             logmsg = "SCM command failed, " 

Also available in: Atom PDF