On Sat, Oct 19, 2024 at 4:43 PM Ivica Siladic via Boost < boost@lists.boost.org> wrote:
...
Ivica, I have some questions :) My first step was to audit the file mqtt_client.hpp by inspecting everything from top to bottom: 1. Where are the type requirements for TlsContext explained? https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... 2. Allocating in a constructor is OK, and the documentation should communicate this and the possibility of an exception: https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... 3. Why bother with this overload? Users can call ctx.get_executor(), why do it for them? https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... 4. What if `other` is performing operations? The documentation does not explain it. https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... 5. The destructor ~mqtt_client calls impl_->cancel(), which is a non-trivial function which can throw. Submitting work to an executor from a destructor is surprising: https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... 6. This "will" look like an MQTT protocol thing. The documentation could explain it better: https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... I found this from searching: https://www.hivemq.com/blog/mqtt-essentials-part-9-last-will-and-testament/ 7. The word "will" appearing three times in this declaration looks unhealthy. The function name doesn't make its operation clear: https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... 8. The ad-hoc format of putting a list into a string is a little weird, why isn't this some kind of range? https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... 9. I found the "TlsContext" concept documentation. It completely omits any requirements, which means that TlsContext is not a real concept. Using std::monostate is weird, why not void? How do you feel about using this declaration for mqtt_client instead? template < typename StreamType, bool enableTLS = false > class mqtt_client; 10. async_subscribe requires a reference to a vector. And the call to `async_initiate` makes a copy? Why not pass the vector by value and then move it? This would allow specifying an initializer list at call sites to async_publish without making an extra copy: https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c... (of course, my C++ is rusty so I could be wrong here) 11. Everything in this document should be in the official library documentation, and a permanent part of the library. Not just for reviewers. This information is very helpful for people who are evaluating whether or not to adopt the library for their needs instead of other offerings: https://www.mireo.com/cdn/Async.MQTT5.pdf 12. The documentation alludes to some kind of queuing or buffering of outgoing messages, to handle the case for recurring disconnections (which are frequent in IoT deployments): "While offline, it automatically buffers all the packets to send when the connection is re-established." Does this mean that mqtt_client objects can consume unbounded memory? What is the limit on the queue, or where are the APIs to control queue depth? How does the client eventually block callers when the queue is full, to prevent resource exhaustion? 13. I have the same questions as above, but regarding buffering for incoming messages. Are they buffered without bounds? What if someone calls async_run() without collecting any messages, will the memory consumption grow unchecked? I will have more soon. Thanks