Re: Capy and Corosio review

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>
_______________________________________________
Boost mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://lists.boost.org/mailman3/lists/boost.lists.boost.org/
Archived at: https://lists.boost.org/archives/list/[email protected]/message/RRE5Z7UMK5MC3MDU2PSEVKCMUMBJ7OKH/