Re: [async-mqtt5] Mohammad's review

From: Ivica Siladic (ivica.siladic_at_[hidden])
Date: 2024-10-23 19:26:42

Hi Mohammad,

thanks a lot for your thorough review!

> On 23.10.2024., at 15:54, Mohammad Nejati [ashtum] via Boost <boost_at_[hidden]> wrote:
> 
> 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.
> 
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Ivica Siladic