Boost.Capy and Boost.Corosio Review ResultRecommendation I recommend that *Boost.Capy and Boost.Corosio be conditionally accepted into Boost.* From my point of view the review participation was at the minimal tolerable participation level for acceptance. I did my best to try and recruit external participation to little avail. Many thanks to all the reviewers, but especially a few that went really deep to expose issues. The libraries of course comply with the basics of a good boost library in terms of layout testing and documentation. There were several testimonials to production utilization so speaks to usefulness. I think it's also clear that the library space in support of coroutines is disjointed -- and so this work makes a good contribution for users desiring coroutine-only concurrency solutions. The numbers look like: For *Capy*, *10 *primary reviews. - 5 ACCEPT - 3 CONDITIONALLY ACCEPT (1 changed from initial reject) - 2 reviews without a verdict - 1 private review without a verdict For *Corosio*, *7 *primary reviews. - 4 ACCEPT - 2 CONDITIONALLY ACCEPT - 1 review without a verdict To me, like with the original ASIO review, this represents a useful starting point - not an end. As the libraries evolve I would suggest repeated reviews diving into particular topics. Some of which I'll enumerate here. Context & Process I realize this took an extended period of time, but please understand that my process involved not just reading the reviews, but reading all of the documentation, writing code, and running tests. And, in particular, thinking about the design details and their overall sufficiency. My biases as a review manager include: 1) being the review manager of asio 20 years ago -- and having used it extensively and to this day in production code. The asio review was controversial and there were many doubts -- for me especially about the complexity. However, in the end -- that decision was clearly the right one in my view. 2) In my professional work having helped design and build a coroutine framework using elements of std, asio, beast, and our own. This framework is dramatically less ambitious than these two libraries and built strictly to need. 3) I'm not an author of any of P2900 or the follow-on works. But because of years of work on the committee, I have reviewed all the papers and appreciate what it is attempting to achieve. I have a working knowledge of the elements. For me, while Capy shares goals and elements of std::execution it takes a clear line of separation with the coroutine-only design. And I think it's perfectly fine for those two things to coexist in the world -- ideally sharing as much as possible. 4) My motivation for doing this work remains unchanged: to serve the best interests of the c++ community as best I can. In the following sections I will outline the recommended changes. In some cases where it's not clear the path forward I'll attempt to motivate further discussion. With the amount of discussion and material I'm sure I've made some incorrect recommendations here which we'll correct via discussion. Acceptance Conditions 1. Documentation rework There's a lot of documentation here. Several reviewers noted the difficulties with the docs -- especially in understanding the Capy design -- and I concur. It's possible some of these were already addressed as I saw some PRs happening during/after the review. Overall, this is unsurprising to me as async coding is really really hard. There's a lot of complex moving parts including threading and synchronization. And there are at least three audiences: - ordinary Capy/Corosio users; - authors of higher-level libraries using Capy; - authors implementing the coroutine/environment protocol for their higher level library The third audience in particular needs normative-level documentation of the rules. If it's there, it didn't really come through for me and clearly for one reviewer that attempted to us Capy directly for his use case. Some specific recommendations from various reviewers and myself: - Consider moving the Introduction to concurrency and C++20 coroutines outside of this documentation. While it's refreshing to see the tutorial on these things it distracts from the goal of these docs which is learning the library. Most of the topics discussed there (aka futures) aren't ever seen again in most of the library docs. Or at least move all that to the bottom so it's not 'the first section you read' by following the links. - Provide bare bones mental models from the various user perspectives up front or in the examples section. When looking at examples, things like 'strand' appear without any introduction. Since the Capy default is a simplified model I think that can be explained. I feel like a UML sequence diagram would bring a ton to understanding the component interactions. - Capy buffer sequences section seems odd in that none of the sub contents are shown in the TOC even though it's the same depth as the other sections. - Reduce the duplication of text and enhance the linkages. - Try to provide search even if it just run google search on the domain like cppreference. - Where is the linkage to Capy performance benchmarks? (see more in performance below). - The design section has 'Why not Cobalt?', 'Why not TooManyCooks', etc -- given where we are there should be 'Why not std::execution'. And while we're there there's an example that bridges the models users might be coming with some knowledge of std::execution. Pointers to papers that describe this are a valid approach. - thread_pool::destructor --> " Signals all worker threads to stop, waits for them to finish, and destroys any pending work items". I couldn't figure out what this means. Obviously it's calling the base class destructor which does shutdown() destroy sequence -- but that didn't clarify either. - io_result needs to be front and center in the Capy docs -- it is in the Corosio docs, but it's buried in the task section of the Capy docs. That is, of course, partially how it is used as an unnamed return value split into elements. Comments in some early examples with a link to the reference could help. - Capy: add a lifetime and ownership section covering the issues that users need to understand when composing Capy coroutines. - The consuming_buffers is listed twice in the index for Capy types - The Corosio Quick Start is at the end instead of the front? - The Corosio docs have a helpful Glossary, Capy does not. - The reference guides should break the typelists into layers/sections like: promise_types, buffer_types, stream_types, synchronization_types. Similar for Corosio. See also discussion of io_task versus task, etc in Minimalism. - Corosio: there is a lot of duplicate information in the documentation in the networking guide tutorial - Corosio: address the documentation comments in this review: https://lists.boost.org/archives/list/boost%40lists.boost.org/message/KHNY67... 3. Capy Naming - Concepts in Capy are mostly in CamelCase -- except for a couple that are inconsistently snake_case (*decomposes_to). - mutable_buffer_archetype_/ const_buffer_archetype_ -- what does the underscore mean? My take is that it's meant to be private and you use the aliases without underscore. 4. Corosio TLS TLS facilities should either be fully implemented and documented as safe for their advertised use, or unsupported/incomplete security functionality must fail safely and consistently rather than silently degrading security. 3. Performance and BenchmarksObviously the point of these libraries is high performance. During the review there was some extensive discussion of benchmarks, but there's nothing really for Capy -- just Corosio to Asio. But since Capy is in part a framework, maybe there could be a site for contributed benchmarks for libraries that build on Capy. On the Corosio side make published performance measurements reproducible and current, with sufficient information to understand the comparison. Required Discussion/Resolution before acceptance 1. Capy Design - Minimalism There's a significant discussion about pulling in various parts of Boost into Capy when they aren't required. There was some back and forth, but I found the results inconclusive for me to make a final directional suggestion beyond DynamicBuffer removal. This really needs a serious revisit to ensure unneeded features aren't shipped. It would be interesting to take the learnings from the downstream efforts outside what parts of Capy were actually needed. Let's talk about Capy::quitter versus task, io_task etc. When I started looking at examples I was perplexed about why I would choose one over the other and how I would integrate signal handling to do something useful like read several files in parallel. Then I found, not in the docs, but in the examples from the repo the 'quitter' example -- pretty much exactly the kind of thing today we'd write using asio. And if I'm just using coroutines it's clear from the example that outside of task Capy has other not well documented 'toolkit elements'. Looking at the code in the quitter example, well I wouldn't have guessed how to write that and the asio code would be more concise. So I think this goes to the heart of the question: why leave this stuff in if it's not really documented? And the reality of the split means that to do the asio equivalent for signal handling I have to write my own fairly ugly code as demonstrated in the example. 2. Overall Design - Splitting the Libraries The consensus seems to be that the separation of the libraries makes sense and I concur. However there are a number of detailed questions about the location of some facilities such as timing primitives that should be addressed. There's also an argument to break things down even further to allow reuse of useful synchronization primitives depending on all of Capy. *1a* Corosio: Delays and Timeouts There was discussion on this point including why this was a Corosio primitive and not in Capy. And the fact that there's allocations, etc. Being able to co_await on timers seems fundamental to me -- especially in a combined operation which is waiting on io or a timeout. *1b* Capy thread_pool What's wrong with boost.thread thread_pool was asked somewhere? Well really this type isn't so much a thread pool as an execution context (the base class). I'd suggest renaming to thread_pool_context or threaded_context clears up the confusion here. *1c* Adding to Capy - io_context and signal handling? From my professional experience writing servers purely with coroutines the io_context is the key to making signals, networking, file-io all go together smoothly. As such it seems like that fundamental construct should be in Capy. Let's take a use case of writing a command line tool to read a directory of files where I read and run a regex across each one to see if there's a match. Of course the user should be able to interrupt cleanly at any time. So we need a signal handler and a stop token to cancel work. Right now I think I have to use Corosio to do this when it seems this should be Capy only. This is basically the analog of an issue identified by another reviewer. Overall handling signals in a coroutine server is pretty essential and seems to be roll-your-own awkward for Capy. See also *Minimalism *section discussion of Capy::quitter. The Corosio io_context is really a simplification of the asio io_context the traffics directly in coroutine handles AFAICS. I'm unsure if this means Corosio's io_context should be redesigned and moved or if Capy should just have an independent facility. Of course feel free to explain why this isn't needed at all. *1d* splitting Capy into more parts I think there's an argument for this going forward, but I wouldn't push for this in particular right now. There are things like Capy::quitter (see below) that are promise types and buffer types that don't depend on the coroutines really. But I follow that there's a critical mass. Some of these smaller synchronization things might be good to push to the standard library. 3. The buffer problem I'm sure the following will cause a substantial amount of potentially harsh discussion (please start a separate email chain). However, I'm unpersuaded that the current design handles the buffer abstractions and buffer concepts correctly. (Note that reviewers in 2005 weren't all convinced of the asio design either). At a minimum I think this is a missed opportunity. While Capy has buffer concepts they are really less developed than the ancient boost.iostreams. In the end the concepts go unused in the Capy core and instead the asio derived mutable and const buffer types become the foundation. I'm sure there was a reason, but it feels ill advised. These two types are actually minimalist versions of std::span. They are pointers and size to the actual buffers with data. So for me at least this is an api usability issue: std::array<char, 1024> buf; auto buffer = capy::make_buffer( buf ); // make a view of the buffer not an actual buffer While I acknowledge Klemans comments in the discussion of this point, it is an unfortunate reality that some views necessarily become owning. Regardless, I'd be much happier if this api used the terms buffer_span or buffer_ref to clarify what the types mean. Even though that breaks from asio tradition the concept of a span was developed later. Now let's look at the actual dimensions of 'memory buffers' in programs. Dimension Examples Storage contiguous / scatter-gather Capacity fixed / dynamically extensible Access mutable / immutable Ownership unique owning / shared owning / non owning (view) Flow Direction source / sink / duplex Range type input, seekable, random access Consumption partial / all / incremental Lifetime ephemeral / retained There may be more, but I think a set of buffer concepts expressing many/most of these dimensions would be useful in expressing behaviors. Downstream in Corosio, of course, we find buffers that depend on these sorts of capabilities. Future considerations 1. A plea for a more interoperable ecosystem My future desire would be that Capy and Corosio continue exploring the concepts and factoring needed to provide greater interoperability with other libraries. This doesn't require every library to directly support every other library. It means finding small, useful boundaries where efficient adapters are possible, allowing applications to combine facilities from Asio, Cobalt, Capy, Corosio, std::execution, and future libraries -- rather than forcing a choice among completely mutually exclusive stacks. I believe the reviewers provided some hints and suggestions for directions, but as Peter Dimov pointed out it's a difficult problem that remains unsolved. Thanks all, Jeff