|
Boost : |
From: Marcelo Zimbres Silva (mzimbres_at_[hidden])
Date: 2024-10-26 15:34:24
On Wed, 16 Oct 2024 at 08:16, Klemens Morgenstern via Boost <
boost_at_[hidden]> wrote:
> Hi all, the formal boost review of the async-mqtt5 library strats today,
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-abstractions.md
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk