Project

General

Profile

Actions
Copy link

Defect #31387

closed

Don't rescue Exception class

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

Status:
Closed
Priority:
Normal
Assignee:
Go MAEDA
Category:
Code cleanup/refactoring
Target version:
4.1.0
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

Download all files
0001-Custom-Exception-classes-should-inherit-StandardErro.patch (3.78 KB) 0001-Custom-Exception-classes-should-inherit-StandardErro.patch Go MAEDA, 2019-05-20 13:44
0002-Don-t-rescue-Exception.patch (19.3 KB) 0002-Don-t-rescue-Exception.patch Go MAEDA, 2019-05-20 14:24

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

Updated by Go MAEDA over 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
Copy link
 #2

Updated by Go MAEDA over 5 years ago

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

Updated by Go MAEDA over 5 years ago

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

Actions
Copy link
 #4

Updated by Pavel Rosický over 5 years ago

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

it should be rescue SyntaxError, StandardError => e

Actions
Copy link
 #5

Updated by Go MAEDA over 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
Copy link
 #6

Updated by Go MAEDA over 5 years ago

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

Updated by Pavel Rosický over 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
Copy link
 #8

Updated by Go MAEDA over 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
Copy link
 #9

Updated by Go MAEDA over 5 years ago

  • Assignee set to Go MAEDA
Actions
Copy link
 #10

Updated by Go MAEDA over 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
Copy link
 #11

Updated by Go MAEDA over 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
Copy link
 #12

Updated by Go MAEDA over 5 years ago

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

Updated by Pavel Rosický over 5 years ago

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

Actions
Copy link
 #14

Updated by Go MAEDA over 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
Copy link

Also available in: Atom PDF