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

OUTCOME_TRY for coroutines #199

Closed
cstratopoulos opened this issue Jul 11, 2019 · 13 comments
Closed

OUTCOME_TRY for coroutines #199

cstratopoulos opened this issue Jul 11, 2019 · 13 comments
Labels

Comments

@cstratopoulos
Copy link
Contributor

@cstratopoulos cstratopoulos commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Owner

@ned14 ned14 commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@cstratopoulos cstratopoulos commented Jul 12, 2019

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 ned14 added the enhancement label Jul 12, 2019
@ned14

This comment has been minimized.

Copy link
Owner

@ned14 ned14 commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Collaborator

@akrzemi1 akrzemi1 commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Owner

@ned14 ned14 commented Jul 12, 2019

:)

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

This comment has been minimized.

Copy link
Collaborator

@akrzemi1 akrzemi1 commented Jul 21, 2019

Update from Cologne meeting: co_return stays.

@ned14

This comment has been minimized.

Copy link
Owner

@ned14 ned14 commented Aug 2, 2019

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 added a commit that referenced this issue Oct 3, 2019
…ts, firstly there is

now an `OUTCOME_CO_TRY()` operation suitable for performing the `TRY` operation from
within a C++ Coroutine. Secondly, in the header `outcome/coroutine_support.hpp` there are
implementations of `awaitable<OutcomeType>` and `task<OutcomeType>` which let you more
naturally and efficiently use `basic_result` or `basic_outcome` from within C++
Coroutines -- specifically, if the result or outcome will construct from an exception
pointer, exceptions thrown in the coroutine return an errored or excepted result with
the thrown exception instead of throwing the exception through the coroutine machinery
(which in current compilers, has a high likelihood of blowing up the program). Both
`awaitable<T>` and `task<T>` can accept any `T` as well. Both have been tested and found
working on VS2019 and clang 9.
@ned14

This comment has been minimized.

Copy link
Owner

@ned14 ned14 commented Oct 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@cstratopoulos cstratopoulos commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Owner

@ned14 ned14 commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@cstratopoulos cstratopoulos commented Oct 15, 2019

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

This comment has been minimized.

Copy link
Owner

@ned14 ned14 commented Oct 15, 2019

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.

@ned14 ned14 closed this Nov 15, 2019
@cstratopoulos

This comment has been minimized.

Copy link
Contributor Author

@cstratopoulos cstratopoulos commented Nov 15, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.