[boost.async] Andrzej's review

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2023-09-29 11:41:08

Hi Everyone,

I would like to thank Klemens for writing and sharing the Async library. It
is something the community needs, and it is clear that a lot of thought and
effort was put in it.

I am not an expert on C++ coroutines or coroutines in general. I have
played with a few echo-server examples in the past from Boost.ASIO, and
created some toy examples of C++ coroutines. I devoted something like
twelve evenings studying the Boost.Async: reading docs, testing toy
examples (Windows 11, MSVC 2022), debugging and asking Klemens a lot of
questions. Still, I cannot say I understand the library well enough to be
able to responsibly give a recommendation. But I can still share my
observations.

On the design. Although my knowledge is superficial, I will risk a claim
that the synchronization mechanisms and supporting features (`with`,
channels, work_group) are the strong part of the design. (I disagree with
the overload set for `select`, but I guess this is a non-design bug.)

Regarding the choice and  design of "coroutine types", I have certain
reservations.
It is still not clear to me why we have two -- promise and task -- and when
I should use which. The docs give some answer: promise is eager, task can
be awaited/spawned on another executor than it was created on. But is that
it? I sense that there is a good reason to have them as two types, a
better/clearer one than this described in the docs.But I fail to see it,
and the docs do not explain it clearly enough, I think.

I am uneasy about the design for `generator`. I filed a couple of issues:
https://github.com/klemens-morgenstern/async/issues/101
https://github.com/klemens-morgenstern/async/issues/70
One problem is how the `generator` signals that it has finished. In all the
examples it has to create a dummy value. I hear from Klemens that it is a
necessity imposed by the coroutine design in C++. I do not feel competent
to confirm it or not. But I note that std::generator does not require the
user to `co_return` anything. And that asio::experimental::coro does allow
the user to specify that they do not want to co_return even if they
co_yield things.

Regarding the value-returning generators, I have even more reservations. As
noted in one of the quoted GitHub issues, the fact that it has a different
interface than the normal generator, and that it has to be lazy to be
usable, It looks to me like it qualifies for a separate coroutine type.

On the documentation. The only other library offering coroutine support and
any documentation that I know of is Boost.ASIO. In comparison to
Boost.ASIO, Boost.Async has a waaay better documentation. It is big (in a
positive sense), it has a lot of tutorial materials, lots of ideas
explained, it is still being improved. It is waaay above the average
quality of a library documentation that you can find on the web.  But still
I would greatly appreciate an even better one, however arrogant that seems.
My reasoning is that for the next couple of years people who will consider
Boost.Async will not only be unfamiliar with the library but also with C++
coroutines in general. They will need additional extra help to be able to
use the library responsibly.

To be more specific, I would expect more full compiling examples (those in
folder examples) that illustrate different aspects of the library. For
instance:
* A convincing example of value-returning generator
* timeout implemented in terms of `async::select`.

I would expect a more detailed explanation of concepts used in this library:
* interruption and cancellation, and how they differ, and when/if I should
care about them.
* being detaching and being detached, and how it is different from
spawning.

I got a response in one of GitHub issues that, "Interrupt is [...] meant to
make select work lossless. What gets detached is the co_await itself, not
the awaited thing." I feel the documentation should explain it more
cleanly, and perhaps introduce some more well defined terms.

The library does not cleanly communicate preconditions on the operations. I
think it sometimes decides to throw an exception when a precondition is
more appropriate.  In other places it is silent about the corner cases. It
also does not specify clearly what is guaranteed of the provided
operations. Klemens correctly points out that postconditions are tricky in
the case of asynchronous operations, but
(1) some things in the library are not asynchronous and (2) this library
(like any other) does offer some guarantees, if not on individual function
calls, then for the entire expressions. For instance

`co_await select(aw1, aw2)`

does provide a guarantee that the result of one of the awaitables will be
returned and the others will be interrupted. It should be specified in the
synopsis of `select`.

On the implementation. It is mostly magic to me, so I do not have much to
say. There are a lot of awaitable types created whose existence I could not
predict from studying the docs. I cannot say if this is a deficiency of the
documentation, or is this how the C++ coroutines work: you might need a lot
of awaitables that are implementation details.

I find the design of the overload set of functions `select` bad and
generic-programming-unfriendly. I explained it in detail in:
https://github.com/klemens-morgenstern/async/issues/103
Functions that do slightly different things should have different names.
Returning different types for special cases breaks generic code, and causes
surprises, sometimes runtime surprises.

Also, because the overload for value-returning promises returns a variant,
I have run into an interesting situation when trying to add a timeout
support to the echo server example. I get the following statement:

```
variant2::variant<variant2::monostate, tcp_socket> ans
  = co_await async::select(
    timer.async_wait(async::use_op),
    acceptor.async_accept()
  );
```

And now I have a variant to unpack inside a coroutine. Normally, the
recommended way to unpack a variant is to use a visitor:

```
struct visitor {
  void operator()(tcp_socket&& sock) {
    co_yield std::move(sock); // ERROR
  }
  void operator()(variant2::monostate&&) {
   }
 };
visit(visitor{}, std::move(ans));
```

But this doesn't work, because now I am co_yield-ing from a non-coroutine.
And I have to resort to manually inspecting the variant, which is taught to
be error prone.

I guess this is the right choice to use variant for the return type of
`select`, and that it is the limitation of variant, until C++ gets pattern
matching. But still, it would be helpful if the documentation (or examples)
illustrated what potential difficulties the user can expect.

Should the library be accepted to Boost? Again, I am not competent enough
in the domain of coroutines, to responsibly say that the library meets the
bar, but because I think the community desperately needs this kind of
library with Boost-quality docs and support, I will risk saying that
Boost.async should be conditionally accepted. The conditions being:
* Address the design of generators (either change it or provide a clearer
rationale)
* Improve the docs
 - clearly describe the notions of interrupting, canceling, detaching,
spawning.
 - provide a more detailed synopses for functions and classes with
preconditions, described effects, and the guarantees.

I filed this as a list of issues in the GitHub:
https://github.com/klemens-morgenstern/async/issues/120
https://github.com/klemens-morgenstern/async/issues/119
https://github.com/klemens-morgenstern/async/issues/117
https://github.com/klemens-morgenstern/async/issues/115
https://github.com/klemens-morgenstern/async/issues/114
https://github.com/klemens-morgenstern/async/issues/103
https://github.com/klemens-morgenstern/async/issues/101
https://github.com/klemens-morgenstern/async/issues/100
https://github.com/klemens-morgenstern/async/issues/70
https://github.com/klemens-morgenstern/async/issues/64

Finally, while the above might look like a list of complaints, I really
appreciate the work that was done in order to provide this library, and the
solutions that were applied (like the clever use of await_transform). I
learned a lot from studying this library, and my intuition tells me that it
will end up being a great addition to Boost. I also appreciate the
responsiveness of the author. Klemens has been answering a lot of my
questions in a timely fashion, and improving the docs based on feedback.

Thanks!

Regards,
&rzej;