Capy review

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_to_resolved_addresses,
https://master.corosio.cpp.al/corosio/3.tutorials/3c.dns-lookup.html#_connecting_to_resolved_addresses
* 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_candidates
* 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/experimental__wait_for_one.html
[2] https://master.capy.cpp.al/capy/4.coroutines/4f.composition.html#_errors_do_not_win_wait_for_one_success
[3] https://master.corosio.cpp.al/corosio/reference/boost/corosio/cancel_at-02.html
[4] https://www.boost.org/doc/libs/latest/doc/html/boost_asio/example/cpp20/channels/mutual_exclusion_1.cpp
[5] https://github.com/boostorg/redis/blob/corosio/include/boost/redis/detail/flow_controller.hpp
[6] https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-COPY
[7] https://github.com/anarthal/nativepg/blob/b33026688885475538a07222b6639a1ed45faf50/include/nativepg/co_connection.hpp#L106
[8] https://www.boost.org/doc/libs/latest/doc/html/boost_asio/reference/Disposition.html
[9] https://github.com/anarthal/mycosql/blob/485bb4ec932955f4256e1ac65bc95395a67db45e/src/mycosql_internal/connection_pool/connection_node.hpp#L52
[10] https://master.corosio.cpp.al/corosio/4.guide/4b.concurrent-programming.html#_strands_in_corosio
[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#_one_coroutine_per_connection
[13] https://master.corosio.cpp.al/corosio/4.guide/4d.sockets.html#_the_io_stream_interface
[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_nonblocking_c_api
[16] https://lists.boost.org/archives/list/[email protected]/thread/OPE4XC5NOKJKERKAKFHG23A5MJGHNGG3/
[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_stream.html
[20] https://github.com/boostorg/redis/blob/develop/doc/on-the-costs-of-async-abstractions.md
[21] https://github.com/cppalliance/capy/blob/9144290189fa149b27617c7d9a476c8fbffb7b8c/include/boost/capy/read_until.hpp#L323
[22] https://isocpp.org/files/papers/P4172R1.pdf
[23] https://github.com/cppalliance/capy/blob/9144290189fa149b27617c7d9a476c8fbffb7b8c/src/ex/detail/timer_service.cpp#L33
[24] https://master.corosio.cpp.al/corosio/4.guide/4l.tls.html#_hostname_verification_sni
[25] https://docs.openssl.org/3.3/man3/SSL_CTX_set_tlsext_servername_callback/#description
[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/cancellation.html
[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
_______________________________________________
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/CHOVX6QIHYUF3XLUFXQGXROICOPMKP7Q/