Re: [async-mqtt5] Formal review starts today

From: Darryl Green (darryl.green_at_[hidden])
Date: 2024-10-28 15:12:42

Thanks for the suggestions and encouraging me to clarify.

On 28/10/2024 6:37 am, Marcelo Zimbres Silva wrote:
> 
> Hi,
> 
>     On Sun, 27 Oct 2024 at 09:53, Darryl Green via Boost
>     <boost_at_[hidden] <mailto:boost_at_[hidden]>> wrote:
 >
>     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.

I simply meant that its somewhat like logging. Im not sure if 
on_<everything> is appropriate but being able to react to connectivity 
changes seems pretty fundamental.

> 
>  > > - 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.

Sometimes you just want to do a "it works or it doesn't" operation in, 
of all things, an MQTT client.
That said, since posting my review I saw in other discussion that there 
is the option to use total cancellation at the async publish level. I'd 
only seen examples and docs for some destructive cancellation 
approaches. This alone might be enough to address some of the problems 
while using only a high level interface.

> 
>  > 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
> ...
> 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.

I wasn't referring to the performance of the library code, I was as in 
all my comments referring to the performance / service offered by a 
system that includes an application using the library code and that is 
trying to deliver a particular type of service.

Consider a client that has a large, active and complex enough collection 
of topics to publish that the performance features we are talking about 
here are of benefit.

In some cases not only is low latency in the sense of ability to utilise 
the large capacity channel important, but so is constraining latency 
when the rate of publication attempts is actually higher than the 
capacity of the channel. When and what to "leak" becomes important.

Consider an intermittently or "weakly" connected but very active client 
publishing thousands of topics, that normally get queued and delivered 
to some remote server (it may or may not be a "broker" in the 
conventional sense).

If even with those features achieving high efficiency / throughput the 
rate of publication is such that the latency from the publisher to the 
the server grows due to channel capacity issues (slow channels and in 
the extreme case, channels that are intermittently unavailable) then for 
some groups of topics it is likely appropriate and even necessary to 
drop old/stale "queued" publications in favour of making sure the server 
remains up to date (at the loss of some visibility / temporal resolution 
regarding what are now "historical" values/changes). Note that it is 
likely there are other topic groups for which delivering every 
publication is important. MQTT and the library claim to support 
multiplexing topics and messages with different QoS. One common 
usage/expectation is that QoS 0 messages will be delivered "fast" but 
that the eventual receiver of them will not see every message (as one 
could expect from calling this at_most_once). Meanwhile a message that 
must be delivered exactly once.. is slow(er).. but not expected (or is 
it) to get "stuck" indefinitely behind a (growing?) set of QoS 0 messages.

Id like to see how one can use the library in a system with this type of 
performance requirement. Id also like to se how it can be used with QoS 
1 and a constrained window used specifically to stop the "channel" (TCP 
stack and general buffer bloat) blowing out latency - its not in 
practice safe to assume latency sensitive but relatively message loss 
insensitive traffic should be sent with "QoS 0" in MQTT terms.

It is important that if one does intend to drop messages (which with a 
TCP transport needs to happen prior to the network stack) that this can 
be done "fairly".

A high level library that can do all this needs a tunable "QoS" in the 
general sense (not just the selection of one of 3 values for an MQTT 
parameter). A low level library can legitimately  claim its out of 
scope. This library has high level features but appears to lack some 
tunability (but that could be user ignorance on my part) and also access 
to low level features to allow the user to take responsibility for these 
issues. That needs addressing.

Ive also seen some informed discussion around whether a library should 
be accepted exactly so that the author is encouraged to modify and 
improve it. I am all for encouraging authors to produce high quality, 
useful libraries and my "reject at this point" is using the "reject" 
word because that is the only "non accept" word on offer unless one can 
formulate a detailed list of conditions for "conditional acceptance" - 
which are as Vinnie already mentioned is hard for a library like this.

Regards
Darryl.