[async-mqtt5] Mohammad's review

From: Mohammad Nejati [ashtum] (ashtumashtum_at_[hidden])
Date: 2024-10-23 13:54:17

Hi everyone,

This is my review of the proposed async_mqtt5 library.

First, I want to thank the Mireo team for submitting this library and
Klemens for managing the review process.

> Does this library bring real benefit to C++ developers for real world use-case?

Developing a high-quality MQTT client library is no small task, and it
is not something everyone wants to take on alongside their main
project. Having a well-implemented and tested library would be highly
valuable for C++ developers.

> Do you have an application for this library?
> Did you work with MQTT in the past?

I have limited experience with MQTT from a previous job where I used
the mqtt_cpp library to transmit and receive telemetry data in the
telecommunications industry.

> Does the API match with current best practices?

The API simplifies library usage by providing high-level features,
such as automatic reconnection, which is often essential in platforms
where MQTT is used. The initiator functions adhere to Asio best
practices, allowing users to provide their own executor and allocator,
and include proper cancellation support.

I believe there’s room to add interfaces for bulk receive and publish
operations, which could enhance performance by minimizing
rescheduling. This could be considered in the future based on user
demand.

`mqtt_client` is templated on the stream type and TLS context, which
isn't inherently a problem, but it has made Boost.Beast a required
dependency, which feels like a design flaw. I suggest abstracting the
stream type and its required functionalities into a separate type,
allowing users to just include the required implementations of it.

I believe `mqtt_client` can type-erase the stream type without any
measurable performance impact. All operations on the stream occur at a
low frequency, so one level of indirection shouldn't matter. The only
higher-frequency operations are `async_write` and `async_read_some`,
but since `async_write` uses scatter/gather pattern and passes all
buffers in a single call, and `async_read_some` reads into a large
buffer, the number of calls to these functions will still be low.

I didn’t find the `mqtt_client::brokers` interface very elegant; it
could accept a range of hosts and ports and delegate the parsing of a
comma-separated list to the users.

> Is the documentation helpful and clear?

While the documentation does a great job explaining design decisions
and corner cases, I'd love to see a beginner-friendly section (like
the example in the introduction) that walks through a few examples
step by step. This is likely the most-read part of any library
documentation and deserves more attention.

> Did you try to use it? What problems or surprises did you encounter?

I've tried experimenting with the provided examples on a locally run
mqtt broker.

The obvious problem was that there is no error reporting when things
go wrong. This is a side effect of having infinite internal
reconnections and no mechanism for reporting them. A logging solution
or callbacks for reporting problems would be necessary.

> What is your evaluation of the implementation?

The implementation has a high quality and shows the authors have a
very good understanding of Asio and asynchronous programing. Other
than the mentioned issue with stream type, I couldn't find any
significant problem during the review.

I suggest specializing asio::associator instead implementation
associator functions one by one:
https://www.boost.org/doc/libs/develop/doc/html/boost_asio/reference/associator.html

Instead of using the associated allocator of the completion handler to
allocate the internal queue, it might be possible to use a form of
circular buffer to eliminate the need for frequent memory allocation
(this might also fix the total cancellation problem in async_publish).

> Do you have experience with asio?

Yes, I've been a user of Asio and currently maintain Boost.Beast.

> How much time did you spend on this review?

I spent about 18 hours reading the documentation, reviewing some parts
of the implementation, and experimenting with the interface.

> Final decision and comments

I vote that we conditionally accept `async_mqtt5` into Boost with the
following criteria:

- Break the hard dependency on Beast and TLS stream by either
abstracting the stream type and placing different implementations in
separate headers, or by type-erasing the stream type and constructing
the connection through a factory. The type-erasure method is
preferred, as it can reduce compile times and simplify usage.

- Provide a mechanism for reporting connection errors and reconnection
attempts to the user.

- Set limits on all internal queues and allow TCP's back-pressure to
regulate the speed of the producer and consumer sides.

- Introduce address, UB and memory sanitizer to CI builds.

Thanks,
Mohammad.