Project

General

Profile

Actions

Defect #31387

closed

Don't rescue Exception class

Added by Pavel Rosický almost 5 years ago. Updated almost 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
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.


Files


Related issues

Related to Redmine - Feature #31361: Include a reason in the error message when an issue status cannot be deletedClosedGo MAEDA

Actions
Related to Redmine - Patch #29441: Remove code related to JRuby and unsupported Ruby versionsClosedJean-Philippe Lang

Actions
Actions #1

Updated by Go MAEDA almost 5 years 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
Actions #2

Updated by Go MAEDA almost 5 years ago

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

Updated by Go MAEDA almost 5 years ago

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

Actions #4

Updated by Pavel Rosický almost 5 years ago

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

it should be rescue SyntaxError, StandardError => e

Actions #5

Updated by Go MAEDA almost 5 years 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?

Actions #6

Updated by Go MAEDA almost 5 years ago

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

Updated by Pavel Rosický almost 5 years 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.

Actions #8

Updated by Go MAEDA almost 5 years ago

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

Setting the target version to 4.1.0.

Actions #9

Updated by Go MAEDA almost 5 years ago

  • Assignee set to Go MAEDA
Actions #10

Updated by Go MAEDA almost 5 years 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:

Show

Actions #11

Updated by Go MAEDA almost 5 years 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, " 
Actions #12

Updated by Go MAEDA almost 5 years ago

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

Updated by Pavel Rosický almost 5 years ago

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

Actions #14

Updated by Go MAEDA almost 5 years 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, " 
Actions

Also available in: Atom PDF