Hi all, This is my formal review of the proposed Capy and Corosio libraries. I am currently affiliated with the C++ Alliance. 1. What is your evaluation of the usefulness of the libraries? Immense. C++20 coroutines offer a robust alternative to other async completion models, without their pitfalls and hacks (looking at you, callback hell!). Coroutines for I/O also feature a unique set of problems (are you _really_ sure that _all_ your handlers are going through the appropriate asio::strand?). Having a Boost-quality set of libraries that solve these problems is really useful. 2. What is your evaluation of the design? The design is sound and well thought. The rest of this point covers some of the key aspects, critical for my use cases. The IoAwaitable protocol ======================== It solves a real problem. It is the same problem that Asio's associated properties (executor, allocator and cancellation slot) try to solve. Asio needs to stay backwards compatible though, and the adopted solutions were constrained by this fact. The IoAwaitable protocol is the robust, clean way to solve this problem. In particular: * Having child coroutines inherit the parent's io_env by default and making hops explicit with run is IMO the right thing. * Properly using symmetric transfer to avoid stack exhaustion problems is critical. This is an immense source of trouble with Asio. The Executor concept is a logical advancement on top of Asio's. * Caring about allocations is important. Providing a default that outperforms the standard allocator is equally important. * Cancellation is a first-class citizen and uses the proper primitives. std::stop_token is the right choice because: ** It is thread-safe by default. Asio's cancellations aren't thread-safe by default. I can't count the number of users having bugs due to this when using mysql::connection_pool. ** It is stateful. Asio's cancellations aren't stateful by default - you need to explicitly store the fact that a cancellation was received, one way or the other. This results in you having to check for cancellations before each initiation, a common source of bugs. You don't need to do this with Capy/Corosio. ** There is a single cancellation type (against Asio's terminal, partial and total). This added IMO unnecessary complexity. * Work counting is acknowledged as something important. I wonder however whether the Executor concept is the right place for the work counting functions to be. Work counting looks like the responsibility of the execution_context, rather than the executor. Has it been considered to make these part of the execution_context interface? capy::task ========== I've used it with no problems. I appreciate the effort that has gone into optimizing it. In this line, I'd like to ask the authors whether they have any data on the cost of immediate completions when using task (rather than a custom awaitable). In Asio, immediate completions have a significant cost (as explained by Marcelo [20]). I've seen that Capy goes to considerable lengths to avoid tasks that complete immediately, making me think there is a considerable cost in Capy, too. Take read_until implementation as an example [21]. The library tries a sync completion, and falls back to creating a task only if the former fails. I think it would be useful to get some data on this. If the cost is enough, I'd like to see a utility to make the pattern in [21] usable by "Application developers" [22]. Something along these lines: task_or_immediate<int> queue::pop() { if (int v = try_pop()) return immediate{v}; return wait_and_pop(); // this returns a task } I have use cases for this in all my libraries: * connection_pool::get_connection() can complete immediately if there are unused connections in the pool. * Redis flow_controller (queue-like class) [5] can have their operations complete immediately if the queue is not empty/full (the common case). Combinators (when_any, when_all) ============================== Extremely important. In Asio, parallel_group has been experimental for years, so having a robust equivalent is critical. The architecture and semantics are sound. I would recommend adding a combinator with semantics equivalent to Asio's wait_for_one [1]. It would be similar to when_any, but cancellation would be triggered upon any task finishing, regardless of its result (whereas capy::when_any requires a successful completion to win). This can be built today with when_any, but is not very ergonomic [2]. Synchronization primitives (async_mutex and async_event) ======================== Vital for high-level libraries like Boost.Redis/MySQL. Writing co_await mutex.lock(); It clearly expresses intent. Compare it with the Asio equivalent [4]: // Claim the write lock by sending a message to the channel. Since the // channel signature is void(), there are no arguments to send in the // message itself. co_await write_lock_.async_send(); // Respond to the client with a message, echoing the line they sent. co_await async_write(socket_, "<line>"_buf); co_await async_write(socket_, dynamic_buffer(data, length)); // Release the lock by receiving the message back again. write_lock_.try_receive([](auto...){}); I would advise giving these more space in the docs, since async_event is only mentioned in the reference. I'd find useful a primitive akin to std::condition_variable, where a single waiter can be notified. I found this while implementing connection_pool in MySQL [9], where I need to wake a single "worker" when a connection is requested. I'm currently using a corosio::timer with an infinite expiry, which is a workaround. Buffers ======= Buffer sequences have a clear rationale and make vectored I/O approachable. As I mentioned in other posts, I would like to see better interoperability with std::span<Ch>, with Ch being std::byte or unsigned char. Ideally, the following should satisfy ConstBufferSequence: std::vector<std::span<const unsigned char>> This would simplify the interoperability of pure-protocol libraries with Capy. This concrete example is inspired by Postgres' COPY OUT functionality [6]. All my libraries use growable (dynamic) buffers. I initially tried to write Postgre's protocol handling code around Capy's DynamicBuffer concept and associated functions, but finally went with a custom buffer and plain read_some. The drawbacks I saw in DynamicBuffer are: * prepare(n) throws when requested more memory than the configured maximum. This is a situation that is not exceptional at all, and I manage it with an error code. * prepare(n) makes space for n bytes and returns exactly space for n bytes. MySQL and Postgres protocols feature many small messages prefixed by a byte length. I want to read at least n bytes, but I don't want to restrict myself to that - I want to pass the entire free area to read_some, to read as many messages as possible. * The library-supplied flat, growable buffers (vector_dynamic_buffer, string_dynamic_buffer) memmove their contents on every consume(). My experience is that this is an inefficient pattern, especially when dealing with many small messages. The buffer type is managed by my libraries, rather than my users, except in advanced cases. I don't think I gain much from sticking to DynamicBuffer, so I didn't. However, this may be an indication that DynamicBuffer might be a better fit for Boost.Http, rather than Capy. Error handling ============== Both libraries consistently use io_result, which contains a std::error_code. Using error codes and only error codes (vs. exceptions) is more verbose but probably the right decision. Code dealing with I/O should handle failures and cancellations, and they don't have anything of special. I'm a bit divided here, because supporting std::error_code only (vs. richer error types) makes reporting advanced diagnostics less ergonomic. Diagnostics need to be reported separately, instead of using the dedicated error channel [7]. Asio has the concept of dispositions [8]. It is a trade-off of generality vs. simplicity. All in all, I am happy with going forward with io_result. Having worked with Asio for this long, simplicity seems a good value to pursuit. Streams, sources and sinks ======= ReadStream and WriteStream are battle-tested concepts that Capy acknowledges. I've found them easy having an Asio background. I haven't come to use sources and sinks, but they look promising. For example, Postgres has a dedicated bulk copy protocol to transfer entire tables to/from the client, where they could fit: connection::copy_to_client(std::string_view copy_sql, WriteSink& dest); Allowing type erasure is a big improvement over Asio, as it allows shipping compiled libraries and hence cutting build times. I've found myself almost always using the non-owning version of any_stream, as I want to re-use my I/O objects across reconnections, and you need the concrete stream type to perform connection establishment. Transfer algorithms =================== I use capy::write() profusely - having it is a must. My use case (length-prefixed message protocols) looks like a good use case for capy::read (plain buffer version). Actually, the reference section lists this use case as example [18]. I'm not sure this pattern is efficient, though, as with most streams in Corosio you'd be running two syscalls per message. As an improvement, I could implement a wrapper stream that buffers bytes (à la Asio's buffered_stream [19]), but manual handling looks simpler to me. Also, read() returns a task, which I understand has overhead even when it completes immediately. I would consider whether capy::read() and capy::read_until() are really necessary at this point, as pointed out by other reviewers. Corosio io_context ================== I've always liked Asio's io_context, and I like seeing a proper io_context implementation here (vs. just capy::thread_pool), as it lets the user much more control. I'd like to see documented better what guarantees can I expect from executor affinity, especially when using multiple threads. My understanding is that executor affinity here does not buy thread affinity, and that coroutines may resume in any of the threads calling io_context::run() (like in Asio). This piece of documentation [10] states that the strand problem is mostly solved through executor affinity. This happens in Asio too ("implicit strand" IIRC), but has _many_ loose edges. Take the Asio's equivalent from [10] as an example: asio::awaitable<void> session(tcp::socket sock) { // All code in this coroutine runs sequentially auto [ec, n] = co_await sock.async_read_some(buf, asio::as_tuple); // No other code in this coroutine runs until above completes co_await sock.async_write_some(response, asio::as_tuple); // Still sequential } This is safe without strands even from multiple threads. However, if I now set a timeout with cancel_after, this is no longer safe because we have two parallel execution chains (async_read_some and the timer): asio::awaitable<void> session(tcp::socket sock) { // Race condition and UB auto [ec, n] = co_await sock.async_read_some(buf, asio::cancel_after(10s, asio::as_tuple)); } My understanding is that Capy/Corosio have gone a step further, and that this is safe: capy::task<void> session(corosio::tcp_socket sock) { // Safe on a single-threaded io_context auto [ec, n] = co_await capy::timeout(sock.read_some(buf), 10s); } If that's the case, having it documented would help IMO. As well as where the limits of this pattern are. For instance, I understand that the parallel fetch pattern proposed in the Capy docs [11] would require strands. So would a websocket connection that reads and writes data at the same time (even when using the "One Coroutine Per Connection" pattern [12]). A mention to capy::strand would be beneficial, since the docs seem to imply that strands are just superseded by executor affinity, which I don't think it's true. What's the behavior if I use more threads than the concurrency_hint passed to the constructor (with concurrency_hint != 1)? Corosio I/O objects only work with corosio::io_context. This may come to a surprise if you come from an Asio background, because asio::thread_pool works with all I/O objects, but capy::thread_pool does not. I'd advise mentioning this in the docs. Corosio sockets =============== I'm happy to see TCP and UNIX sockets, without templates and with semantics matching Asio's. I have had no problems using them. I was a bit surprised to see the io_stream interface [13], as it somehow looks to overlap with capy::any_stream (you may do polymorphism using both). The implemented functionality covers all my present use cases, but I think it may fall short on extensibility. There are _many_ socket types [14], most of them with very niche use cases. The library makes the right decision to support the most common subset. But it'd be good practice to provide an extensibility mechanism to allow users to interact with whatever socket types they may need ("open-closed principle"). For example, a stream_socket/datagram_socket base class may be provided. The native types indicate real industry needs and are a sign that the library is facing real use cases. They are however not covered in the docs. As I mentioned in previous posts [16], current utility of the tcp_socket::wait() function is limited. It is aimed at integrating with non-blocking C libraries, but many of these supply you with a file descriptor that you should poll. But there is no tcp_socket::assign() function to achieve it. My advise here would be removing the libpq example from the docs [15], as it is not implementable, and re-think how the integration would look like. I don't think copying Asio is the best choice here, as it leads to messy, brittle code [16]. I don't think this is core functionality though, and may be deferred as required. I found the tcp class surprising. It makes sense in Asio because sockets and acceptors are generic and templated on the protocol. This is not the case in Corosio. I recommend replacing the class by an enum. I found acceptors easy to use coming from an Asio background. I found tcp_server odd at first, but useful. My only suggestion would be to consider extensibility as mentioned above (having common code could simplify having a local_stream_server, for instance). DNS and connection establishment ================================ Again, simple enough. I'd consider making resolver_entry and reverse_resolver_result aggregate types. Having range connect is important. Is there a reason for supporting iterator-based overloads in C++20 (as opposed to using std::ranges::subrange [17])? Corosio timers, capy::timeout() and capy::delay() ================================================= There is a lot of overlap between capy::timeout, capy::delay, corosio::timer, corosio::cancel_at and corosio::cancel_after. First, there is need to have a robust way of waiting and setting timeouts to asynchronous operations. I use these two primitives heavily in all my libraries. They need to be provided, in one way or another. Second, I don't know if having these primitives in Capy is a good option. On the one hand, timeout() seems a foundational primitive, useful in almost all contexts, even those without I/O. On the other hand, timeouts are better implemented in the presence of a reactor. Having them in Capy, with today's architecture, implies an extra thread, at the very least. It is also not clear to me whether capy::timeout() and capy::delay() are safe to use with io_context with a concurrency_hint of one. Third, I find delay() and timeout() ergonomics much better than Asio's timer interface. They are stateless and express intent much better. I have been pointed out by Vinnie that they imply an extra allocation though [23]. My understanding is that this could be avoided by making the functions live in Corosio, even with the stateless API, so I'd recommend going down this path. I suspect one of the reasons for Asio's stateful timer interface is supporting cancellation, since timers predate per-operation cancellation (by a lot). While timer::cancel() is superseded by the stop token in io_env, timer::cancel_one() is not. I use the latter as a synchronization primitive [9]. I don't think that we need timer::cancel_one(), but a proper synchronization primitive that allows waking a single waiter (as opposed to async_event::set). capy::timeout() is missing an overload taking a time_point (equivalent to cancel_at [3]). TL;DR: my recommendation is to move timeout() and delay() to Corosio, remove timers and cancel_at/cancel_after, and implement a synchronization primitive in Capy akin to std::condition_variable. TLS === I have been able to use it effectively. I appreciate the effort to decouple the interface (tls_context and tls_stream) from individual libraries, as it makes it much easier to avoid hard dependencies. I appreciate having a function to set SNI (tls_context::set_hostname [24]), but I think it should be a member of tls_stream, rather than tls_context (matching what OpenSSL does [25]). Issue in [26]. I miss a way to access the native TLS context/stream handles. This is useful if you know that you're using a particular library. The OpenSSL API is huge, and Corosio doesn't (and shouldn't) provide support for most of it. Yet, it should provide an escape hatch for users that do need any of these options. My concrete use case here was using SSL_CTX_set_keylog_callback [27] to enable traffic visualization for a particular issue that only happened with TLS. Corosio should _not_ support this option, but allow an escape hatch to set it without modifying Corosio's source code. The current implementation has still some gaps [28] that should be filled before Boost acceptance. Note that I've used tls_stream with non-owning any_stream values only. I want to re-use the stream object (with tls_stream::reset). As part of a reconnection, you need to perform transport-level connection (tcp_socket::connect), which any_stream doesn't support. Casting any_stream objects is also not possible, because the type-erasing technique doesn't use inheritance. For this reason, I think that tls_stream::next_layer() is not really useful and should be removed. Asio integration ================ I think that this is an important topic and it is currently missing from the library. For me, a seamless Asio integration allows me to ship only a Capy/Corosio implementation in my libraries. Not having it forces me to ship two APIs until Capy/Corosio are sufficiently adopted. Ideally, users should be able to start using a Capy/Corosio-based library in their Asio applications without a full migration. 3. What is your evaluation of the implementation? I included all I had to say about the implementation in the corresponding point above. I haven't conducted an exhaustive review of the implementation, but peeked around the components that I have used. 4. What is your evaluation of the documentation? I miss the guarantees that Corosio's I/O operations give me upon cancellation. For example, if I run: tcp_socket sock; // ... connect the socket auto [ec, bytes] = co_await capy::timeout(sock.read_some(buff), 1s); And the operation is cancelled due to the timeout firing, what state is the socket left in? Can I issue another read_some operation, or do I need to close the socket and connect it again? I suggest using Asio's cancellation types (terminal, partial, total [29]) as cancellation guarantees. Note that I'm not suggesting adding support for requesting a particular cancellation type like in Asio, but to use these same terms to describe what to expect after a cancellation happens. I find the "Introduction to Concurrency" [30] section of the docs surprising, as it is not really related to the library. I'd move it to an annex. Some minor points: * These Corosio examples look like a good case for range connect: https://master.corosio.cpp.al/corosio/4.guide/4j.resolver.html#_connecting_t..., https://master.corosio.cpp.al/corosio/3.tutorials/3c.dns-lookup.html#_connec... * I'd advise recommending std::ranges::filter (rather than ConnectCondition) for this example: https://master.corosio.cpp.al/corosio/4.guide/4d.sockets.html#_filtering_can... * I'd advise to use C++20 std::jthread in the documentation and examples, rather than std::thread. * tcp_server::start() docs mention providing workers to the constructor (should be to set_workers, instead). 5. Have you used either or both libraries? What was your experience? I have written a Boost.MySQL Capy equivalent [31], a Corosio API for Boost.Redis [32] and a Postgres library [33] (still a work in progress). I have already described my experience above. 6. Are the libraries ready for inclusion in Boost? If not, what changes would you recommend before acceptance? My recommendation is to CONDITIONALLY ACCEPT Capy and Corosio into Boost, provided that: * Asio integration is properly implemented. * The delay/timeout/timer interface is redesigned. * The TLS implementation gaps are filled. Kind regards, Ruben. [1] https://www.boost.org/doc/libs/latest/doc/html/boost_asio/reference/experime... [2] https://master.capy.cpp.al/capy/4.coroutines/4f.composition.html#_errors_do_... [3] https://master.corosio.cpp.al/corosio/reference/boost/corosio/cancel_at-02.h... [4] https://www.boost.org/doc/libs/latest/doc/html/boost_asio/example/cpp20/chan... [5] https://github.com/boostorg/redis/blob/corosio/include/boost/redis/detail/fl... [6] https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-COPY [7] https://github.com/anarthal/nativepg/blob/b33026688885475538a07222b6639a1ed4... [8] https://www.boost.org/doc/libs/latest/doc/html/boost_asio/reference/Disposit... [9] https://github.com/anarthal/mycosql/blob/485bb4ec932955f4256e1ac65bc95395a67... [10] https://master.corosio.cpp.al/corosio/4.guide/4b.concurrent-programming.html... [11] https://master.capy.cpp.al/capy/8.examples/8g.parallel-fetch.html [12] https://master.corosio.cpp.al/corosio/4.guide/4b.concurrent-programming.html... [13] https://master.corosio.cpp.al/corosio/4.guide/4d.sockets.html#_the_io_stream... [14] https://man7.org/linux/man-pages/man2/socket.2.html [15] https://master.corosio.cpp.al/corosio/4.guide/4r.wait.html#_wrapping_a_nonbl... [16] https://lists.boost.org/archives/list/boost@lists.boost.org/thread/OPE4XC5NO... [17] https://en.cppreference.com/cpp/ranges/subrange [18] https://master.capy.cpp.al/capy/reference/boost/capy/read-00.html#_example [19] https://www.boost.org/doc/libs/latest/doc/html/boost_asio/reference/buffered... [20] https://github.com/boostorg/redis/blob/develop/doc/on-the-costs-of-async-abs... [21] https://github.com/cppalliance/capy/blob/9144290189fa149b27617c7d9a476c8fbff... [22] https://isocpp.org/files/papers/P4172R1.pdf [23] https://github.com/cppalliance/capy/blob/9144290189fa149b27617c7d9a476c8fbff... [24] https://master.corosio.cpp.al/corosio/4.guide/4l.tls.html#_hostname_verifica... [25] https://docs.openssl.org/3.3/man3/SSL_CTX_set_tlsext_servername_callback/#de... [26] https://github.com/cppalliance/corosio/issues/239 [27] https://docs.openssl.org/3.3/man3/SSL_CTX_set_keylog_callback/ [28] https://master.corosio.cpp.al/corosio/4.guide/4l.tls.html#_overview [29] https://www.boost.org/doc/libs/latest/doc/html/boost_asio/overview/core/canc... [30] https://develop.capy.cpp.al/capy/3.concurrency/3.intro.html [31] https://github.com/anarthal/mycosql [32] https://github.com/boostorg/redis/pull/417 [33] https://github.com/anarthal/nativepg