Re: [async-mqtt5] Formal review starts today

From: Marcelo Zimbres Silva (mzimbres_at_[hidden])
Date: 2024-10-27 20:37:27

Hi,

On Sun, 27 Oct 2024 at 09:53, Darryl Green via Boost <boost_at_[hidden]>
> 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