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!