Adam Spiers (aspi...@suse.com) wrote: > *** PUBLIC HEALTH AND SAFETY ANNOUNCEMENT *** > > I notice Crowbar has a lot of > > rescue Exception => e > > clauses. AFAIK, this is widely considered to be a bad idea: > > > http://stackoverflow.com/questions/10048173/why-is-it-bad-style-to-rescue-exception-e-in-ruby > > Next time you encounter this, please consider either changing it to > rescue a much narrower scope within the Exception hierarchy, or (in > some cases) rewriting it to avoid the need to catch exceptions at all. > > This announcement was prompted by observing a horrible bug in pebbles > where the *only* evidence that something went wrong was seeing > "undefined method `[]’ for nil:NilClass" in the flash. Not a single > helpful stacktrace or error message in the logs :-( I can't be sure > yet, but I suspect the guilty code is this: > > def save_node > ... > begin > ... > @node.save > true > rescue Exception=>e > flash[:notice] = @node.name + ": " + t('nodes.list.failed') + ": " + > e.message > false > end > end > > That's quite evil, because it a) catches a huge range of exceptions > which it shouldn't, b) guarantees near-silent failure, and c) > eliminates any reasonable chance of debugging the failure.
I have made a start on fixing this for Pebbles: https://github.com/crowbar/barclamp-crowbar/pull/693 _______________________________________________ Crowbar mailing list Crowbar@dell.com https://lists.us.dell.com/mailman/listinfo/crowbar For more information: http://crowbar.github.com/
