Open tgockel opened 5 years ago
Hey,
We would also like to test boost::future
for zk::future
implementation and I was poking around looking how difficult it is to add support for it.
Unfortunately boost's exception handling in promises/futures is somewhat different than std's, which causes exceptions to be thrown with wrong types when boost::current_exception()
is used.
I was looking at boost::enable_current_exception()
which might be a solution (although not the prettiest), but it derives from exception type and all error types are declared final
.
So I wanted to ask, is there any reason to mark classes final
?
I use final
to mark that either (1) the class shouldn't be derived from because it would break something or (2) I haven't thought about what deriving means yet. I suspect the exception classes are an example of case 2, as there usually isn't a problem with doing that.
IIRC, Boost's implementation of this stuff is supposed to resemble the Standard Library fairly closely. What version of Boost are you using that you see the problems?
What version of Boost are you using that you see the problems?
I use version from ubuntu 18.04 repos (which is 1.65.1), and I believe I did a quick test with debian 10 (1.67)
On the other hand they do mention this behaviour in their documentation
Correct implementation of current_exception may require compiler support, unless enable_current_exception was used at the time the currently handled exception object was passed to throw. Whenever current_exception fails to properly copy the current exception object, it returns an exception_ptr to an object of type that is as close as possible to the original exception type, using unknown_exception as a final fallback.
I am not sure which compilers do implement what is needed, but gcc 7.4.0 from ubuntu 18.04 repos did throw mentioned unknown_exception
. This, of course, might mean, that some compiler configuration is required, but I was unable to find something related to this.
Bit of guesswork here: Boost's "as close as possible" might be pretty bad in this situation because the exception classes are marked final
.
The suggestion on that to be to see if removing final
from the exception types makes boost::current_exception
just work (sorry if that was unclear).
While I was trying to understand this issue, I have written myself a quick, small program to test boost exception handling. Notice struct my_error
does not have final
specifier.
I tested that on ubuntu:18.04
and debian:10
docker containers with installed build-essential
and libboost-all-dev
packages, and compiled with:
g++ -std=c++17 boostExceptions.cpp -lboost_system -lboost_thread -pthread
I got output:
root@c4704b9302a9:~/source/test# ./a.out
WRONG (boost::unknown_exception)
Throw location unknown (consider using BOOST_THROW_EXCEPTION)
Dynamic exception type: boost::exception_detail::clone_impl<boost::unknown_exception>
std::exception::what: std::exception
So it seems like final
is not the culprit here.
That's a good, simple test case with a rather surprising output...I suspect my understanding of boost::current_exception
is off. Even simpler version has the same output:
#include <iostream>
#include <boost/thread/future.hpp>
#include <boost/throw_exception.hpp>
struct my_error {};
int main(void)
{
boost::exception_ptr ex_ptr;
try
{
throw my_error{};
}
catch (...)
{
ex_ptr = boost::current_exception();
}
try
{
boost::rethrow_exception(ex_ptr);
}
catch (const my_error & e)
{
std::cout << "CORRECT" << std::endl;
}
catch (boost::unknown_exception & e)
{
std::cout << "WRONG (boost::unknown_exception)" << std::endl;
std::cout << boost::diagnostic_information(e);
}
return 0;
}
Perhaps it needs boost::enable_current_exception
to be used in order to work. I'm a bit concerned over this, as cases where zkpp would call some other code that doesn't use boost::enable_current_exception
.
So I decided to dig a little bit deeper.
When boost::current_exception()
is called, internally it maps to boost::current_exception_impl(), which in turn calls boost::exception_detail::clone_current_exception() this function returns exception_detail::clone_current_exception_result::not_supported
(more on this later).
When not_supported
is returned from clone_current_exception()
it does still fallback to handle most of (maybe all of) the std
exception types and some boost types. But in our case - none of the handlers managed to pickup the exception, so catch-all case, which maps to boost::unknown_exception
takes over.
Looking at declaration of boost::exception_detail::clone_current_exception() it is obvious, that without BOOST_ENABLE_NON_INTRUSIVE_EXCEPTION_PTR
define previously mentioned not_supported
is returned always. I did miss this define, but, unfortunately, after adding it the behaviour did not change. Looking at the implementation of the boost::exception_detail::clone_current_exception() it turns out, that when internal variable exception_info_offset
is negative, the same not_supported
is returned always. And looking at its definition it turns out, that the only compiler which is in fact supported is MSVC :( .
When boost::enable_current_exception() is used we can see that the type is wrapped in exception_detail::clone_impl
, which is one of the types handled in boost::current_exception_impl()
when not_supported
is returned by boost::exception_detail::clone_current_exception()
.
So it seems that, for gcc or clang or any other non-msvc compiler boost::enable_current_exception()
is required. Good news (which was worrying me before), that std
exceptions are handled correctly. And I did test this, it worked.
I'm a bit concerned over this, as cases where zkpp would call some other code that doesn't use boost::enable_current_exception.
Correct me if I am wrong, but it seems like apart from std
, zkpp
does not use any other libraries with their own exception types.
There should be a way to use
boost::future<T>
andboost::promise<T>
forzk::future<T>
andzk::promise<T>
. Something along the lines ofcmake -DZKPP_BUILD_SETTING_FUTURE=BOOST_THREAD ..
. While we're here, better documentation for how to use alternative future implementations.This will help give a better answer to #107.