[exception] Virtual inheritance with no default constructors
The Boost.Exception library mandates that exceptions classes hierarchy should use virtual inheritance (http://www.boost.org/doc/libs/1_39_0/libs/exception/doc/using_virtual_inheri...). The document notes that this does not go well with exceptions without default constructors ("(...) Note that virtual bases are initialized directly by the constructor of the most-derived-type (...)") however this should not be a concern as boost::exception provides other means to transport data. What is missing is however documentation that in normal circumstances boost::throw_exception cannot be used with exceptions types which have in their inheritance graph virtual base classes which are not default constructible. This is because boost::enable_current_exception constructs the clone_impl object which inherits the actual exception type and copy-constructs it. However this code will not compile if some base class of the callers exception type is virtual and non-default constructible. In my opinion few changes seem appropriate: 1) Note in the documentation explicitly this requirement (possibility of default construction). 2) Name an entity which should be made friend of exception class to use its non-public default constructor. This entity would be latter used by clone_impl. This way an exception could be default constructible only for this particular use case. 3) Provide means to late-initialize exception object. This would bypass the default construction problem (and goes well with private default constructor as described in 2)). 4) Allow to specify in boost::throw_exception (or similar places) not only the exception type but as well its (virtual) base types so that latter clone_impl could mention them on the initializers list as well and copy-construct them. (As far as I know there is no way of automatic detection of base types thus caller must specify them explicitly - however if I am mistaken such automation would be even better.) Adam Badura
On Tue, May 12, 2009 at 12:21 AM, Adam Badura <abadura@o2.pl> wrote:
The Boost.Exception library mandates that exceptions classes hierarchy should use virtual inheritance <snip> What is missing is however documentation that in normal circumstances boost::throw_exception cannot be used with exceptions types which have in their inheritance graph virtual base classes which are not default constructible.
This is because boost::enable_current_exception constructs the clone_impl object which inherits the actual exception type and copy-constructs it. However this code will not compile if some base class of the callers exception type is virtual and non-default constructible.
Good point. Definitely, the documentation needs to be updated to note the problem. In general, a good guideline to follow when using virtual inheritance is to always provide a default constructor for the virtual bases. I came up with a solution which would involve renaming boost::exception_detail::clone_base to boost::exception_clone_base, documenting its requirements, and modifying boost::enable_current_exception to be able to detect if the type of its argument derives from boost::exception_clone_base. In that case, it wouldn't wrap it, and thus it is up to the user-defined exception type to initialize any virtual bases, including the ones that can't be default-initialized. See the attached patch -- I've only tested with MSVC but it should work with other compilers too. I'm not committing this to trunk yet. It seems acceptable, but I'm not sure if this extra complication is worth the trouble. I doubt that user code that requires a similar workaround exists in practice. Was your observation mostly theoretical, or was it based on trouble you had with existing code? Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
I came up with a solution which would involve renaming boost::exception_detail::clone_base to boost::exception_clone_base, documenting its requirements, and modifying boost::enable_current_exception to be able to detect if the type of its argument derives from boost::exception_clone_base. In that case, it wouldn't wrap it, and thus it is up to the user-defined exception type to initialize any virtual bases, including the ones that can't be default-initialized.
Good idea. Should have thought about it myself. Somehow I failed to do this.
See the attached patch -- I've only tested with MSVC but it should work with other compilers too. I'm not committing this to trunk yet. It seems acceptable, but I'm not sure if this extra complication is worth the trouble. I doubt that user code that requires a similar workaround exists in practice.
How to apply the patch to normally downloaded Boost? We use an official release (1.39) which we build ourselves. And the use it. Is is enough to replace the original files with those in the patch? We work on MS VC as well (2005 with SP). I haven't seen yet the changes but are they so complicated that you have to think whether they are worth applying?
Was your observation mostly theoretical, or was it based on trouble you had with existing code?
Currently I am introducing exception handling into already existing project (rather large but not extremely large). The project used exceptions before but we wanted to establish a common philosophy and policies for throwing, catching handling and so on, refractor the code appropriately and provide documentation for all this. So I started playing with the task. I decided to use Boost.Exception. The arbitrary data injection was what convinced me (however I am somewhat afraid catastrophic cases when std::bad_alloc is thrown during data injection and original exception is lost). One of exceptions I defined was WinAPIException which was constructible from DWORD error code. I wanted it to return the stored error code and return retrieved error message upon request basing on the error code. This cannot be provided without initializing the stored error code and thus my post. The solutions I had were: 1) To inherit from WinAPIException non-virtually. This seemed rather safe as I suspected that likelihood that a class will in result inherit from WinAPIException more then once is rather low and the base classes like std::exception and boost::exception would be inherited virtually anyway. But this seemed inconsistent and does not go well with other components. I had for example CombinedException which took as template parameters some exception classes and inherit from them all. This was useful since I had clear separation on logic errors and runtime errors (expressed in appropriate base class) and sometimes it was not known for an exception class whether it will be thrown in logic error or runtime error until it was actually thrown. The WinAPIException is a good example. It might be due to an WinAPI error like ERROR_INVALID_HANDLE which is (usually) due to a logic error or a ERROR_FILE_NOT_FOUND which is (usually) due to a runtime error. This could be decided only in place the exception is thrown so I had something like: if ( errorCode == ERROR_INVALID_HANDLE ) throw CombinedException< MyComponentException, WinAPIException, LogicException >( WinAPIException( errorCode ) ); else if ( errorCode == ERROR_FILE_NOT_FOUND ) throw CombinedException< MyComponentException, WinAPIException, IOException, RuntimeException >( WinAPIException( errorCode ) ); else .... (The code skips additional staff like using boost::throw_exception, adding function/file name/line number data and so on...) The CombinedException class is build using Boost.Preprocessor and it can copy-construct any subset of base classes (the caller knows which subset is required - in this case only WinAPIException). It saves us from defining an enormous number of exception classes or reducing type information (tagging) of the thrown exceptions. And to sum up having to inherit from WinAPIException non-virtually makes the CombinedException more complicated as it has to somehow know which classes are to be inherited virtually and which not. 2) Skip having non-trivial WinAPIException and use the Boost.Exception data injection alone. This makes however throwing much more complicated. How can I be sure that thrower will in fact inject the error code? I cannot. As I don't like trusting and hoping for the caller to remember to do the right thing I would have to provide some functions/macros to do that. This again makes it a little bit more complicated. Also this does not go well with legacy code/third party code/different policy code which might use non-trivial exceptions types. We would not be able to use them in such case. And I don't like when some policy does not give options. However note that I still experiment with the code trying to establish the policy. I still have to answer some questions. General requirements are to (random order): 1) Make declaring exceptions classes simple and short when no additional data is required. Empty classes (with base classes) are great here and data injection goes well with it. 2) Make throwing expressions short and simple. Here data injection and composites like the mentioned CombinedException are not good at all (just look at the above example code!). 3) Support exception wrapping on the module boundary (so that module has to care only for exceptions of the modules it uses directly) - and this I still don't know how to do as I have no good ideas for wrapping which will not loose a lot of data on the original exception. 4) Go well with somewhat different approaches to exceptions from "read-only modules". 5) Provide as much diagnostic data as possible. Currently it seems that we will almost never use any data in exceptions for other then diagnostic reasons. But that still has to be investigated. Adam Badura
On Wed, May 13, 2009 at 1:32 AM, Adam Badura <abadura@o2.pl> wrote:
I came up with a solution which would involve renaming boost::exception_detail::clone_base to boost::exception_clone_base, documenting its requirements, and modifying boost::enable_current_exception to be able to detect if the type of its argument derives from boost::exception_clone_base. In that case, it wouldn't wrap it, and thus it is up to the user-defined exception type to initialize any virtual bases, including the ones that can't be default-initialized.
Good idea. Should have thought about it myself. Somehow I failed to do this.
See the attached patch -- I've only tested with MSVC but it should work with other compilers too. I'm not committing this to trunk yet. It seems acceptable, but I'm not sure if this extra complication is worth the trouble. I doubt that user code that requires a similar workaround exists in practice.
How to apply the patch to normally downloaded Boost? We use an official release (1.39) which we build ourselves.
I think you can use http://gnuwin32.sourceforge.net/packages/patch.htm to apply patch files on Windows.
I haven't seen yet the changes but are they so complicated that you have to think whether they are worth applying?
Not complicated, but it seems to me that it is unlikely that anyone would need these changes in practice and if that's the case I'd rather not commit them. Consider the downsides of having to document the clone_base type; after all, once compilers start supporting exception_ptr natively, all of these concerns will disappear.
Was your observation mostly theoretical, or was it based on trouble you had with existing code?
One of exceptions I defined was WinAPIException which was constructible from DWORD error code. I wanted it to return the stored error code and return retrieved error message upon request basing on the error code. This cannot be provided without initializing the stored error code and thus my post.
Here's how I handle this type of errors -- an example of calling TerminateProcess: struct exception_base: virtual std::exception, virtual boost::exception { }; struct win32_error: virtual exception_base { }; typedef boost::error_info<struct tag_win_last_error,unsigned> win32_last_error; typedef boost::error_info<struct tag_c_function,char const *> c_function; .... if( !TerminateProcess(hProcess,uExitCode) ) BOOST_THROW_EXCEPTION( win32_error() << win32_last_error(GetLastError()) << c_function("TerminateProcess")); A main advantage of Boost Exception is that your exception types don't have to have any data members or constructors; see http://www.boost.org/doc/libs/release/libs/exception/doc/exception_types_as_....
Skip having non-trivial WinAPIException and use the Boost.Exception data injection alone. This makes however throwing much more complicated. How can I be sure that thrower will in fact inject the error code? I cannot. As I don't like trusting and hoping for the caller to remember to do the right thing I would have to provide some functions/macros to do that. This again makes it a little bit more complicated.
My approach is to write wrappers for WIN32 (and other C functions) I call, which throw exceptions on failure. So, instead of calling TerminateProcess, I call win32_TerminateProcess, which always throws on error and adds the relevant error code, etc. Your concern about possibly forgetting to add the error code is valid, but if you call C functions directly (without a wrapper to throw on failure) you have the problem that users might forget to check for errors altogether -- and thus forget to throw, which is a bigger problem than forgetting to add the error code to the exception. If you have a few common items that should all be added to a particular class of exceptions, you can use boost::tuple of boost::error_info objects of types that don't have default constructors, and while you can still "forget" to add the tuple itself, you can't forget to add a particular member of the tuple (I think). See http://www.boost.org/doc/libs/release/libs/exception/doc/tuple_operator_shl...., and "Adding Grouped Data to Exceptions" in http://www.boost.org/doc/libs/release/libs/exception/doc/tutorial_transporti....
Also this does not go well with legacy code/third party code/different policy code which might use non-trivial exceptions types. We would not be able to use them in such case. And I don't like when some policy does not give options.
In my experience most legacy code doesn't use virtual inheritance in exception types. If that's the case, I don't see a reason to change it to use virtual inheritance.
3) Support exception wrapping on the module boundary (so that module has to care only for exceptions of the modules it uses directly) - and this I still don't know how to do as I have no good ideas for wrapping which will not loose a lot of data on the original exception.
Wrapping is not a good idea. If a module can handle an exception -- great, if not -- it shouldn't interfere (by changing the original exception's type.)
5) Provide as much diagnostic data as possible. Currently it seems that we will almost never use any data in exceptions for other then diagnostic reasons. But that still has to be investigated.
See http://www.boost.org/doc/libs/release/libs/exception/doc/tutorial_diagnostic.... Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
3) Support exception wrapping on the module boundary (so that module has to care only for exceptions of the modules it uses directly) - and this I still don't know how to do as I have no good ideas for wrapping which will not loose a lot of data on the original exception.
Wrapping is not a good idea. If a module can handle an exception -- great, if not -- it shouldn't interfere (by changing the original exception's type.)
I do not agree. Consider module responsible for providing data to the application, building the "data model". It might fail to load the data because of loss of connection do database for example. Now if your philosophy was used the "view module" would have to catch for example database_error when loading the data model. Everything fine except that if I change the database to a different one the exception thrown by database controller will likely change - after all what are the chances that different vendors use the same class - other then generic std::exception - as base class? And thus the "view module" will have to switch to "database_2_error". But what about changing the "data module" to provide data from a local file? What if it provided data from the local file and database at the same time? Each such change requires to change the "view module" to catch appropriate exception. This just does not seem right. And after all it reveals implementation details of the "data module". The "data module" should rather throw its own exception ("load_error") regardless of the underlying raw data provider (file, database 1, database 2, ...). Then "view module" would be independent on the internals of implementation of the "data module". But that requires "data module" to cache the exceptions of raw data provider and throw load_error instead. This obviously leads to loss of diagnostics data (after all the error may be due to database authorization error and someone has to correct the user/password). That is way I thing that some kind of wrapping/chaining/nesting or how you call it would be useful here. It would make the "view module" independent on the internals of "data module" however full information will be still available and could be for example logged. Or in some weird special cases the "view module" could even investigate the inner exception and deal with it - however it would not have to. I agree that inheriting would be better here. But this in most cases cannot be done dynamically or would lead to enormous large code. If we wanted load_error to inherit from the raw data provider error then we would have to declare base_load_error and then file_load_error, database_1_load_error and so on... However this is not even near the end. This would work only if the raw data provides modules declared exact types of thrown objects and did not abuse that declaration. And then file provider could throw file_does_not_exist, access_denied or file_corrupted, or inherited from file_error then "data module" would have to catch them as well and provide appropriate load_errors (here we start to think about making base_load_error and template_load_error : public base_load_error). Also we would have to require from the exception classes that they were copy-constructible and that such copies are equivalent. This puts a lot of requirements on the providers modules. And adding an additional exception in the lowest module propagates to massive changes in upper modules. That is why I think wrapping/chaining/nesting is better. Currently we can chain exceptions with boost::exception by constructing a new boost::exception and injecting to it error_info<struct nested_exception_tag, exception_ptr> with exception taken by current_exception. However at the catch site there is little we can do with exception_ptr without rethrowing it to catch it and print diagnostic data. How about allowing diagnostic data for exception_ptr? Adam Badura
Also we would have to require from the exception classes that they were copy-constructible and that such copies are equivalent.
This is required anyway. Somehow I forgot that. Sorry. But the other notes I still consider true. Adam Badura
On Wed, May 13, 2009 at 11:26 PM, Adam Badura <abadura@o2.pl> wrote:
Currently we can chain exceptions with boost::exception by constructing a new boost::exception and injecting to it error_info<struct nested_exception_tag, exception_ptr> with exception taken by current_exception. However at the catch site there is little we can do with exception_ptr without rethrowing it to catch it and print diagnostic data. How about allowing diagnostic data for exception_ptr?
If you don't insist on the diagnostic message to be user-friendly, just use current_exception_diagnostic_information: boost::exception_ptr p; ... try { boost::rethrow_exception(p); } catch(...) { std::cerr << boost::current_exception_diagnostic_information(); } You can also catch( boost::exception & ), in which case you can use get_error_info to probe the exception for data. You can't do better than that without *knowing* what the exception is and what it indicates in your particular program. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On Wed, May 13, 2009 at 11:26 PM, Adam Badura <abadura@o2.pl> wrote:
Currently we can chain exceptions with boost::exception by constructing a new boost::exception and injecting to it error_info<struct nested_exception_tag, exception_ptr> with exception taken by current_exception. However at the catch site there is little we can do with exception_ptr without rethrowing it to catch it and print diagnostic data. How about allowing diagnostic data for exception_ptr?
If you don't insist on the diagnostic message to be user-friendly, just use current_exception_diagnostic_information:
That is not the same. With current_exception_diagnostic_information I sure can achieve the desired result but in an ugly way which likely is also inefficient and prone for failure due to another exception being thrown (for example in low memory conditions). In cases I want to wrap just caught exception I inject it into exception object to be thrown as exception_ptr data. This is something like: typedef boost::error_info< struct Tag, boost::exception_ptr > wrapped_exception_ptr; void load_data() { try { // Do internal staff. // Caller should not be interested in the details. } catch ( const implementation_specific_type& ) { BOOST_THROW_EXCEPTION( load_data_error() // << inject some additional information as required << wrapped_exception_ptr( boost::current_exception() ) ); } } and then latter try { load_data() } catch ( const load_data_error& e ) { std::clog << boost::diagnostic_information( e ); } catch ( ... ) { std::cerr << "Fatal error. Abort." << std::endl; std::abort(); } Note that this way, as I already described, while calling load_data I do not have to bother what is its current implementation and what it might throw. I know that it throws load_data_error if it fails but I can continue (however without the data) or anything else in case of catastrophic errors. (The examples might be more complicated.) However I do not loose even thinnest bit of information. Or rather I wouldn't if boost::diagnostic_information was able to deal with error_info of exception_ptr. But it does not. I could simulate this by asking the exception object for wrapped_exception_ptr and if it returns one then rethrowing that exception catching it and doing current_exception_diagnostic_information for it. And likely doing it in loop since the chain of exceptions may be longer. Adding function like std::string to_string( const wrapped_exception_data& _e ) { try { boost::rethrow_exception( _e.value() ); } catch ( ... ) { return boost::current_exception_diagnostic_information(); } return "to_string exiting unexpectedly"; } helps a bit since boost::diagnostic_information will handle now wrapped_exception_data and the "loop" for chained exceptions is achieved automatically. However it still requires to throw and catch. And this does not seem right. (Also note the return value at the end. I had to add it while compiling on MS VS 2005 since is warned about not every control path returning a value. Why rethrow_exception (and likely other similar functions as well) is not marked as no-return? In case of MS VS 2005 it is done quite easy... But if I am recalling well someone already asked for it.) If I am not mistaken it shouldn't be hard to allow boost::diagnostic_information act on exception_ptr like on ordinary exception. And yes. I know you are against chaining exceptions. However I showed some use cases and you haven't showed any other solution to those (which does not mean that non exists). Also note that simply copying data from one exception to the other will not do as it might override some data (like the basic ones: file name, line number and function name). Adam Badura
On Fri, May 15, 2009 at 7:57 AM, Adam Badura <abadura@o2.pl> wrote:
On Wed, May 13, 2009 at 11:26 PM, Adam Badura <abadura@o2.pl> wrote: If I am not mistaken it shouldn't be hard to allow boost::diagnostic_information act on exception_ptr like on ordinary exception.
Ah, yes this makes sense. Thanks for the suggestion, I'll add exception_ptr handling in diagnostic_information.
And yes. I know you are against chaining exceptions. However I showed some use cases and you haven't showed any other solution to those (which does not mean that non exists). Also note that simply copying data from one exception to the other will not do as it might override some data (like the basic ones: file name, line number and function name).
Well, I didn't want this to turn into an argument for or against wrapping, but consider a generic context: void open_file( boost::function<void(char const *)> const & loader, char const * name ) { try { loader(name); } catch( ... ) { throw open_file_error(boost::current_exception()); } } Sure, callers of open_file can now catch open_file_error. And then what? It may contain any exception at all in it. You have the equivalent of a void pointer: it's only useful if you know its original type, so you can "cast" it back to it. How do I deal with this situation *generically*? Obviously, if I have to deal with it specifically, there's no point in wrapping, I'd simply catch the original exception. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On Fri, May 15, 2009 at 7:57 AM, Adam Badura <abadura@o2.pl> wrote:
On Wed, May 13, 2009 at 11:26 PM, Adam Badura <abadura@o2.pl> wrote: helps a bit since boost::diagnostic_information will handle now wrapped_exception_data and the "loop" for chained exceptions is achieved automatically.
This is now implemented, see trunk revision 53038: adding an exception_ptr as error_info to a boost::exception is displayed nicely when the outer exception is passed to diagnostic_information(). There is also a new diagnostic_information overload that takes exception_ptr. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
Will this changes be introduced with Boost 1.40? Adam Badura "Emil Dotchevski" <emildotchevski@gmail.com> wrote in message news:c72e33ce0905151555n47b8fd93h641d8b9e922c188a@mail.gmail.com...
On Fri, May 15, 2009 at 7:57 AM, Adam Badura <abadura@o2.pl> wrote:
On Wed, May 13, 2009 at 11:26 PM, Adam Badura <abadura@o2.pl> wrote: helps a bit since boost::diagnostic_information will handle now wrapped_exception_data and the "loop" for chained exceptions is achieved automatically.
This is now implemented, see trunk revision 53038: adding an exception_ptr as error_info to a boost::exception is displayed nicely when the outer exception is passed to diagnostic_information().
There is also a new diagnostic_information overload that takes exception_ptr.
Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Sat, May 16, 2009 at 1:39 AM, Adam Badura <abadura@o2.pl> wrote:
Will this changes be introduced with Boost 1.40?
Yes. Until then, you can use a SVN client (for example Tortoise SVN) and download it from Trunk: http://svn.boost.org/svn/boost/trunk/ Beware, however, that Trunk is being modified all the time with possibly breaking changes in various libraries. Current Trunk test logs are here: http://www.boost.org/development/tests/trunk/developer/summary.html. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
participants (2)
-
Adam Badura -
Emil Dotchevski