[boost] [boost.async] Andrzej's review
Andrzej Krzemienski via Boost
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;
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Niall Douglas via Boost
Firstly, it's weird I don't see this on the Boost mailing list. In fact,
I seem to only get about half the mail to the Boost mailing list ever
since they fiddled with the servers. So thanks for sending this review
to me directly.
I've also caught up with all your other posts on Boost.Async from
https://lists.boost.org/Archives/boost/2023/09/index.php, none of which
I had seen before now.
Thanks for the review. If you don't mind if I jump in with a few bits
myself which are intentionally not about the review itself ...
On 29/09/2023 14:08, Andrzej Krzemienski wrote:
> 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.
Outcome calls the same thing eager and lazy. I felt lazy is a more
descriptive name than task. I mean, why should a task be lazy rather
than eager? The reason is simply because Lewis Baker happened to choose
that default, and it's stuck since, but there isn't any strong rational
for "task ~= lazy" other than convention.
> 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.
It's been a while since I used coroutine generators, but my memory is
when the coroutine exits then the generator returns its end iterator
value. I don't remember dummy values. But then I 99.9% use C++
coroutines with Result, to avoid ever throwing C++ exceptions through
C++ coroutine machinery which *still* is riddled with showstopper bugs
in current compilers.
> On the documentation. The only other library offering coroutine support
> and any documentation that I know of is Boost.ASIO.
There are actually a good few Boost libraries with C++ coroutine support
now in one form or other. Outcome is obviously one, but there is at
least one other apart from ASIO (somebody else may remind me which one now).
Boost.Async's are much fuller featured obviously, but even Outcome's
simple ones are usually good enough for most use cases. You don't need
to use them with Result either.
> 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.
BTW tip for the future: just use helper coroutine lambdas if you need to
do coroutine stuff from within normal code. These normally don't work
with the Coroutine support from cppcoro, stdexec, libunifex nor ASIOs
because their coroutines don't allow it. But Boost.Async's does, as does
Boost.Outcome's.
It cannot be understated the usefulness of being able to dip in and out
of "coroutine land" using inline lambdas from normal code. It makes C++
coroutines vastly more convenient to use.
Niall
Vinnie Falco via Boost
bo...@lists.boost.org> wrote:
> Firstly, it's weird I don't see this on the Boost mailing list. In fact,
> I seem to only get about half the mail to the Boost mailing list ever
> since they fiddled with the servers.
>
Point of reference: I got the review through the mailing list.
Thanks
Klemens Morgenstern via Boost
<bo...@lists.boost.org> wrote:
>
> 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.
Thank you for submitting a review & and for all the previous feedback.
>
> 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.
That is pretty much it, but it has some api implications.
You can only spawn tasks, because only a lazy coroutine can safely run
on another thread. If that was a runtime decision it would be a
trip-wire.
You can only detach an eager coroutine, because it starts running when
you create it. If the task interface had it, you'd have the same
issue.
Regarding naming, that actually comes from JS & python.
If you have an `async def` in python, it's lazy & called a task,
if you have an `async func` in JS, it's eager & a promise.
>
> 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.
experimental::coro has a return & yield type that may not be the same.
If the return type is void, you can skip the co_return, but value
produced by such a coroutine is optional<T>.
Which is ok, I guess, but it introduces another thing users have to
remember. See the result table here:
https://think-async.com/Asio/asio-1.28.0/doc/asio/overview/composition/coro.html
>
> 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.
As a side-note: that might be different if we had an `async for` in
the language, but that was rejected from C++20.
<SNIP>
>
> 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.
struct visitor {
void operator()(tcp_socket&& sock) -> async::promise<tcp_socket> {
co_return std::move(sock); // ERROR
}
void operator()(variant2::monostate&&) -> async::promise<tcp_socket>
{
throw std::logic_error("Timeout");
}
};
co_await visit(visitor{}, std::move(ans));
That would work. Although we'd need an example where the variant
actually needs to be a coro.
> 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.
>
Thank you, I'll keep working on it!
Andrzej Krzemienski via Boost
klemensdavi...@gmail.com> napisał(a):
The above is actually a very good explanation and the justification of the
choice. (Better than what I have seen in the docs.) The lazy-eager choice
has to be reflected in the typesystem in order to provide type safety, and
implies different interfaces. Now, the difference between detaching and
spawning becomes clear: you detach something that has already started, and
you spawn something that has not yet started.
>
> Regarding naming, that actually comes from JS & python.
> If you have an `async def` in python, it's lazy & called a task,
> if you have an `async func` in JS, it's eager & a promise.
>
And the above tells me that the choice of names in Boost.Async is not the
best. Name "task" in Python makes sense because there is no "promise" in
sight. Name "promise" makes sense in NodeJS because there is no "task" in
sight. But putting them together in one library adds unnecessary confusion.
I agree with Niall here that names `async::lazy` and `async::eager` would
be more descriptive.
Now I can also see the problem with `generator` more clearly. It isn't
neither lazy nor eager in the sense used above: you cannot detach it nor
spawn it, because both detaching and spanning require the interface with a
single co_await (rather than the sequence of co_awaits). Am I right?
Thanks. This will work for me.
I think it deserves its place in the docs doe select, and definitely in the
examples.
Regards,
&rzej;