Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Boost Future #108

Open
tgockel opened this issue Jul 1, 2019 · 8 comments
Open

Support for Boost Future #108

tgockel opened this issue Jul 1, 2019 · 8 comments

Comments

@tgockel
Copy link
Owner

tgockel commented Jul 1, 2019

There should be a way to use boost::future<T> and boost::promise<T> for zk::future<T> and zk::promise<T>. Something along the lines of cmake -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.

@Jauler
Copy link
Contributor

Jauler commented Sep 24, 2019

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?

@tgockel
Copy link
Owner Author

tgockel commented Sep 24, 2019

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?

@Jauler
Copy link
Contributor

Jauler commented Sep 24, 2019

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.

@tgockel
Copy link
Owner Author

tgockel commented Sep 24, 2019

Bit of guesswork here: Boost's "as close as possible" might be pretty bad in this situation because the exception classes are marked final.

@tgockel
Copy link
Owner Author

tgockel commented Sep 24, 2019

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).

@Jauler
Copy link
Contributor

Jauler commented Sep 24, 2019

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.

@tgockel
Copy link
Owner Author

tgockel commented Sep 24, 2019

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.

@Jauler
Copy link
Contributor

Jauler commented Sep 24, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants