Re: [async-mqtt5] Formal review starts today

From: Ivica Siladic (ivica.siladic_at_[hidden])
Date: 2024-10-24 14:28:27

Thank you sincerely, Vinnie, for your detailed review and the considerable effort you’ve put into understanding how MQTT works! We're, of course, really happy that you believe the library should be accepted into Boost.

> On 23.10.2024., at 22:51, Vinnie Falco via Boost <boost_at_[hidden]> wrote:
> 
> On Tue, Oct 15, 2024 at 11:16 PM Klemens Morgenstern via Boost <
> boost_at_[hidden]> wrote:
> 
>> Please explicitly state that you either *accept* or *reject* the inclusion
>> of this library into boost.
>> 
> 
> What follows is my review of the async-mqtt5 library by Ivica Siladic
> (through Mireo).
> 
> I believe this library should be ACCEPTED.
> 
> I spent about 20 hours looking at the documentation, the source code,
> Internet resources related to the MQTT protocol, and talking with the
> author. I was not able to compile the library and examples, for reasons
> related to the dependency on OpenSSL.
> 
> In my opinion this submission represents the gold standard of library
> proposals. It has been deployed by multiple companies in commercial
> settings for over a year. It has received considerable field testing and
> bug fixes. While not perfect, the API clearly works and delivers value for
> users. In short, async-mqtt5 has proven itself outside of Boost.
> 
> The library could use improvement in the documentation, the example code,
> the APIs, and the implementation that would bring it up to an outstanding
> level of quality. I struggled quite a lot with deciding whether or not some
> of these improvements should be conditions of acceptance and ultimately I
> have concluded that Boost would be better with this library in it as the
> library is currently implemented, rather than not.
> 
> Writing out a list of conditions is laborious and I aborted two attempts at
> doing so. Furthermore, the nature of some of the improvements are quite
> complicated and require experimentation, measurement, and iteration. It
> doesn't make sense to hold out for a "perfect" submission.
> 
> Furthermore a good MQTT client implementation is necessarily complex; the
> interaction of reading and writing combined with quality of service and
> server behavior is tricky to get right. Optimizing such an implementation
> requires making a lot of correct decisions regarding tradeoffs. I have
> rough ideas for improvements, and only the author can explore them in a way
> that preserves the unique features that MQTT clients need.
> 
> 
> 
> ANSWERS
> 
> Q: Does this library bring real benefit to C++ developers for real
> world use-case?
> A: It obviously does. MQTT is the industry-wide peer to peer communication
> solution for the Internet of Things ("IoT"). This library has already
> benefited its users for over a year.
> 
> Q: Do you have an application for this library?
> A: I personally do not have a use for this library. However I will note
> that the use of async-mqtt5 would be a natural fit for a reimplementation
> of Beast's websocket chat server.
> 
> Q: Does the API match with current best practices?
> A: I personally am unfamiliar with MQTT and IoT best practices. Yet I am
> confident that it either matches or will match best practices, as the
> pressures from its commercial usage create the necessary incentives to do
> so. I have some concerns that the
> 
> Q: Is the documentation helpful and clear?
> A: The documentation needs a lot of work.
> 
> Q: Did you try to use it? What problems or surprises did you encounter?
> A: I wanted to build it locally but cmake and vcpkg conspired against me.
> My willingness to offer acceptance despite the inability to build the thing
> comes from the existence of actual users prior to submission.
> 
> Q: What is your evaluation of the implementation?
> A: The implementation is good, not great. More importantly, the author is
> skilled with Asio and he appears to have his wits about him so I would bet
> that the support and improvement for this library should it become part of
> Boost, will be very good.
> 
> Q: Do you have experience with asio?
> A: I am an expert with Asio, and I have over 30 years of experience writing
> concurrent network programs.
> 
> Q: Did you work with MQTT in the past?
> A: I did not work with MQTT in the past, but I have heard about it. Eight
> years ago Michael Caisse presented a talk at CppCon 2016 on it, yet the
> promised code was never published.
> 
> 
> 
> NOTES
> 
> The implementation looks very complicated, just like the MQTT protocol. I
> have confidence that it mostly does the right thing, since it is used in
> production. There are a lot of memory allocations and a lot of intermediate
> structures, and I wonder if this could be improved.
> 
> The implementation queues both incoming and outgoing packets, and these
> queues appear to be unbounded. My experience tells me this is a problem, as
> the lack of backpressure subverts TCP's application-level flow control. Yet
> I do not have evidence that this is a problem in practice. A lot of time
> spent in my review is struggling over whether to demand improvements here
> or to just let it be.
> 
> I do not know how MQTT is used in the IoT, and a refactoring of this
> library's APIs to handle backpressure would be an enormous undertaking. I
> have a lot of ideas about how to reduce the allocations, and make
> backpressure available to the application but these need experimentation
> and design work to fit into the protocol. I trust that the author will
> explore these in his own time.
> 
> The async_run interface might be unnecessary. A typical program using
> mqtt_client will always have an outstanding call to async_receive, and
> periodically call async_publish. In a fashion similar to beast websocket,
> it is possible to piggyback the low level reading and writing on top of the
> async_receive and async_publish calls. If there are no outstanding calls to
> those functions, then the implementation will effectively stop
> communicating. And this might be a good thing for better support of
> application level TCP flow control.
> 
> The library dedicates a lot of ceremonies to allocators, and I appreciate
> the effort. However there are no motivating use-cases and no measurements.
> Given that there are so many calls to allocate, in the current state I
> would not advertise the allocator support so much. A refactoring of the
> implementation to cause fewer calls to allocate and deallocate may make
> allocators relevant to performance. Disclaimer, I have also not done any
> measuring.
> 
> The async_receive API only returns one message at a time, which is probably
> inefficient. A better technique would be to read as much data from the
> socket into a large buffer as possible in a single I/O, extract all
> complete messages into a range, and return that as a batch to the caller. I
> am of course glossing over many important details such as QoS, the need for
> ACK packets in response, and so on, and this idea will need to be explored
> further and measured to determine if it is practical. Is the necessary? I
> don't know.
> 
> Header files should be included in the order of most specific to most
> generic. That is, library specific (async-mqtt5) headers first, then
> dependencies (asio, boost), and then standard library headers. This
> helpfully increases the chances of getting a compiler error when a header
> is missing.
> 
> None of the examples work when you run them, because the broker
> "<your-mqtt-broker>" does not exist. I'm not sure what will happen if you
> run them as-is, but I would guess that since the library does infinite
> reconnection attempts with no logging, it will appear to hang.
> 
> Publishing, receiving, and properties are core aspects of MQTT yet these
> words do not appear anywhere in the table of contents. Nor do the subjects
> appear by other names.
> 
> Using operator[] instead of get<> for properties is unusual, as this
> creates something which looks like an array where each index holds a
> different typed object. I'm not going to get too worked up about it, since
> according to the author properties are an advanced usage.
> The implementation is in sore need of comments especially in the more
> complex areas where it is not clear which executor or which allocator is
> being used.
> 
> Examples need to have command line arguments for brokers and credentials.
> Otherwise, default to HIveMQ's public demo broker here
> https://www.mqtt-dashboard.com/. Running the example with no arguments
> could show a "usage" string which shows the HiveMQ broker as choice for
> broker
> 
> Table of Contents needs to have "Reading Messages", "Publishing Messages",
> and "Using Properties" as list items.
> 
> The documentation needs a section which explains the MQTT protocol at a
> high level. How it is used, the client and broker roles, and links to more
> supporting documentation and information. This will help users who come
> across on async-mqtt5 that do not have any knowledge of the protocol, to
> determine if the library is a fit for their needs. One common opinion is
> that explaining MQTT in the documentation is out of scope. Is isn't
> strictly wrong for an introduction to the protocol to be delegated to
> outside sources, yet it is better for Boost if this is included.
> 
> More clear documentation regarding properties. How commonly used they are,
> why they are used, and especially to point out how the indexes to
> operator[] result in objects of varying types depending on the index.
> 
> More information in the documentation is strictly better than less. Things
> like design notes, rationale for trade offs, references to where the
> library is used, benchmarks, and so on all have value and including this
> distinguishes the library from its competitors.
> 
> As pointed out in other reviews, the beast dependency should be optional
> and the TlsContext specification should be better, or the feature of TLS
> should be refactored.
> 
> 
> 
> CONCLUSION
> 
> Overall this is a good, working library which has considerable room to
> become great. Boost is better off with it than without it.
> 
> Thanks to the author for investing the time to prepare the library for
> review.
> 
> Regards
> 
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Thanks.
Ivica Siladic