[Async] Peer review report and conditions

From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2023-10-11 10:25:26

Dear Boost,

Proposed Boost.Async by Klemens Morgenstern was reviewed between 8th and 
18th August, and again between 22nd September and 2nd October. Reviews 
or substantially detailed comments came from:

- /u/trailing_zero_count on Reddit
- Ruben Perez
- Marcelo Zimbres Silva
- Andrzej Krzemienski
- Mohammad Nejati
- Janko Dedic
- Robert Leahy

I would like to thank those above for taking the time to write their 
detailed comments or reviews, and if I forgot to list anyone, please do 
forgive me. I did read all reviews and comments as they came in, so even 
if your name is not listed above, I did take into account your feedback.

I would also like to thank all those far too many to list here who gave 
initial impressions, relaying spelling mistakes in the docs, or 
otherwise gave feedback of various forms. Boost could not continue 
without you!

The tl;dr; outcome is that proposed Boost.Async is ACCEPTED but with 
CONDITIONS. Next follows are the **binding** conditions without which 
the library is not accepted:

1. The library needs to be renamed to something the Boost community 
doesn’t object to

Quite a bit of discussion and feedback surrounded the proposed name 
“Boost.Async”. Multiple people felt that the name claims too much seeing 
as it is a C++ 20 coroutines only library, and is from first appearances 
entirely tied into Boost.ASIO (technically, any ASIO i/o executor can 
wrap any third party executor, but I can see where people get the strong 
ASIO association).

I agree with Klemens that putting “ASIO” in the new name would be out of 
character with the other Boost libraries also heavily orientated around 
Boost.ASIO which do not have ASIO in their name.

I find the suggestions that C++ 20 coroutines needs to be somehow in the 
name convincing. Exactly what form it should take I’ll leave up to the 
author and community to negotiate, but “Boost.Async” is definitely not a 
name with good buy-in in the Boost community. It cannot be named 
“Boost.Async” if it is to enter Boost.

2. The `select()` operation either needs to have its semantics changed, 
or be renamed

Quite a few reviewers found the current semantics unpleasantly 
surprising, in that a random ready awaitable is chosen and all the 
others cancelled. This is far away from what `select()` typically means 
in the C and C++ worlds, and multiple reviewers commented negatively 
about this.

The docs are unclear if it is guaranteed that the first ready awaitable 
is always chosen, or whether it is _some_ ready awaitable. The 
difference is important in many kinds of async programming. As this is a 
single threaded implementation, it should be possible to offer strict 
guarantees here.

The semantics are acceptable if the operation is not called “select”, so 
I would suggest the easiest course here is to rename that operation to 
something more acceptable to the Boost community.

3. Channels need to terminate the process if relocated during use, or be 
made immovable, rather than it being UB to relocate them

Some reviewers found the hard UB of moving a channel in use a poor 
design decision. Hard UB is good if it confers distinct benefits, 
however reviewers couldn’t see one here, and neither can I. As this 
library requires C++ 20 in any case, the obvious thing to do is make 
them immovable, but if that is felt too cumbersome, then they ought to 
noisily terminate the process or something not footgunny if relocated 
during use.

Other non-binding feedback, which is not a hard condition of acceptance:

- Multiple people found the naming of `promise` confusing. A promise is 
something in C++ land associated with a future, and I suppose in a few 
years it will also be associated with C++ coroutine awaitables. But here 
we have an awaitable named `promise`? Yes I know it makes sense in other 
programming languages, but here in C++ land it just seems weird and 
confusing for no obvious gain. I personally would suggest `eager_task` 
and `lazy_task` but I have no especially strong opinions on naming here 
other than that `promise` is a poor choice for any awaitable type. In 
fact, I think just don’t put the word “promise” into any awaitable type 
period.

- I am sympathetic to multiple people raising the lack of preconditions 
and postconditions on operations described in the documentation. I also 
accept that in an async world, it’s hard to be precise. However, I think 
it would be a large benefit to the documentation to describe for each 
operation a list of the ways in which it can fail, and I think this 
should be added.

- Andrzej took issue with the design of the generators, and I can see 
why. However, out in real world code land, C++ coroutine generators have 
surprisingly large amount of design variation. I’ve seen ones which you 
loop on testing them for boolean true, I’ve seen others which loop on 
their iterators, I’ve even seen ones which don’t implement either 
boolean true nor iterators and require you to call a `value()` member 
function which returns an Optional. Why? No idea, people seem to 
innovate uselessly here for some reason. I think it wise to adhere to 
`std::generator` from the standard, and do exactly what it does. From my 
reading of Async’s `generator`, whilst it has extensions e.g. the 
invocable call operator, the only thing missing is the iterator 
interface to make it implement `std::generator` and I don’t see any 
obvious reason why an iterator interface can’t be added here?

- I agree with Andrzej about ever throwing `std::logic_error`, it should 
not be done in newly written code. It is recognised as a mistake by most 
on the standards committee nowadays.

- The Mersenne Twister is a poor PRNG by modern standards. You should 
use JSF or anything else both much faster and higher quality.

- More than one reviewer wants the docs to do a much better job on 
documenting pseudo-awaitables and awaitables and `await_transform`.

- Quite a few times the docs talks about “async” when it really means 
“C++ coroutines”.

- I think the docs does a poor job of disambiguating when what it is 
describing is part of the C++ standard, or it is part of Boost.Async. 
The reason this is important is that users need to know when/if they are 
locking themselves into Boost.Async proprietary extensions vs standard 
C++ coroutines.

Finally, as a personal note on the review, it was notable how much 
negativity there currently is against C++ coroutines in general from the 
C++ community. A small number of us have been writing C++ coroutine code 
for many years now, we have been through the process of getting 
footgunned over lifetime issues, got frustrated that these coroutines 
are not the coroutines we were looking for, and eventually came round to 
understanding what C++ coroutines are good at solving, and more 
importantly what they are NOT good at solving.

Once your head is wrapped around that, then comes the problem of having 
to reinvent a C++ coroutine support library because all the current open 
source ones have severe issues, especially around very tight coupling 
being imposed on your codebase. I don’t personally agree with every 
design decision in proposed Boost.Async, but I do think this will be a 
major step forward in making C++ coroutines accessible to many more 
developers than now, especially because this is to my knowledge the 
first standards-based loosely coupled C++ coroutines library which is 
open source.

Undoubtedly after it ships with Boost, deficiencies will be found both 
in this library and in the currently quite poor quality implementation 
of C++ coroutines in the three major compilers. I very much hope that 
the increased number of developers using C++ coroutines as a result of 
this new Boost library will result in large and substantial improvements 
in implementation and library support of C++ coroutines.

My thanks to Klemens for putting the work into what is a controversial 
and opinionated library design quite unlike any other currently 
available. It’s not easy to tread new ground without much precedent.

My thanks to the Boost and Reddit communities for their ample and 
detailed review feedback, which made me managing this review possible.