ASIO integration

[cstratopoulos]

@Naios

Here is the preliminary PR for #27 . Opening it now so we can discuss/improve with code review features, but it is not ready to merge. The main blockers are

• the release of Boost 1.72
• the release of standalone ASIO 1.15 (I believe)

This is required of course for the new form of async_result, but also to get rid of a bug workaround for MSVC detection of auto return type support (workaround here, bug report here)

However there are other things I wanted to flag for your approval or discussion, mostly related to error handling.

1. Note the current example calls std::make_exception_ptr(asio::error_code). From the std::make_exception_ptr documentation, this suggests that such errors will only be caught in a .fail handler if the user does catch(asio::error_code const&). However I think this is not standard, as the usual approach for moving from error codes to exceptions would be to throw/catch an asio::system_error constructed from an asio::error_code. If my assessment is correct I can amend the line linked above to do make_exception_ptr(system_error(e)). This is also why I added the cancelled_async_wait example, to be sure exceptions can be caught as expected.

2. Consequently from 1., the distribution-specific detail code has some logic to establish the pair exception_t and error_code_t such that the former is constructible from the latter. This is a divergence from our discussion in #27 where we thought all mention of the error_code type could be elided by having a lambda handler with an auto ec parameter. Since we're already doing this, I used the error_code_t in the continuable_result<Signature> specializations. I think this could be done with specializations for template<typename Error, typename T>, but this is a bit cleaner and clearer in my opinion. a) This is also why I put use_cti_t back in the ASIO namespace, since it fits the convention of other ASIO helper tokens (e.g., use_future_t), and we have to muck around in the ASIO namespace anyway. But if you like I can just make it cti::asio_token_t or similar!

3. Currently the code only works with exception handling enabled. The standard default for no exceptions is an std::error_condition, which will work in the standalone ASIO case (where the error types alias the corresponding std types), but not with Boost, which uses Boost.System. I think we can accommodate a couple options. If exceptions are enabled, this fits with the ASIO use_future_t error handling mechanism for std::future which stores exceptions with std::promise::set_exception(std::exception_ptr). Also, users may want to set CONTINUABLE_WITH_CUSTOM_ERROR_TYPE to asio::error_code or boost::system::error_code, which would also be in line with idiomatic ASIO error handling. Finally, users may have the error type set to std::error_condition either by disabling continuable's exceptions, with a custom error type, or they may set it to boost::system::error_condition. That one feels maybe less likely/more awkward to me, but it's possible. Anyway to sketch a solution,

   
   // distribution-specific aliases
   using error_code_t = ...
   using error_cond_t = ...
   using exception_t = ...

template auto promise_fail_helper(Promise& p, error_code_t ec) -> decltype(p.set_exception(ec)) { p.set_exception(ec); }

template auto promise_fail_helper(Promise& p, error_code_t ec) -> decltype(p.set_exception(error_cond_t(ec.value(), ec.category())) { p.set_exception(error_cond_t(ec.value(), ec.category())) }

template auto promise_fail_helper(Promise& p, error_code_t ec) -> decltype(p.set_exception(std::make_exception_ptr(exception_t(ec)))) { p.set_exception(std::make_exception_ptr(exception_t(ec))); }

to respect the setting of `CONTINUABLE_WITH_NO_EXCEPTIONS`, the `exception_t` alias and overloads may be guarded with an `#ifdef`. And since `promise_base::set_exception` only accepts `cti::exception_t`, I think there is no overload ambiguity. Anyway let me know your thoughts and I can pursue the sketch solution above if it sounds reasonable to you. 

### Check lists (check `x` in `[ ]` of list items)

- [ ] Additional Unit Tests were added that test the feature or regression
- [X] Coding style (Clang format was applied)

I applied clang-format while editing the code. As for unit tests, the added example tries to test the robustness of completion handler signatures ( `void(error_code)`, `void(error_code, T)`) and correctness of error handling. This could be added to a unit test with asserts, but I am not sure if this is an approach you want to take. 

[cstratopoulos]

Thanks for the feedback! I will respond to your code review comments inline/push changes as needed.

As for the compatibility question

In the ideal case we could provide the non zero abstraction implementation for async_result for older boost asio versions and switch to the improved one when a suficient version number is detected (which can be done through ASIO_VERSION or BOOST_VERSION macros which should be exposed somehow by the implementation). Same gos for the MSVC bug you mentioned, is this possible?

That's a good idea so some points are probably worth clarifying.

Re: non-zero overhead version: This is doable and probably worth pursuing. However there are three possible tiers of integration.

1. Best case: ASIO >= 1.15, Boost >= 1.72, zero-overhead adaptation. We have the lazy form async_result::initiate plus return type deduction

2. Not bad: ASIO >= 1.14, Boost >= 1.70, adaptation with type erasure on the continuable. We still have the async_result::initiate which allows lazy initiation of async ops, but return_type must be specified purely based on signature. So we can do return_type = cti::continuable<_result_type_> which, if I understand right, uses your function2 type erasure. Still not bad!

3. Worst case: prior versions. Have to specify return_type, and only the async_result::get form exists, which requires eager initiation.

If I'm reading the invocation model docs correctly, case 3 is incompatible with the continuable invocation model, as it requires eager initiation. For example I am not doing this in my example code currently, but I could write auto ct = asio::async_meow(use_cti_t); And then nothing is actually initiated until I call ct.done() or ct is destructed, etc (possibly with some then/fail chained on beforehand). But strictly speaking this is illegal with the async_result::get model.

So if I'm understanding right, we want to provide support for both 1. and 2., but for 3. the recommendation remains to use promisify and/or follow the example of the existing example-asio.cpp? In this case we could even add macro checking to support/asio.hpp and say older versions are not supported.

Regarding the MSVC bug, this is indeed possible and trivial to do but I would argue not necessary. ASIO and Boost.ASIO both follow a release schedule with discrete releases, so I think we will say something like "here is ASIO support for ASIO >= 1.14 or Boost >= 1.70". But right now I am coding against some stray commits from the Boost 1.72/ASIO 1.15 beta, but I do expect the MSVC fix to make it into the actual 1.72/1.15. Again this is low effort to support, but arguably not really necessary as I imagine users tend to lock into release versions rather than random commits.

Those are my thoughts, but happy to go with whatever you see fit!

[Naios]

Yes, I think you are on a good way there.

You are right:

Worst case: prior versions. Have to specify return_type, and only the async_result::get form exists, which requires eager initiation.

This should not be supported by this library and is also out of scope for this PR since for that there is the promisify helper provided already. I'm looking forward to your changes.

[cstratopoulos]

I've addressed some of the changes you requested, others still in progress. I'll leave you to mark issues as resolved as you see fit.

In particular I wanted to get feedback on the new type deduction approach. I think this is kind of nice as we only need one non-trivial struct template specialization. This works as before on my machine with the previous examples, both in a vanilla configuration (ASIO version is 1.14, no deduced ret type) and with my manual check (I changed ASIO_VERSION to be 1.15, use deduced ret type).

I can also hopefully write a simple example of a novel composed operation to test the new signature with multiple args is handled correctly.

Let me know your thoughts on the CTI_DETAIL_ASIO_HAS macros related to ASIO integration, definitely open to naming convention changes.

Also you will note a bit of verbosity with the #if defined checks. An alternative is to do like ASIO here and define something like CTI_DETAIL_ASIO_INTEGRATION_DECL(...) which pastes its args iff we need explicit ret type. Thus we could use the macro to conditionally provide using return_type = ..., etc.

Similarly we could do CTI_DETAIL_ASIO_INTEGRATION_RET_TYPE(type) which evaluates to type if it's needed, else auto.

I think it may be a tossup--BOOST_ASIO_INITFN_AUTO_RESULT_TYPE appears many times throughout the codebase but we're only using these rules like 1-3 times.

[Naios]

The PR has made a huge step forward, good job so far!

Beside the comments I've made I've spotted two major points:

1) Conversion between `boost::error_code and cti::exception_t

Is it always given that for the 2x2 matrice of possible 4 combinations between asio/boost::asio and no/with exceptions the conversion between boost::error_code and cti::exception_t is correct and meaningful?

	With exceptions	No exceptions
standalone asio
boost asio

2) Specialization against void signatures

The specialization of initiate_make_continuable and its usages can be shortened a lot as described above. Probably CTI_DETAIL_ASIO_INTEGRATION_RET_TYPE can be dropped completely too, in order to address your question from above.

All in all the fallback from previous boost versions looks great to me and blends in quite nicely!

[cstratopoulos]

Thanks for the pointers/encouragement!

Regarding point 1), this comment https://github.com/Naios/continuable/pull/28#discussion_r356044790 has lead me to an answer.

I think the answer is yes, and also that the case analysis may be simpler than I had initially expected. I was accounting for the possibility that a user had disabled exceptions and set boost::system::error_condition as the custom error type, which personally I would be very surprised to see in the wild.

So to address the matrix entries. In all cases assuming a handler call like h(ec, args...) 1.a) standalone ASIO with exceptions: we std::rethrow_exception(std::make_exception_ptr(exception_t(ec))) and catch(exception_t const&). This is meaningful and indeed standard practice, see e.g., the ASIO use_future_t or use_awaitable (and co_spawn).

1.b) standalone ASIO, no exceptions: we are converting an std::error_code to an std::error_condition (i.e., cti::exception_t with exceptions disabled) which is indeed a well-defined (but lossy, see below) conversion.

2. a) Boost.ASIO with exceptions: as in 1.a) but of course exception_t and error_code_t are their Boost counterparts; again standard practice.

2.b) Also the same, and due to the implicit conversions I discovered above, we may convert boost::error_code_t to cti::exception_t (i.e., std::error_condition) with no intermediate conversions.

Setting CONTINUABLE_WITH_CUSTOM_ERROR_TYPE to error_code_t also strikes me as a very probable use case for someone using ASIO with continuable, and is easy to support.

This may be better suited to a separate issue (please create a new one if you'd like to discuss it further), but I am curious about the rationale error_condition as the fallback error type for exceptions disabled. In abstract terms, error_code represents a particular exception type and error_condition represents a base class exception you'd derive from. For a concrete example, I might have error_codes like err::missing_colon or err::unmatched_paren, and those could be mapped to an error_condition like cond::parse_error, so I can do if (ec_val == errc::missing_colon) in the particular case or if (ec_val == cond::parse_error) for a more coarse check. So to answer your question above, the mapping is meaningful in all cases, but it is lossy in most cases too.

edit: looking more closely at the documentation of error_condition, I was wrong to call the conversion lossy as the original error_code can be recovered from an error_condition, but I think it is accurate still to say the goal of error_condition is more coarse error handling at the level of, say, family of errors, rather than a particular code.

Regarding point 2) I think you are right, you have given me some helpful leads to try out and I will report back accordingly!

[cstratopoulos]

I've addressed most of your comments at this point I think! Some questions:

1. Thoughts on the name cti::asio_token_t, and also if cti is an appropriate namespace--at a glance I did not see other non-detail entities which were in further nested namespaces.
2. Now that the token lives in namespace cti, maybe we can broach the topic of doxygen documentation comparable to that of cti::promisify? I may need some guidance on what modules or groups to put stuff in but I am happy to write the docs themselves

[Naios]

1

In my opinion the equivalent to boost::use_future_t would be cti::use_continuable_t and maybe we can also have the docs in the same section as promisify since it is meant for facilities which make other libraries compatible and easy to use with continuable.

2

std::rethrow_exception(std::make_exception_ptr(exception_t(ec)))

Is a std:.exception_ptr not std::exception which means the conversion should look like the following: std::rethrow_exception(std::make_exception_ptr(ec))

The other conversions look good to me.

[Naios]

@cstratopoulos any news?

[cstratopoulos]

@Naios have just been taking a bit of a break the last week or so, should be able to get back to it tomorrow :)

1. sounds good!

2. Not quite, an exception_ptr can point to (and even prolong the lifetime of) anything that was thrown with a throw statement, so it’s necessary to convert to an std::exception (sub) object if it’s to be rethrown and caught as such