0000168: Bad option reads throw uncaught exceptions

Notes
(0000134) momeara (Attentive Developer) 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 (Administrator) 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 (Administrator) 2013-01-04 11:10	PS, this is also going to rosetta-devel

(0000148) smlewis (Administrator) 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 (Developer) 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 (Developer) 2013-03-20 10:58	Relating to Note (0000148), why is B required, if all apps have a catch?

(0000191) smlewis (Administrator) 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 (Administrator) 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 (Administrator) 2013-03-20 11:34	A core::init trap also ties neatly to bug 0000200

(0000194) renfrew (Developer) 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 (Developer) 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 (Administrator) 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 (Administrator) 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 (Developer) 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 (Developer) 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 (Developer) 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 (Administrator) 2013-03-21 12:02	Mike Tyka fixed this in 53112 and didn't tell anyone!

(0000205) smlewis (Administrator) 2013-03-21 12:18	(re-resolving after reassignment)

Issue History
Date Modified	Username	Field	Change
2013-01-04 10:55	smlewis	New Issue
2013-01-04 10:57	smlewis	Command Line Used	ex: fixbb -thisisnotanoption => ex: AnchoredDesign -thisisnotanoption
2013-01-04 10:57	smlewis	Additional Information Updated	View Revisions
2013-01-04 11:03	momeara	Note Added: 0000134
2013-01-04 11:06	momeara	Note Edited: 0000134	View Revisions
2013-01-04 11:10	smlewis	Note Added: 0000135
2013-01-04 11:10	smlewis	Note Added: 0000136
2013-03-01 10:42	smlewis	Note Added: 0000148
2013-03-01 10:46	smlewis	Relationship added	parent of 0000189
2013-03-01 10:49	smlewis	Note Edited: 0000148	View Revisions
2013-03-01 10:52	smlewis	Tag Attached: exception handling
2013-03-01 10:52	smlewis	Tag Attached: Minicon_2013_defaults
2013-03-01 10:52	smlewis	Tag Attached: robust_rosetta
2013-03-04 09:18	renfrew	Note Added: 0000157
2013-03-04 09:18	renfrew	Assigned To	=> renfrew
2013-03-04 09:18	renfrew	Status	new => assigned
2013-03-20 10:58	renfrew	Note Added: 0000190
2013-03-20 11:09	smlewis	Note Added: 0000191
2013-03-20 11:32	smlewis	Note Added: 0000192
2013-03-20 11:34	smlewis	Relationship added	related to 0000200
2013-03-20 11:34	smlewis	Note Added: 0000193
2013-03-20 12:03	renfrew	Note Added: 0000194
2013-03-20 13:01	renfrew	Note Added: 0000195
2013-03-20 13:02	renfrew	Note Edited: 0000195	View Revisions
2013-03-20 18:33	smlewis	Note Added: 0000196
2013-03-21 09:20	smlewis	Note Added: 0000200
2013-03-21 10:10	renfrew	Note Added: 0000201
2013-03-21 10:13	renfrew	Note Added: 0000202
2013-03-21 11:46	renfrew	Note Added: 0000203
2013-03-21 12:02	smlewis	Fixed in SVN Version	=> 53112
2013-03-21 12:02	smlewis	Note Added: 0000204
2013-03-21 12:02	smlewis	Status	assigned => resolved
2013-03-21 12:02	smlewis	Resolution	open => fixed
2013-03-21 12:16	smlewis	Relationship added	related to 0000226
2013-03-21 12:18	smlewis	Assigned To	renfrew => mtyka
2013-03-21 12:18	smlewis	Status	resolved => assigned
2013-03-21 12:18	smlewis	Note Added: 0000205
2013-03-21 12:18	smlewis	Status	assigned => resolved