Re: [boost.async] Andrzej's review

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2023-10-01 11:18:21

niedz., 1 paź 2023 o 07:23 Klemens Morgenstern <
klemensdavidmorgenstern_at_[hidden]> napisał(a):

> On Fri, Sep 29, 2023 at 7:42 PM Andrzej Krzemienski via Boost
> <boost_at_[hidden]> 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.
>

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.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.
>

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!
>