Re: [Async] Review of proposed Boost.Async begins

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2023-08-17 21:58:57

Hi all,

This is my review of the proposed Boost.Async.

- Does this library bring real benefit to C++ developers for real world
use-case?

I strongly believe so. There is no de-facto standard on C++20 I/O coroutines,
and this library can achieve this. It defines simple primitives that are
well-known in almost all other modern languages.

Writing async code using the universal Asio model is currently extremely
hard, time-consuming and error prone. This library can lift this burden.
Coroutines are now in the standard, so it's likely that we will see a rise
in demand for this kind of library in the forthcoming years. This library
anticipates this and puts Boost in a good position regarding I/O.

- Do you have an application for this library?

I have already written a real-time chat application with it. I envision this
library as the seed of an async ecosystem for C++, similar to Javascript's
promises or Python's asyncio. I think most use cases will involve
high-performance web/networking server applications.

- Does the API match with current best practices?

It does for most high-level classes and utilities. This includes promise,
task, generator, select, gather, join, co_main, use_op and detached. They
model well-known concepts and are easy to use.

I think this library uses exceptions too much. For instance, channel::read()
and channel::write() will always throw an exception on cancellation. Considering
that select may cancel all but the first-to-complete awaitable, you may end up
having exceptions thrown and caught in your regular code flow, which IMO
is a design error. I think this must be addressed before acceptance.
Tracked by https://github.com/klemens-morgenstern/async/issues/76

Is exposing enable_await_allocator, enable_await_executor
and friends necessary?. It increases the API surface of the library,
which makes evolving more difficult. These seem to be geared towards
implementing your own coroutine types. I'd place these in an experimental
namespace or hide them completely. If they are to be exposed,
we need more docs about this.

What's the rationale behind BOOST_ASYNC_USE_STD_PMR and friends? Being a
C++20 library, why is std::pmr not enough? From the Jamfile, the library is
built only with std::pmr by default. This looks like a source of problems -
you need to explain to users how to build your library with Boost.Container pmr
and without it. Config macros like these increase maintenance effort a lot.

I've also noticed that boost::async::this_coro::executor_t is an alias for
boost::asio::this_coro::executor_t. I think this is confusing - if
I got the implementation right, no functionality from Asio is used here,
but just the name. Can we not define an async::executor_t?

- Is the documentation helpful and clear?

The discussion is more or less clear for the high-level classes and utilities,
including promise, task, generator, select, gather, join and co_main.
use_op assumes too much Asio knowledge. It should at least mention that it's
a completion token, and at least link to Asio's page.

I'm missing a proper reference for some functions. While promise, task or
wait_group have these, select, gather and join only show examples, and not
the actual signatures of the functions.

The section on implementing hand-coded operations needs some expansion, too.
async::handler and async::completion_handler are not shown in the reference,
which they should.

I've had a hard time navigating the docs, since they are single-page.
I'm a little bit concerned about the duplication between code and docs - having
docs generated from the source code makes sure that there are no
inconsistencies.
I think the current setup violates the DRY principle. Not having in-source
comments makes usage harder, too, since in modern IDEs it's common to go
to the entity's definition, and it's useful to have some comments there.

I've almost missed the examples that are not included in the docs. I'd advocate
to include all of them in the docs, since they add valuable information that is
easy to miss. The Python one needs more comments, BTW.

- Did you try to use it? What problems or surprises did you encounter?

I migrated a real-time chat application I'm writing from stackful coroutines
(i.e. boost::asio::spawn) to Boost.Async coroutines. In general, my experience
was positive, with some gotchas. I summarized my experience in this email:
https://lists.boost.org/Archives/boost/2023/08/254921.php

- What is your evaluation of the implementation?

I have only looked at parts of it. I like the part where promise, task and
friends inherit from a common set of classes to implement individual features
(enable_await_allocator, enable_await_executor and friends).

I've got the feel that I found a bunch of bugs while testing. Concretely,
https://github.com/klemens-morgenstern/async/issues/68 this one made the library
not usable. The author has fixed most of them already (thanks Klemens).
I think more testing is required to cover cases like the one above.
I can't see any regression tests covering the different pmr B2 features
and BOOST_ASYNC_CUSTOM_EXECUTOR, either.

- Are you knowledgeable about the subject?

I've implemented generic, async code using Boost.Asio in the form of a
MySQL client (currently Boost.MySQL). I'm not an expert in C++20 coroutines,
but I know enough to understand the implementation. I've used
asio::yield_context and similar coroutines in other languages thoroughly.

- How much time did you spend evaluating the library?

I've built the library, read the source code and used it to implement a
web server. I've dedicated something less than 20h to the full process.

- Please explicitly state that you either _accept_ or _reject_ the inclusion
of this library into boost.

I think Boost.Async should be _CONDITIONALLY ACCEPTED_ into Boost, provided that
the following is addressed:

- It should be possible to use generators without exceptions
(https://github.com/klemens-morgenstern/async/issues/76)
- All build errors and bugs reported during testing should be fixed and
proper regression tests added for each of them.
- BOOST_ASYNC_USE_STD_PMR macros are either removed or regression tested.
- enable_await_allocator, enable_await_executor and friends are either
hidden or expanded in the docs.

Disclaimer: both Klemens and I are associated with the C++ Alliance. I'm
writing this review trying to be as impartial as possible.

Thanks Klemens for submitting and Niall for managing the review.

Regards,
Ruben.

On Tue, 8 Aug 2023 at 07:33, Niall Douglas via Boost
<boost_at_[hidden]> wrote:
>
> Dear Boost,
>
> It is my great pleasure to announce the beginning of the peer review of
> proposed Boost.Async, which is hoped to become the most popular way of
> programming C++ coroutines in future C++. The peer review shall run
> between Tuesday 8th August and Friday 18th August.
>
> Like many of you have found, until now there has been no good standard
> and easy way of writing C++ coroutine code which works easily with
> multiple third party codebases and foreign asynchronous design patterns.
> ASIO’s current coroutine support is tightly coupled to ASIO, and it
> rejects foreign awaitables. Libunifex is dense to master and non-obvious
> to make work well with foreign asynchronous design patterns, not helped
> by a lack of documentation, a problem which also afflicts stdexec which
> also has a steep and dense learning curve. CppCoro is probably currently
> the best in class solution to the space of easy to use fire and forget
> C++ coroutine programming, but it isn’t particularly portable and was
> designed long before C++ coroutine standardisation was completed.
>
> Klemens very kindly stepped up and created for us an open source
> ground-up reimplementation of the best ideas of hitherto proprietary
> commercial C++ coroutine libraries, taking in experience and feedback
> from multiple domain experts in the area to create an easy to use C++
> coroutine support library with excellent support for foreign
> asynchronous design patterns. For example, if you want your coroutine to
> suspend while a stackful fiber runs and then resume when that fiber has
> done something – AND that fiber implementation is unknown to Boost.Async
> – this is made as easy as possible. If your C++ coroutine code wants to
> await on unknown foreign awaitables, that generally “just works” with no
> added effort. This makes tying together multiple third party
> dependencies into a solution based on C++ coroutines much easier than
> until now.
>
> The industry standard executor is ASIO, so Boost.Async uses Boost.ASIO
> as its base executor. To keep things simple and high performance,
> Boost.Async requires each pool of things it works with to execute on a
> single kernel thread at a time – though there can be a pool of things
> per kernel thread, or an ASIO strand can ensure execution never occurs
> on multiple kernel threads. The basic concurrency primitives of promise,
> generator and task are provided, with the fundamental operations of
> select, join and gather. Some more complex async support is supplied:
> Go-type channels, and async teardown. Performance is superior to both
> stackful coroutines and ASIO’s own C++ coroutine support, and sometimes
> far superior.
>
> You can read the documentation at https://klemens.dev/async/ and study
> or try out the code at https://github.com/klemens-morgenstern/async.
>
> Anyone with experience using C++ coroutines is welcome to contribute a
> review at the Boost mailing list
> (https://lists.boost.org/mailman/listinfo.cgi/boost), here at this
> Reddit page, via email to me personally, or any other mechanism where I
> the review manager will see it. In your review please state at the end
> whether you recommend acceptance, acceptance with conditions, or
> rejection. Please state your experience with C++ coroutines and ASIO in
> your review, and how many hours you spent on the review.
>
> Thanks in advance for your time and reviews!
>
> Niall
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost