ned14 / outcome

Provides very lightweight outcome<T> and result<T> (non-Boost edition)
https://ned14.github.io/outcome
Other
676 stars 62 forks source link

OUTCOME_TRY for coroutines #199

Closed cstratopoulos closed 4 years ago

cstratopoulos commented 4 years ago

This may be kind of silly but during my work on #198 I found myself wanting a co_return-flavored version of BOOST_OUTCOME_TRY. Do you think it would make sense to add something like that either to try.hpp or the ASIO recipe?

Looking at the implementation, specifically

#define OUTCOME_TRYV2(unique, ...)                                                                                                                                                                                                                                                                                             \
  auto && (unique) = (__VA_ARGS__);                                                                                                                                                                                                                                                                                            \
  if(!OUTCOME_V2_NAMESPACE::try_operation_has_value(unique))                                                                                                                                                                                                                                                                   \
return OUTCOME_V2_NAMESPACE::try_operation_return_as(static_cast<decltype(unique) &&>(unique))

it seems we could do something like

#define OUTCOME_TRYV2(unique, return_keyword, ...)                                                                                                                                                                                                                                                                                             \
  auto && (unique) = (__VA_ARGS__);                                                                                                                                                                                                                                                                                            \
  if(!OUTCOME_V2_NAMESPACE::try_operation_has_value(unique))                                                                                                                                                                                                                                                                   \
return_keyword OUTCOME_V2_NAMESPACE::try_operation_return_as(static_cast<decltype(unique) &&>(unique))

and then generate implementations for return and co_return.

Again this may be a bit silly as it could involve muddying up try.hpp and landing us with some distasteful macro name like OUTCOME_CO_TRY, but I thought I'd mention it all the same.

ned14 commented 4 years ago

I feel an instinctive negative reaction due to all the misuse of await for monads in C++, but can you show me a compelling use case of what an OUTCOME_CO_TRY would look like in real world code?

I mean, is it just a shorthand for:

outcome::outcome<T> foo();
T val = co_await foo();

In this sense it's the same as marking up an optional<T> as an awaitable, and I'd prefer to push that onto users, as awaitable optionals come with unpleasant side effects.

However, you also mentioned a shortcut for co_return, and that might be more compelling. Back to you!

cstratopoulos commented 4 years ago

Oh it's simpler than that even, the monads bit hadn't occurred to me. Here's a before/after with a manually edited try.hpp:

awaitable<result<void>>
MyWebSocketClass::connect(std::string target, std::string port)
{
    result<net::tcp::resolver::results_type> resolve =
        co_await _resolver.async_resolve(_host, port, as_result(use_awaitable));

    if (resolve.has_error()) {
        co_return resolve.assume_error();
    }

    result<net::tcp::resolver::results_type::endpoint_type> connect =
        co_await beast::get_lowest_layer(_ws).async_connect(
            resolve.assume_value(), as_result(use_awaitable));

    if (connect.has_error()) {
        co_return connect.assume_error();
    }

    _ws.set_option(websocket::stream_base::timeout::suggested(beast::role_type::client));

    result<void> handshake =
        co_await _ws.async_handshake(_host, target, as_result(use_awaitable));

    if (handshake.has_error()) {
        co_return handshake.assume_error();
    }

    co_return boost::outcome_v2::success();
}
awaitable<result<void>>
MyWebSocketClass::connect(std::string target, std::string port)
{
    BOOST_OUTCOME_TRY(
        resolve, co_await _resolver.async_resolve(_host, port, as_result(use_awaitable)));

    BOOST_OUTCOME_TRYV(
        co_await beast::get_lowest_layer(_ws).async_connect(resolve, as_result(use_awaitable)));

    _ws.set_option(websocket::stream_base::timeout::suggested(beast::role_type::client));

    BOOST_OUTCOME_TRYV(co_await _ws.async_handshake(_host, target, as_result(use_awaitable)));

    co_return boost::outcome_v2::success();
}
ned14 commented 4 years ago

I find this use case very compelling. Thank you.

I've actually got a fair bit of coroutines stuff from LLFIO that I can patch in here I think. It is, as always, a question of time to get it done. I'm also minded that this is a feature, and Boost is closed to new features right now. But it can still accept docs changes, so I'd focus on your recipe if you want to make Boost 1.71.

Thanks for the idea. Once you see it in code, it's obvious!

akrzemi1 commented 4 years ago

We might want to wait a couple of days with this. P1485r1 is going to be discussed on Tuesday in Cologne. If it is accepted, there will be no co_return anymore. Only return will be used.

ned14 commented 4 years ago

:)

For work, this past week I have been implementing http://www.vldb.org/pvldb/vol11/p1702-jonathan.pdf. Turns out that the clang and MSVC Coroutines implementations are really far behind where the Coroutines TS is, and both implementations differ from one another greatly too, and they don't implement the Core Coroutines customisation points properly. So, I gave up, and fell back to https://github.com/jamboree/co2 which works identically on all platforms and compilers, and can be made into never-allocate-memory easily.

In short, to be honest, anything but toy usage of Coroutines isn't doable for long lived production code for probably another two years. So I feel unconcerned about yet more breaking changes to Coroutines, I can't see anybody caring about code stability using them for years yet to come. Hence, we can change co_return to return whenever.

akrzemi1 commented 4 years ago

Update from Cologne meeting: co_return stays.

ned14 commented 4 years ago

Example implementation taken from #198:

#define BOOST_OUTCOME_CO_TRYV2(unique, ...)                                                                                                                                                                                                                                                                                             \
  auto && (unique) = (__VA_ARGS__);                                                                                                                                                                                                                                                                                            \
  if(!(unique).has_value())                                                                                                                                                                                                                                                                                                    \
  co_return BOOST_OUTCOME_V2_NAMESPACE::try_operation_return_as(static_cast<decltype(unique) &&>(unique))
#define BOOST_OUTCOME_CO_TRY2(unique, v, ...)                                                                                                                                                                                                                                                                                           \
  BOOST_OUTCOME_CO_TRYV2(unique, __VA_ARGS__);                                                                                                                                                                                                                                                                                          \
  auto && (v) = BOOST_OUTCOME_V2_NAMESPACE::detail::try_extract_value(static_cast<decltype(unique) &&>(unique))

#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ >= 8
#pragma GCC diagnostic pop
#endif

#define BOOST_OUTCOME_CO_TRYV(...) BOOST_OUTCOME_CO_TRYV2(BOOST_OUTCOME_TRY_UNIQUE_NAME, __VA_ARGS__)

#define BOOST_OUTCOME_CO_TRYA(v, ...) BOOST_OUTCOME_CO_TRY2(BOOST_OUTCOME_CO_TRY_UNIQUE_NAME, v, __VA_ARGS__)

#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY8(a, b, c, d, e, f, g, h) BOOST_OUTCOME_CO_TRYA(a, b, c, d, e, f, g, h)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY7(a, b, c, d, e, f, g) BOOST_OUTCOME_CO_TRYA(a, b, c, d, e, f, g)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY6(a, b, c, d, e, f) BOOST_OUTCOME_CO_TRYA(a, b, c, d, e, f)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY5(a, b, c, d, e) BOOST_OUTCOME_CO_TRYA(a, b, c, d, e)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY4(a, b, c, d) BOOST_OUTCOME_CO_TRYA(a, b, c, d)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY3(a, b, c) BOOST_OUTCOME_CO_TRYA(a, b, c)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY2(a, b) BOOST_OUTCOME_CO_TRYA(a, b)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY1(a) BOOST_OUTCOME_CO_TRYV(a)

#define BOOST_OUTCOME_CO_TRY(...) BOOST_OUTCOME_TRY_CALL_OVERLOAD(BOOST_OUTCOME_CO_TRY_INVOKE_TRY, __VA_ARGS__)
ned14 commented 4 years ago

Ok, support for C++ Coroutines has been added. Note the additional awaitable<T> and task<T> which should appear shortly in the reference docs. Do please review the implementation code for correctness, you have a much better understanding of this stuff than I do. Thanks in advance.

cstratopoulos commented 4 years ago

Perhaps the comment about implementation review was directed at @akrzemi1 ? I have a bit of experience doing ASIO/Outcome coro stuff in practice but the machinery that goes into actually implementing awaitable types is beyond me :)

I'm curious what the expected use case is for the lazy/eager types too. For the moment I'm writing functions that traffic in net::awaitable<boost_result<T>> or similar. The stuff in the documentation about types constructible from exception_ptr/error_code caught my attention, but I'm drawing a blank on if/how that gets integrated to the ASIO use case, or maybe you're envisioning something for LLFIO, etc.

Could be a candidate for an example or snippet?

ned14 commented 4 years ago

For ASIO, I would expect you can swap net::awaitable<boost_result<T>> for outcome::awaitables::eager<boost_result<T>>, as you want to execute the socket i/o now and hope it completes immediately. If the coroutine throws an unhandled exception, it will be put into the result via error_from_exception(), or into the outcome as an exception ptr.

Yes I am aware that I need to write some docs. A yet-to-be-done.

cstratopoulos commented 4 years ago

If you'll bear with me I'm still trying to build a mental model for the ASIO integration. Should this swap work with the help of a novel completion token?

The net::use_awaitable token makes it so that initiating an async op returns a net::awaitable; co_awaiting it suspends the coroutine, and completion of the operation resumes the coroutine. My as_result (with as_result(use_awaitable)) just interjects so that an exception_ptr or error_code is stuffed into an outcome type rather than being rethrown on coroutine resumption.

So would we still be letting ASIO handle the coroutine transformation and doing something like as_eager_result(use_awaitable) or, to get the performance benefit you've outlined, is it necessary to cut out the use_awaitable middleman entirely?

ned14 commented 4 years ago

My as_result (with as_result(use_awaitable)) just interjects so that an exception_ptr or error_code is stuffed into an outcome type rather than being rethrown on coroutine resumption.

That's all Outcome's awaitables do as well. Nothing magical.

So would we still be letting ASIO handle the coroutine transformation and doing something like as_eager_result(use_awaitable) or, to get the performance benefit you've outlined, is it necessary to cut out the use_awaitable middleman entirely?

I would imagine a new ASIO completion token outcome::awaitables::use_eager would do nicely. If you PR an implementation into that namespace using a new header e.g. asio_coroutine_support.hpp and boost_asio_coroutine_support.hpp, I'll merge it. That in turn should greatly simplify your ASIO recipe for Outcome.

cstratopoulos commented 4 years ago

Hey just got the notification for this. I had hoped to take a stab at the use_eager idea but this month has been....insane for me. Maybe you'd like to open a separate issue ticket for the [boost]_asio_coroutine_support.hpp stuff? Would love to give it a shot when the dust settles on my end, probably a matter of weeks still though.