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/associa... 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.