[boost.async] Andrzej's review
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;
On Fri, Sep 29, 2023 at 7:42 PM Andrzej Krzemienski via Boost <boost@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....
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!
niedz., 1 paź 2023 o 07:23 Klemens Morgenstern < klemensdavidmorgenstern@gmail.com> napisał(a):
On Fri, Sep 29, 2023 at 7:42 PM Andrzej Krzemienski via Boost <boost@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
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
when 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.
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?
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....
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.
Thanks. This will work for me. I think it deserves its place in the docs doe select, and definitely in the examples. Regards, &rzej;
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!
participants (2)
-
Andrzej Krzemienski
-
Klemens Morgenstern