[boost] [Async] Peer review report and conditions
Niall Douglas via 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.
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Klemens Morgenstern via Boost
> 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.
>
How do we judge what is acceptable to the boost community? Because
there might always be someone to object to it.
I'll compose a list and start a separate thread.
>
> 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.
>
There is left_select, which goes strictly left to right.
Would this criteria be satisfied if I renamed left_select to select,
and the current select to `unordered_select` or `random_select` ?
>
> - 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?
>
The reason is that we don't have "async for".
I would need to resume the generator on increment,
but if `itr++` is an awaitable expression that needs to be co_awaited,
it's not an iterator.
If there was an async for (or a common pattern) I'd make generator work with it.
>
> 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.
>
Thank you for managing this double-period review!
Christian Mazakas via Boost
> negativity there currently is against C++ coroutines in general from the
> C++ community.
I think most people are incredibly excited about coroutines. They're
just less excited about an Asio wrapper.
Though even Rust suffers the problem that in the ecosystem, Futures
are tied to their runtimes. In practice, this isn't overwhelmingly a
showstopper but I think this is the next area of exploration and
development: building a common language for disparate runtimes to use
for cross-communication.
- Christian
Klemens Morgenstern via Boost
bo...@lists.boost.org> wrote:
> > 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.
>
> I don't think this is a very accurate description at all.
>
> I think most people are incredibly excited about coroutines. They're
> just less excited about an Asio wrapper.
>
> Though even Rust suffers the problem that in the ecosystem, Futures
> are tied to their runtimes. In practice, this isn't overwhelmingly a
> showstopper but I think this is the next area of exploration and
> development: building a common language for disparate runtimes to use
> for cross-communication.
provide that?
Niall Douglas via Boost
>> 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.
>>
>
> How do we judge what is acceptable to the boost community? Because
> there might always be someone to object to it.
>
> I'll compose a list and start a separate thread.
I don't claim it'll be easy, but there was strong consensus that the
current name must go.
> There is left_select, which goes strictly left to right.
> Would this criteria be satisfied if I renamed left_select to select,
> and the current select to `unordered_select` or `random_select` ?
The review objections where to naming that operation "select" at all.
Select for 99% of C and C++ users is something which does not cancel the
other waiters on return. Several mentioned how surprising it was, for
them Select is something which returns when something is ready and then
code might do stuff like cancel things.
I don't think there is anything wrong with the current semantics of the
operation, except its name.
`select_cancelling_others()` would be fine, but I'm sure a shorter name
is possible without the BSD sockets type connotations.
>> - 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?
>>
>
> The reason is that we don't have "async for".
> I would need to resume the generator on increment,
> but if `itr++` is an awaitable expression that needs to be co_awaited,
> it's not an iterator.
> If there was an async for (or a common pattern) I'd make generator work with it.
Y'see I don't see the need for async for. That's separate to this.
`std::generator` provides iterators. When you iterate one of its
iterators the coroutine gets resumed and it yields a new value.
The reason `std::generator` is like this is because it can be called
from non-coroutine code, so `co_await` isn't available. Implied in this
is iterator implementations must pump the coroutine by hand.
This is very straightforward for the simple case, it gets harder if the
generator suspends on say i/o. Then your generator iterator
implementation needs to pump the event loop too. This isn't a hard to
surmount problem to solve for Boost.Async I think. And I think a Boost
level library does need to implement what the C++ standard demands, even
if it's a bit dumb.
Down the road if WG21 ever gets round to async for, support for that can
be added later. I have also wondered if a `BOOST_CO_ASYNC_FOR` macro
couldn't implement async for right now.
And finally, I have a gut feeling without proof nor evidence that it
*may* be possible to detect from a macro whether it is being invoked by
a coroutine or not, and if so, "do the right thing" based on caller
context. So a `BOOST_ASYNC_FOR()` macro which does the right kind of for
loop for iterating a generator depending on caller may be achievable.
Niall
Niall Douglas via Boost
>> 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.
>
> I don't think this is a very accurate description at all.
>
> I think most people are incredibly excited about coroutines. They're
> just less excited about an Asio wrapper.
A year to eighteen months ago /r/cpp was all excited about coroutines.
When I announced this review much spilled forth about C++ coroutines
being awful. Various people reporting trying them out, blowing off their
leg, and thus concluded they must never use them again.
They'll get over that I think eventually. Give it another eighteen months.
Niall
David Sankel via Boost
bo...@lists.boost.org> wrote:
> 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...
>
Thanks Niall. Could you please indicate the number of formal Boost reviews
that were made and their tally of accept/reject?
Niall Douglas via Boost
> On Wed, Oct 11, 2023 at 6:25 AM Niall Douglas via Boost
> <bo...@lists.boost.org <mailto:bo...@lists.boost.org>> wrote:
>
> 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...
>
>
> Thanks Niall. Could you please indicate the number of formal Boost
> reviews that were made and their tally of accept/reject?
I listed the reviews or feedback substantial enough to be worth naming
their authors at the top.
I intentionally did not list whether they recommended accept or reject.
If you want to know that, you can look up the review. I believe all but
one was accept, but some gave no formally written recommendation so I
had to infer from the body of the feedback.
In any case, it does not matter, approval or rejection isn't based on
vote counts.
Niall
Richard Hodges via Boost
wrote:
>
> 2. The `select()` operation either needs to have its semantics changed,
> or be renamed
>
library, which itself is a distant descendant of work I and later Klemens
did to replicate Go's channel and select() in C++ I/O loops.
The Go select() function does exactly what Klemens' does - suspends until
the first source/coroutine has a value available (ties broken by pseudo
random selection in order to prevent starvation of the right-most sources),
and then continues, leaving the values in the unselected coroutines
available for subsequent select() calls.
Being a user of Asio for the past decade, It didn't occur to me that people
still actually used BSD select()...
Accepting that the name select() is not acceptable, my motivation for
encouraging Klemens to have the default xxx() function take a random ready
value, and the left_xxx() function to take the leftmost, was that taking
the leftmost ready value is very likely to lead to starvation of code that
responds to the the right-most sources delivering values if the leftmost
sources are constantly ready. xxx() is therefore the safer unsurprising
default for an inexperienced or tired developer.
Niall Douglas via Boost
> I must claim some culpability for this. I was an early adopter of the
> library, which itself is a distant descendant of work I and later Klemens
> did to replicate Go's channel and select() in C++ I/O loops.
>
> The Go select() function does exactly what Klemens' does - suspends until
> the first source/coroutine has a value available (ties broken by pseudo
> random selection in order to prevent starvation of the right-most sources),
> and then continues, leaving the values in the unselected coroutines
> available for subsequent select() calls.
>
> Being a user of Asio for the past decade, It didn't occur to me that people
> still actually used BSD select()...
>
> Accepting that the name select() is not acceptable, my motivation for
> encouraging Klemens to have the default xxx() function take a random ready
> value, and the left_xxx() function to take the leftmost, was that taking
> the leftmost ready value is very likely to lead to starvation of code that
> responds to the the right-most sources delivering values if the leftmost
> sources are constantly ready. xxx() is therefore the safer unsurprising
> default for an inexperienced or tired developer.
callable which gets invoked to decide on the tie break, and I'd default
its implementation to one choosing randomly. Then users can choose
whatever suits them, and no need for left_xxx() functions etc.
Niall
Richard Hodges via Boost
Sent from my iPad
> On 11 Oct 2023, at 21:11, Niall Douglas via Boost <bo...@lists.boost.org> wrote:
This is probably why it’s best I stick to hacking together trading systems.
Wouldn’t it be difficult to default the argument to prefer_left() or similar though, given that the parameters are a variadic pack of sources?
Not having a default would be unacceptable to me as it would involve:
- more typing, and
- having to read the documentation prior to hacking the code together.
Niall Douglas via Boost
>> If I were designing that API, I'd personally provide a user supplied
>> callable which gets invoked to decide on the tie break, and I'd default
>> its implementation to one choosing randomly. Then users can choose
>> whatever suits them, and no need for left_xxx() functions etc.
>
> This is probably why it’s best I stick to hacking together trading systems.
>
> Wouldn’t it be difficult to default the argument to prefer_left() or similar though, given that the parameters are a variadic pack of sources?
>
> Not having a default would be unacceptable to me as it would involve:
> - more typing, and
> - having to read the documentation prior to hacking the code together.
I guess it depends on what you're used to, but in principle I have no
idea with convenience wrapper functions which save obtuse syntax for a
base implementation function.
You can design APIs to take variadic packs of variadic packs if you
want. I actually have an API like that in my work code base, I couldn't
think of anything less worse, so I went with it.
Klemens Morgenstern via Boost
> The review objections where to naming that operation "select" at all.
>
> Select for 99% of C and C++ users is something which does not cancel the
> other waiters on return. Several mentioned how surprising it was, for
> them Select is something which returns when something is ready and then
> code might do stuff like cancel things.
>
> I don't think there is anything wrong with the current semantics of the
> operation, except its name.
>
> `select_cancelling_others()` would be fine, but I'm sure a shorter name
> is possible without the BSD sockets type connotations.
>
I checked if I could force select to only work on interruptible ops,
but that's too restrictive,
as it would not allow idempotent operations like timer.async_wait()
that might be used as a timeout.
The idea for the name comes from go. I guess a similar one is race
which is used in JS.
Ah, gotcha I thought you wanted that to be in async, i.e. with a
co_await somewhere.
Yeah that could be done & might actually be something useful for
non-pushable generators.
I'll look into it.
> Down the road if WG21 ever gets round to async for, support for that can
> be added later. I have also wondered if a `BOOST_CO_ASYNC_FOR` macro
> couldn't implement async for right now.
>
> And finally, I have a gut feeling without proof nor evidence that it
> *may* be possible to detect from a macro whether it is being invoked by
> a coroutine or not, and if so, "do the right thing" based on caller
> context. So a `BOOST_ASYNC_FOR()` macro which does the right kind of for
> loop for iterating a generator depending on caller may be achievable.
>
I couldn't think of a way to detect that.
I have a very simple BOOST_ASYNC_FOR macro here Documentation
https://klemens.dev/async/reference.html#async_for
It's more of a while loop though, so I might need to reconsider that
one as well.
Klemens Morgenstern via Boost
> > > The reason is that we don't have "async for".
> > > I would need to resume the generator on increment,
> > > but if `itr++` is an awaitable expression that needs to be co_awaited,
> > > it's not an iterator.
> > > If there was an async for (or a common pattern) I'd make generator work with it.
> >
> > Y'see I don't see the need for async for. That's separate to this.
> >
> > `std::generator` provides iterators. When you iterate one of its
> > iterators the coroutine gets resumed and it yields a new value.
> >
> > The reason `std::generator` is like this is because it can be called
> > from non-coroutine code, so `co_await` isn't available. Implied in this
> > is iterator implementations must pump the coroutine by hand.
> >
> > This is very straightforward for the simple case, it gets harder if the
> > generator suspends on say i/o. Then your generator iterator
> > implementation needs to pump the event loop too. This isn't a hard to
> > surmount problem to solve for Boost.Async I think. And I think a Boost
> > level library does need to implement what the C++ standard demands, even
> > if it's a bit dumb.
> >
>
> Ah, gotcha I thought you wanted that to be in async, i.e. with a
> co_await somewhere.
> Yeah that could be done & might actually be something useful for
> non-pushable generators.
> I'll look into it.
>
made the `++itr` awaitable.
With std::generator the increment also rethrows exceptions.
That means I could in theory have an async_for do a `co_await
async::next(itr)` instead of `co_await ++itr` and have the same
iterator type work.
There is however a problem with "pumping the event-loop". I have the
executor but I might not know the event-loop it's referring to (it
might be a thread_pool) or something custom.
Now I would either need to somehow create an interface to allow the
user to supply a pump function or require that he uses
asio::io_context for that to work.
I don't like either, I think I'll hold off until there's a user asking for it.