MantisBT - Rosetta
View Issue Details
0000168Rosetta[All Projects] Crashpublic2013-01-04 10:552013-03-21 15:08
smlewis 
mtyka 
highcrashalways
resolvedfixed 
All platformsAnyAny
Trunk 
 
any
ex: AnchoredDesign -thisisnotanoption
53112
0000168: Bad option reads throw uncaught exceptions
I don't know how long this has been going on, but bad option reads (non-existent option files, or non-existent options) causes the option reader to throw uncaught exceptions. The option reading is done inside init, which is NOT expected to be wrapped at app-level in a try/catch. I'm guessing the right place to put the try/catch is at the top of devel::init or protocols::init, on the theory that failed initialization is always a killer (but should be cleanly reported).
fixbb IS wrapped in a try/catch for some reason - is that the new standard?
exception handling, Minicon_2013_defaults, robust_rosetta
related to 0000200new  When given a bad option on the command line print what is usually displayed with -help 
parent of 0000189resolved renfrew Turn on EXIT_THROWS_EXCEPTION by default 
related to 0000226new  Feature Request: catch option-system throws locally; handle them somewhat more fancily 
Issue History
2013-01-04 10:55smlewisNew Issue
2013-01-04 10:57smlewisCommand Line Usedex: fixbb -thisisnotanoption => ex: AnchoredDesign -thisisnotanoption
2013-01-04 10:57smlewisAdditional Information Updatedbug_revision_view_page.php?rev_id=47#r47
2013-01-04 11:03momearaNote Added: 0000134
2013-01-04 11:06momearaNote Edited: 0000134bug_revision_view_page.php?bugnote_id=134#r49
2013-01-04 11:10smlewisNote Added: 0000135
2013-01-04 11:10smlewisNote Added: 0000136
2013-03-01 10:42smlewisNote Added: 0000148
2013-03-01 10:46smlewisRelationship addedparent of 0000189
2013-03-01 10:49smlewisNote Edited: 0000148bug_revision_view_page.php?bugnote_id=148#r53
2013-03-01 10:52smlewisTag Attached: exception handling
2013-03-01 10:52smlewisTag Attached: Minicon_2013_defaults
2013-03-01 10:52smlewisTag Attached: robust_rosetta
2013-03-04 09:18renfrewNote Added: 0000157
2013-03-04 09:18renfrewAssigned To => renfrew
2013-03-04 09:18renfrewStatusnew => assigned
2013-03-20 10:58renfrewNote Added: 0000190
2013-03-20 11:09smlewisNote Added: 0000191
2013-03-20 11:32smlewisNote Added: 0000192
2013-03-20 11:34smlewisRelationship addedrelated to 0000200
2013-03-20 11:34smlewisNote Added: 0000193
2013-03-20 12:03renfrewNote Added: 0000194
2013-03-20 13:01renfrewNote Added: 0000195
2013-03-20 13:02renfrewNote Edited: 0000195bug_revision_view_page.php?bugnote_id=195#r61
2013-03-20 18:33smlewisNote Added: 0000196
2013-03-21 09:20smlewisNote Added: 0000200
2013-03-21 10:10renfrewNote Added: 0000201
2013-03-21 10:13renfrewNote Added: 0000202
2013-03-21 11:46renfrewNote Added: 0000203
2013-03-21 12:02smlewisFixed in SVN Version => 53112
2013-03-21 12:02smlewisNote Added: 0000204
2013-03-21 12:02smlewisStatusassigned => resolved
2013-03-21 12:02smlewisResolutionopen => fixed
2013-03-21 12:16smlewisRelationship addedrelated to 0000226
2013-03-21 12:18smlewisAssigned Torenfrew => mtyka
2013-03-21 12:18smlewisStatusresolved => assigned
2013-03-21 12:18smlewisNote Added: 0000205
2013-03-21 12:18smlewisStatusassigned => resolved

Notes
(0000134)
momeara   
2013-01-04 11:03   
(edited on: 2013-01-04 11:06)
Steven, why do you think init() shouldn't be wrapped in an app-level try catch block? Since un caught exceptions fail harder than segfaults, it's worth wrapping all code in any main block in a try catch block in all apps. I've wrapped a few applications but I think all of them should be wrapped.

Adding a catch and re-throw around the various init() calls wouldn't hurt, though, so more information can be added to the error message.

(0000135)
smlewis   
2013-01-04 11:10   
If we want all applications (all our code) to forever be wrapped in try/catch blocks, we can, but it's an unenforceable mandate. Each time someone writes a new one they'll have to remember to add in a bunch of copy/pasted try/catch from some other app to do error handling (duplication). Ideally at application level we should have only init and a call to the job distributor; the latter already has appropriate try/catches so only init needs one.

If we want to put try/catch around all of each application, we can, but it seems...unelegant?
(0000136)
smlewis   
2013-01-04 11:10   
PS, this is also going to rosetta-devel
(0000148)
smlewis   
2013-03-01 10:42   
(edited on: 2013-03-01 10:49)
MiniCON 2013 proposed resolution to this bug: A) add try/catch around all executables, possibly as homework at the bootcamp; B) add try/catch for the option system to core::init. If you fix A without B, or B without A, file a bug for the other one please.

(0000157)
renfrew   
2013-03-04 09:18   
The Bonneau lab is going to add try/catch to all apps in a mini action day this week (2013-03-04)
(0000190)
renfrew   
2013-03-20 10:58   
Relating to Note (0000148), why is B required, if all apps have a catch?
(0000191)
smlewis   
2013-03-20 11:09   
1) because if we expect the option system to throw, catching it as close to the option system as possible (with catch code that knows how to handle the option system) is best

2) because someone is going to make a new app without a try/catch wrapper, and catching in core::init will provide a backstop for some of those errors
(0000192)
smlewis   
2013-03-20 11:32   
Having seen your commit log for https://svn.rosettacommons.org/trac/changeset/54403 [^] , I would suggest maybe having the integration test check for a particular type of error via a core::init catcher, instead of looking for an actual uncaught exception via "Abort trap". Either way, this is a 95% solution! Thanks for taking care of it!
(0000193)
smlewis   
2013-03-20 11:34   
A core::init trap also ties neatly to bug 0000200
(0000194)
renfrew   
2013-03-20 12:03   
Alright, I understand the idea of catching the exception near where it was thrown is elegant and bug 0000200 hints at some useful ways the options system could handle options exceptions:
* if an option cant be found it could do a quick check and see if you misspelled an option and say did mean...
* it could tell you if an option matches multiple things and is therefore ambiguous and you need to precede it with more namespace qualifications
* etc.

To do that it sounds like we would want a new exception class (or possibly several), lets say EXCN_Options. Only the app would know the correct options for it self (if you wanted to do something like bug 0000200) so the options catch in core::init would probably need to re-throw and the app would need to have a special catch for EXCN_Option before the catch for EXCN_Base.

As for (2) we added a new integration test that tests everything in bin (whether or not the new app has its own integration test) by passing a bogus option.
(0000195)
renfrew   
2013-03-20 13:01   
(edited on: 2013-03-20 13:02)
I guess I am confused as to what you mean by having the IT check for different types of exceptions from core::init(). Do you mean having something like below in core::init()

try {
...
catch( utility::excn::EXCN_Msg_Exception const & e ) {
   std::cout << "caught message exception" << std::endl;
} catch( utility::excn::EXCN_BadInput const & e ) {
   std::cout << "caught bad input exception" << std::endl;
} catch( utility::excn::EXCN_RangeError const & e ) {
   std::cout << "caught bad range error exception" << std::endl;
} catch( utility::excn::EXCN_NullPointer const & e ) {
   std::cout << "caught null pointer exception" << std::endl;
}
...

and then having the IT look for "caught message exception", "caught bad input exception", "caught bad range error exception", or "caught null pointer exception"?

I think wrapping core::init() in a try/catch (internally) would only catch throws that happen during core::init().

Do we think that with these changes (all apps being wrapped with try/catch + the new IT) we can close this bug and remove the #ifdef EXIT_THROW_EXCEPTION tags, ( and close bug 0000189)?

We tested r54403 with that flag turned on and saw no problems.

(0000196)
smlewis   
2013-03-20 18:33   
By "I would suggest maybe having the integration test check for a particular type of error via a core::init catcher", what I meant was that the option-system catcher, living inside core::init and catching only option-system throws (and the rare misc. throw that occurs during option time?), would do something slightly fancier than the standard "print the .msg() string", and instead "print that this is a bad option error, then print the .msg() string".

I didn't think it through, though - this won't work with your desired integration test setup unless the core::init catcher re-throws, which is stupid...so I guess for your integration test to work, (unless there is a better way of forcing a throw than a bad option read), we can't add a core::init option catcher.

I guess we could put code in core::init that throws only in the presence of a flag "-always_die", and then run your integration test in the presence of that to see uncaught exceptions, instead of relying on the option system to throw?
(0000200)
smlewis   
2013-03-21 09:20   
Matt points out that the obvious thing to do, to fix bug 200 and keep this new itest working, is to just have a core::init catcher simply re-throw the option. I was vaguely (and incorrectly) under the impression that we disallowed re-throwing.
(0000201)
renfrew   
2013-03-21 10:10   
I think having the options system throw EXCN_Options and catch EXCN_Options (after we make a EXCN_Options class that is) and re-throwing is a good idea.

The catcher in core::init() could be set up to only catch EXCN_Option. Any other exception type would actually bypass that catch passed up until caught at a place that could handle them (with the catch in the app being a catch all). Apps that want to specifically catch the EXCN_Option could do so before they catch the EXCN_Base, and do their own processing since they will know what options work for them. With respect to bug 200, its probably a good idea for the options catch in core::init() to rethrow, regardless of the new IT, so the app can handle it if it wants to.

It might be good policy for catchers (with the exception of the catch in the app) to first modify the message and then rethrow. This way we will avoid lots of double output where the core::ini() options catcher ouputs a message and then rethrows and then the app base catcher outputs the message again.
(0000202)
renfrew   
2013-03-21 10:13   
Although there are definitely cases (like with utility exit) where we have failed, and just want to stop and exit when we catch them.
(0000203)
renfrew   
2013-03-21 11:46   
Also, I just noticed, that there is already a try/catch around core::init() that catches EXCN_Msg_Exception, prints the msg and re-throws.

https://svn.rosettacommons.org/trac/browser/trunk/rosetta/rosetta_source/src/core/init.cc#L1019 [^]

I think this bug can be closed. Perhaps though another bug should be filed, to have the options system throw a specific type of exception that can be caught and handled rather that just rethrown.
(0000204)
smlewis   
2013-03-21 12:02   
Mike Tyka fixed this in 53112 and didn't tell anyone!
(0000205)
smlewis   
2013-03-21 12:18   
(re-resolving after reassignment)