Disclaimer: I have been involved in the development in affiliation with the
C++Alliance.
I wish to first of all express that I really admire the effort of creating
a coroutine-first alternative to boost.asio that can optimize run- and
compile-time
by having only one asynchronous completion mechanism.
However, I fear that capy & corosio are not ready.
Below is a list of criticisms & issues that might end up as part of my
review.
However, I think it is better to allow the authors to address these first.
I have discussed some of these with Vinnie on Slack already, but I think it
is better to get this on the record for the review. So I hope he doesn't
mind reiterating some of the discussion we had.
# 1. Comparison to asio
Asio is incredibly feature-rich and I would not expect capy/corosio to
match all the features. However, I would hope it to would more features or
different approaches, that take the coroutines into account.
However, they are mainly compile and run-time improvements and very little
in functionality.
Since `capy::task` is functional equivalent to `asio::awaitable` the only
thing I can identify are the type erased streams (e.g. any_read_stream) and
the ability to use different SSL implementations like wolfSSL.
The type-erase stream types however are then not even used, the
corosio::read_until function is templated.
Their ownership semantic (construction by pointer is non-owning,
construction by pointer is owning) is very unintuitive.
Construction by lvalue ref is non-owning, by rvalue ref is owning would
make more sense to me.
As a side-note: Botan actually offers a SSL stream for asio, so technically
other SSL implementations for asio exist.
Other than that there seems to be a lot of "asio did it" design decisions.
## 2. capy::continuation
The continuation type is useful because the executor doesn't need to
allocate a list on it's own, it uses an intrusive list of which
`continuation` is a node.
However, the `next` member is public and default constructed as null.
If this was an implementation detail, why isn't it private? Or at least
name `next_` or `_next` ?
It could be public and allow me to submit multiple continuations at once,
but as it is, it is an odd api.
At the very least I would expect a non-explicity constructor from a
`coroutine_handle`.
It is an implementation detail leaking out for no apparent reason.
## 3. capy::async_mutex
The mutex is a thing that's not thread-safe. This naming is just bad.
And no, it's not enough that it's documented, as this is very
counter-intuitive.
## 4. capy::DynamicBuffer / capy::read_until
The dynamic_buffer concept comes from asio and I don't think it is needed
in a coroutine library. It's one of the "asio did it" features.
The reason asio has those is because of `async_read_until`. In asio,
writing an asynchronous algorithm is extremely painful, so using
`async_read_until` is a massive help. However, once we have coroutine, this
becomes a `for` loop.
std::string buffer;
auto [ec, n] = co_await read_until(s, capy::dynamic_buffer(buffer), "\n",
2048);
Can be written as :
std::string buffer;
while (buffer.find('\n') == std::string::npos)
{
const auto prefix = buffer.size();
buffer.resize(prefix + 2048);
auto buf = capy::make_buffer(buffer);
buf += prefix;
auto [ec, n] = co_await s.read_some(buf);
buffer.resize(prefix + n);
if (ec)
co_return ec;
}
From my experience using asio for years, I can also say that I almost never
used read_until. It usually ended up being replaced because the matching
function wasn't ideal for many protocols. Mainly because the matching
condition (as it does in capy' read_until) always rescans the whole buffer.
Even in the example above it would be more efficient to pass `prefix` after
find to not scan through the whole string again.
I would also like to add that beast is providing it's own buffer types to
use with it's http functions. Dynamic Buffers may be a good idea for
certain protocols, but it is not at all evident to me that they make a good
generic concept. Rather I think that a dynamic buffer should be optimized
for it's protocol.
But, because we have `read_until` we now replace 10 lines of code with
multiple types and concepts that I consider unnecessary.
It seems yet another design decision based on "asio did it" that doesn't
seem to have reevaluated the changed conditions.
## 5. capy's non dynamic buffers
First of all, I agree with asio's decision to use a pair of `void*` and
`size_t`,
because memory is untyped. So those two are correct.
However, it seems capy is copying `asios` buffer sequence without
reconsidering the following new circumstances:
If I have a function accepting a `span<const_buffer>` and I have a generic
buffer sequence I could do this:
some_buffer_sequence bs;
co_await write(s, as_span(bs));
Where span returns a type that can be converted to the span.
Because the `co_await` expression is - as one might guess - an expression,
the lifetime of the container referenced by the span will be destroyed
after the coroutine is returned.
Thereby we could avoid templating the write function over the buffer
sequence and reduce compile time and complexity. An IO object could also
communicate through it's API if actually supports vectorized IO or not. The
buffer types could also just wrap around the OS buffer types (e.g. iovec)
and thus avoid any copying.
If a stream does not get templated on the buffer_sequence, but just takes a
span,
implementation of other stream types gets much simpler, too. This is
important if capy wants to establish it's stream concepts as the de-factor
or actual standard.
To be clear: I am not saying that a `span` is the right solution, but there
are simpler solutions that can have a concrete type. Asio's buffer
sequences need to work with asynchronous completion that do not get
`co_await`ed and thus has to solve lifetime problems, that capy hasn't. The
design of capy does not seem to consider this subtle difference.
## 6. capy::any_executor
I don't understand why there's an `executor_ref` and an `any_executor`. If
the `any_stream` support non-owning mode, why couldn't the `any_executor`
follow the same model and we don't have an `executor_ref` ?
I know that `executor_ref` is uses in the tasks and must be cheaply
copyable, but why couldn't that use a single any_executor at the structure
allocated by `async_run` and then a `any_executor&` in the task.
At the very least, there's an inconsistency between `any_executor` and the
`any_stream` types.
## 7. capy::execution_context
Next, the `capy::execution_context` is odd. It is only used by `corosio`,
however it is part of any `executor`.
However, it turns out that the `execution_context` can be default
constructed but is completely useless unless constructed as the parent
class of corosio::io_context? Why is that?
In asio, I can default construct an `execution_context` and it'll spawn off
a thread for the reactor and it'll work great. Likewise the
`asio::thread_pool` will work, where as capy::thread_pool will not allow me
to create a corosio::tcp_socket.
If the `execution_context` must not be used this way, the constructor
should be `protected`. I would very much prefer it behaved like `asio`,
because that is required to make the asio bridge I wrote work.
## 8. capy::IoAwaitable
The IO awaitable protocol is an interesting idea of how to solve the
executor propagation problem. However, it has one issue: not every waitable
is asynchronous. That means that not every awaitable will need to dispatch
back through the executor.
I think restricting `co_await` to just `io_awaitables` is too restrictive
and puts `capy::task` as `asio::awaitable`.
I don't think that proposing io_awaitable for the standard is an argument
either, since at this point regular `awaitable`s are the standard protocol.
I don't know how capy should fix it, but this is a missing feature, if
`capy` wants to a generic coroutine library.
`asio::awaitable` achieves a similar thing, by only allowing
`asio::awaitables` to await each other. That is fine, but it turns a
`capy::task` into something more specific, a coroutine type aimed towards
use with corosio.
## 9. capy::async_run
The `async_run` double invocation just looks weird. I get the intention of
setting the thread_local memory resource for when the task is created. I do
however think that this is a consequence of `asio did it` design. This
could be much more intuitive, if the `corosio::io_context` API changed to
this:
// we use a main task that can use wait_all & wait_any to manage other work
capy::task<double> main_task();
{
corosio::io_context ctx;
double res = ctx.run(main_task());
}
We would achieve two things: we got rid of the callback, which is generally
a good idea in a coroutine library, and we could bind the memory_resource
to the io_context. This would give us a simpler and much more intuitive API
and an implicit work guard with the `main_task`.
But alas, capy/corosio just copies `asio`'s API for `io_context` and
requires a call to `run`;
## 10. capy::io_result
`io_result` is not great, because std::get<> is not an extension point.
That means I can't use `std::tie`. The code examples use structured binding
for the `error_code`, but that means I will need to redeclare `ec` for
every operations.
const auto [ec, nr] = co_await read_some(...);
const auto [ec2, nw] = co_await write_some(...);
This needs to be solved.
