Re: [async-mqtt5] Review questions

From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2024-10-20 17:06:27

On Sat, Oct 19, 2024 at 4:43 PM Ivica Siladic via Boost <
boost_at_[hidden]> 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/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/mqtt_client.hpp#L41

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/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/mqtt_client.hpp#L82

3. Why bother with this overload? Users can call ctx.get_executor(), why do
it for them?
https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/mqtt_client.hpp#L114

4. What if `other` is performing operations? The documentation does not
explain it.
https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/mqtt_client.hpp#L135

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/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/impl/client_service.hpp#L428

6. This "will" look like an MQTT protocol thing. The documentation could
explain it better:
https://github.com/mireo/async-mqtt5/blob/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/mqtt_client.hpp#L242

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/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/mqtt_client.hpp#L242

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/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/mqtt_client.hpp#L296

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/0b935bd1a2ba65427d1fb491aa1133b07c076cd6/include/async_mqtt5/mqtt_client.hpp#L576
(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