Hi all, the formal boost review of the async-mqtt5 library strats today,
On Wed, 16 Oct 2024 at 08:16, Klemens Morgenstern via Boost < boost@lists.boost.org> wrote: the 16th of October and will last until and including the 25th of this month.
<snip>
Please explicitly state that you either *accept* or *reject* the
inclusion of this library into boost. I believe I have spent close to 8 hours reviewing this library and discussing it on Slack with the author and other people. In the following I summarise the most important points of my review. 1. The lack of logging generates a lot of frustration to users. I suggest logging at least the following operations: resolve, connect, handshakes, reconnection retries and the size of reads and writes. It should be enabled by default. 2. This library is missing a very important optimization. It is not possible to publish more than one message with the current async_publish interface. For comparison, in Boost.Redis first you create a request with as many commands as you like and execute them with only one call to async_exec. I feel async-mqtt5 could do the same, for example request req; req.add(item1) req.add(item2) ... conn.async_publish(req, ...) 3. Again on performance and even more important than 2. If a call to socket.async_read_some reads multiple messages from the socket into the connection internal buffer, it should be possible to the user to consume them without starting an additional async_receive since calling an async_ function that completes immediately is very costly comparatively [1]. 4. This library should provide a type erased connection that uses any_io_exector and any_completion_handler to reduce compilation times. It could be provided as a separate compilation where the user has to include src.hpp in one of its source files. With this scheme you can also introduce enable or disable macros that exclude e.g. Beast from the compilation. 5. I found it hard to follow the implementation and hope the author changes his mind and replaces callbacks with asio's coroutines. I find callbacks unmaintainable. 6. I am skeptical it is a good idea to trigger writes from async_publish calls. IMO the write operation should be owned by async_run just as async_read. This does not only provide a simpler implementation but also enforces executor correctness. 7. Does per-operation cancellation really make sense or only results in more complex? Likewise for allocator support. 8. I think this library should follow suit and rename mqtt_client to connection and the library itself should be renamed to mqtt5 (i.e. drop async- from its name). I recommend ACCEPT with conditions 1. and 4. The other conditions are strongly recommended though. [1] https://github.com/boostorg/redis/blob/develop/doc/on-the-costs-of-async-abs...