Hi everyone. This is my review of the proposed Capy and Corosio libraries. I'm part of The C++ Alliance. That being said, I've tried to review the libraries as strictly as I would any other submission. The questions ============= 1. Usefulness ------------- Capy is genuinely useful as a foundation: There is no standard and no Boost answer for a coroutine-native I/O substrate, and the authors note that there is an HTTP library built on Capy without Corosio, which suggests the abstraction stands alone (I did not verify this, as that library was not part of the submission; what I can confirm is that Capy's concepts carry no dependency on any Corosio header). The buffer algorithms are a real ergonomic improvement over Asio. Corosio is useful as an end-to-end demonstration, and its coroutine echo server is concise, but it can't go unnoticed that, as a successor to Asio, it is materially incomplete, and its TLS is not currently safe against the public internet. In short, it is an early networking library, with the *potential* to eventually replace Asio. 2. Design --------- The core is strong and internally coherent, and the Capy/Corosio split is principled and the cleanest part of the architecture. The one weak point I found is the `run_async(ex)(task())` two-call idiom: The rvalue-qualified, non-movable, `[[nodiscard]]` wrapper correctly prevents storing-and-reusing the wrapper (which would break the TLS/LIFO discipline); but it does not protect the other invariant of the idiom: preconstructing the task: ``` auto t = make_task(); run_async(ex, my_pool)(std::move(t)); ``` silently allocates that frame from the wrong allocator. Compare that to Asio's single `co_spawn(ex, task(), token)` call. 3. Implementation ----------------- The quality is high: Disciplined lifetime/ownership handling, correct symmetric transfer, careful frame-allocator handling. My verification repeatedly found the hard cases handled correctly (e.g., the epoll speculative-read pattern). Nonetheless, I think I found some defects, concentrated in teardown and the platform backends (exactly where the tests are thinnest): - In strand_service.cpp:173, there seems to be a null dereference on `service_` during context teardown. Posting work to a strand is a two-step, non-atomic sequence: `enqueue()` locks the strand's impl mutex, queues the continuation, and, if the strand was idle, sets `locked_ = true` and returns `true` to signal "you must now spawn an invoker"; the caller (`post`/`dispatch`) then releases that lock and calls `post_invoker()`, which creates the invoker coroutine. The invoker's `operator new()` reads the strand's owning service via `impl->service_.load()` and immediately dereferences it. Meanwhile `shutdown()` walks every impl under its locks and stores `nullptr` into `service_`. Nothing serializes these two paths across the gap between `enqueue()` returning `true` and `post_invoker()` running. So: a) Worker thread calls `post()` -> `enqueue()`: locks the impl, queues the work, finds `locked_ == false`, sets it to `true`, returns `true`, releases the lock. It is now committed to calling `post_invoker()` but hasn't yet. b) Teardown thread runs `shutdown()`, reaches this impl, sets `locked_` and stores `nullptr` in `service_`. c) Worker thread resumes: `post_invoker()` -> `make_invoker()` -> `operator new()` loads the now-null `service_` and dereferences it. - io_uring `drain_cqes_for` never decrements `io_uring_inflight_`. Unlike `process_completions`, which decrements the inflight counter on each terminal/cancel CQE (corosio/include/boost/corosio/native/detail/io_uring/io_uring_scheduler.hpp:1115, :1124), `drain_cqes_for` consumes CQEs via `io_uring_cq_advance` without ever decrementing `io_uring_inflight_`, including the cancel SQE it submits itself (`inflight_inc()` at :1262). - IOCP `ready_` is a plain `long`. The field (corosio/include/boost/corosio/native/detail/iocp/win_overlapped_op.hpp:52) is accessed via both `InterlockedCompareExchange` (win_scheduler.hpp:350, :579) and a plain store `op->ready_ = 1` (win_scheduler.hpp:366), and the result fields `dwError`/`bytes_transferred` are published without release/acquire ordering. This is fine on x86 (TSO + locked instructions) but an ordering bug on ARM64 (a supported target). - `circular_dynamic_buffer::prepare(0)` causes a division by zero on a zero-capacity buffer. - Finally, POSIX signal handling is not async-signal-safe (corosio/include/boost/corosio/native/detail/posix/posix_signal_service.hpp). This is documented, but it's a regression against Asio, which performs only an async-signal-safe `write()` to a self-pipe (or uses `signalfd`) in the handler and does the real work on a normal thread. 4. Documentation ---------------- The documentation is extensive and strong. It is organized as a coherent multi-tier Antora site - introduction, tutorials, a topic-by-topic user guide, a generated API reference, and a design-rationale section - with clean navigation and a glossary. The guide chapters are the high point: pages such as `corosio/doc/.../4.guide/4c.io-context.adoc` and the Capy `9.design/9a.CapyLayering.adoc` explain the model concretely, with code that compiles against the actual API. Documented behavior I spot-checked matched the headers - for example the `io_context` default concurrency hint (`std::max(2u, hardware_concurrency())`) and the single-threaded-mode trigger are described exactly as the code implements them. The testing toolkit is documented unusually well (`7.testing/*`), with clear explanations of `run_blocking`, the `fuse` error-injection harness, the mock streams/sources/sinks, and `bufgrind`. A particular strength is honesty about incomplete features. `corosio/doc/.../4.guide/4l.tls.adoc` carries an "Implementation status" warning that enumerates every `tls_context` setting that is currently accepted but not wired up - including that `set_default_verify_paths()` leaves an empty trust store and "cannot verify a public server," and that `set_verify_callback()` "fails to link." That is exactly the kind of candid status disclosure a reviewer wants to see. Nitpick: The warning in the TLS guide is not mirrored in the HTTPS-client tutorial, which calls `set_default_verify_paths()` and `set_verify_mode(peer)` (3.tutorials/3b.http-client.adoc:278-280). 5. Did I try to use the libraries? ---------------------------------- I did not compile and run them. I read them as an implementer and verified a large number of source files line by line. 6. Should the libraries be accepted into Boost? ----------------------------------------------- My vote is *conditionally accept*, conditioned on fixing the issues above. 7. Do they fit the Boost ecosystem? ----------------------------------- I think so. Although the libraries are proposed to replace parts of Boost, I believe this is a legitimate thing to do. 8. API, naming, usability, extensibility ---------------------------------------- - I'm not fully convinced about the concept naming: There are seven stream/buffer concepts -`ReadStream`, `ReadSource`, `WriteStream`, `WriteSink`, `BufferSource`, `BufferSink`, `Stream`- and the `Source`/`Sink` suffix seems to name two unrelated things. `ReadSource` and `WriteSink` are the *complete-I/O refinements* of the stream tier - `ReadSource` refines `ReadStream` and adds `read()`; `WriteSink` refines `WriteStream` and adds `write()`/`write_eof()` - both still over caller-owned buffers. But `BufferSource` and `BufferSink` carry the same suffix while modeling a completely different, callee-owned-buffer API (`pull()`/`consume()` and `prepare()`/`commit()`/`commit_eof()`) and refine nothing in the stream hierarchy. P.S.: In both libraries, the license file should be named "LICENSE_1_0.txt". -- Gennaro Prota <https://prota.dev>