Remove bad exception handling! #2476
Conversation
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 etc which is a bad idea. Fix: rescue from StandardError.
lib/tasks/paperclip.rake
Outdated
| @@ -46,7 +46,7 @@ namespace :paperclip do | |||
| attachment = instance.send(name) | |||
| begin | |||
| attachment.reprocess!(*styles) | |||
| rescue Exception => e | |||
| rescue => e | |||
There was a problem hiding this comment.
I think it would be even better to explicitly say rescue StandardError => e
There was a problem hiding this comment.
Agreed, this appears to have the same problem as before, it's just now even broader. Agree in principle that rescuing from StandardError would be an improvement unless there's some system reason that we needed to use Exception here. Since it's not covered by a test I think we can assume this was just an oversight.
lib/tasks/paperclip.rake
Outdated
| @@ -46,7 +46,7 @@ namespace :paperclip do | |||
| attachment = instance.send(name) | |||
| begin | |||
| attachment.reprocess!(*styles) | |||
| rescue Exception => e | |||
| rescue => e | |||
There was a problem hiding this comment.
Agreed, this appears to have the same problem as before, it's just now even broader. Agree in principle that rescuing from StandardError would be an improvement unless there's some system reason that we needed to use Exception here. Since it's not covered by a test I think we can assume this was just an oversight.
|
Updated. Ready for another pass. |
|
Looks great! Thank you for your contribution. |
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 etc which is a bad idea.
Fix: rescue from StandardError.