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.