Hi,
On Sun, 27 Oct 2024 at 09:53, Darryl Green via Boost
wrote: The user is free to avoid queuing by only calling async_publish once the completion handler for the previous publication has been called. And one can manage that per topic if appropriate. The fact there is a convenience feature doesn't mean one has to use it but I found using the library to be a little surprising and lacking in transparency due to the lack of any notifications about connectivity or timeouts. This is another face of the "it needs logging" comments from other reviewers. If the user can be informed what the library is doing, then the user can simply log this, or, potentially, react in other ways.
I would like to mention that depending on how it is implemented, logging will also solve the "notification" problem. In Boost.Redis, for example, the async_run function gets a lightweight log object with the following interface class logger { public: void on_resolve(...); void on_connect(...); void on_ssl_handshake(...); void on_connection_lost(...); void on_write(...); void on_read(...); void on_run(...); void on_hello(...); void on_check_health(...); ... }; This is primarily meant for logging but can also be used to communicate events to the upper layer. I must admit though that I don't find this approach scalable, there are too many places that need logging and you don't want to extend the class with new functions every time. Also, there does not seem to be any use for this except logging, so why not use an interface with trace(), info(), warning(), critical() etc. instead.
- Does the API match with current best practices?
If we took Beast as an example of best practice, probably not.
I would like to understand how you come to this conclusion, because it is low-level Beast requires users to perform steps manually: resolve, connect, handshake, write, read etc. Also, if anything more advanced like http pipelining is needed you can expect to write a lot more code, at least compared to popular http libs written in go, node-js etc. The library being reviewed on the other hand hides all those steps plus the multiplexing of multiple async_publishes behind a single call async_run function, this is extremely convenient IMO. I fail to see which best practices employed by beast could be used to judge async-mqtt5 given those differences.
As an interface for implementing latency sensitive real time distributed systems, probably not.
What part of this library exactly is this referring to? I agree it has room for improving latency, for example, the async_receive signature reads void ( async_mqtt5::error_code, std::string topic, std::string payload, async_mqtt5::publish_props, ) During my review I thought this is a bad choice because the payload will always be copied from the connection internal buffer in which socket.async_read_some reads its data. IMO a better solution would be to allow users direct access to the buffer so he can avoid copies, this is how Boost.Redis and IIRC Boost.Mysql works. Marcelo