Re: aedis review starts today

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2023-01-21 19:00:37

On Sun, 15 Jan 2023 at 16:17, Klemens Morgenstern via Boost
<boost_at_[hidden]> wrote:
>
> The formal review of the aedis starts today (15th) and ends next week on
> Tuesday (24th).

Hi all,

This is my review of the proposed Boost.Aedis.

First of all, thank you very much Marcelo for submitting the library, and for
your quick responses to my (not few) questions. Thanks also Klemens for acting
as review manager.

> Does this library bring real benefit to C++ developers for real world
> use-case?

I strongly believe so. It integrates in the new Asio-based ecosystem we are
creating, together with Beast and MySQL. Redis is popular. I buy the vision.

> Do you have an application for this library?

I don't. I'm currently just maintaining Boost.MySQL, and there's little room
for it in the task. But I've had it in the past - a company in the IoT world
that was using Redis as a message-passing system. I would have liked having
Aedis back then.

> Does the API match with current best practices?

In terms of Asio primitives, yes, it does. It complies to the universal async
model, which is what I'd expect. I've also liked how you implemented
rebind_executor for SSL streams to make default completion tokens easier -
which is something I've recently done in MySQL myself, too.

This library introduces something unusual to Asio-based components, which is
the command queue. I see advantages on having it (as it provides out-of-the-box
efficiency), but also some downsides (it is opinionated, not allowing for
strategies like connection pooling). I don't think this is necessarily a bad
thing - field experience will give us insights. I do think this makes good
docs even more essential.

I like how adapt() works. Parsing into compile-time structures is always good.
However, as a Redis novice I am, I've had a hard time debugging things when I
got a mismatch between the type passed to adapt() and what the command expects.
The error happens at parse time, which I think is pretty dangerous, as it's very
easy to get things overlooked. As an example of this, the library's own example
cpp20_containers, in the hgetall function, uses the type
std::map<std::string, std::string> for the hgetall response, which will be
okay unless the key is not set, in which case you will get an error. I think
this has an enormous potential for causing hidden bugs (and even
vulnerabilities). I've also noticed that if you mismatch the sizes of the
pipeline request and response, you get an assertion (so UB).

I initially went with a lot of (false) confidence, as adapt() changed my mind
to "this is type safe, you're not writing C, the compiler will know if you
mess it". It's true that if you think it twice, the compiler can't know,
but this false confidence is something that may happen to more people.
So I think this should somehow be addressed.

Exposing just request::push() seems terse. I think some methods for the most
common commands would help a lot. Something like
request::push_set(string_view key, string_view value). Or maybe
request::push(set_cmd(string_view key, string_view value)).
Maybe doing that could help address the problem above. I still think a general
request::push() should be exposed for the less common commands.

The way of handling network errors is very elegant, and it shows the author's
effort on it. The reconnect loop is clean. Congratulations.

I think error handling at the command level is flawed, though. If a command
fails, the entire pipeline operation fails, and no easy access to the
diagnostics is provided. I've also seen that Redis will execute all commands
in a pipeline even if an error
is encountered in one of them. So, in pipeline [C1, C2, C3], if C2 errors,
C3 is still executed. The API should provide a way to access C2's error and
C3's result. The proposed solution in
https://github.com/mzimbres/aedis/issues/40 seems insufficient to me. I'd
give Boost.Leaf (or a similar solution) a thought.

I don't know if making the connection hang when a push is received and there is
no async_receive active is safe. I don't know enough of edge cases to know,
but it causes me to raise an eyebrow as user, at least.

> Is the documentation helpful and clear?

It is far away from the level I'd ask for a Boost library. Please remember that
this is one of the criticisms Boost gets at all times, so we should pay special
attention to it. What I think it can be improved:

* It needs a proper discussion section, as other libraries do. Explaining goals,
design decissions and usage patterns. I'd structure this as:
* A proper tutorial. You may assume familiarity with Redis to the point
"I know what HGETALL does" (although I would start with a SET/GET). But
HELLO and QUIT are not obvious. I'd explain that this 3 is the
protocol version, what happens if you don't send a HELLO, whether QUIT
is handled specially or just like any other command, etc.
Do not place an opaque function "connect" that
is not part of the public API in a tutorial. The code should run "as-is".
Provide also error handling code.
* A design discussion, where you present your design choices with examples
and snippets. I like how Boost.URL does it. You've implemented a pretty
opinionated queueing system, so I'd draw a diagram of how that works, with
the different tasks and how they relate to each other. Also state
what benefits does it bring. It would be great if a user could figure out
most of the questions I've asked you throughout the review by reading that
document.
I would include some of the code you currently have in examples as snippets,
like the ones in containers, transactions, subscriptions, and so on.
A section on error handling would be nice, too. Some docs on request::config
flags and when to use each would be great, too.
* An example section, where you provide the full listing of all examples.
I'm currently using quickbook and its import facility to make this happen,
and I'm pretty happy.
* A proper reference.

> Did you try to use it? What problems or surprises did you encounter?

* Doesn't build with clang15 and libc++ under Ubuntu 22.04,
as <memory_resource> appears to not be stable yet (as of libc++ 15).
* Builds and runs fine with gcc11 and stdlibc++ under Ubuntu 22.04.

I was a little bit let down by finding what seems to be a bug in the queueing
system by just playing with the subscription example. I see that all tests
covering the connection class (and its underlying queue system) are integrated.
This leads to the impression that edge and error conditions haven't been
tested as I'd have expected.

> What is your evaluation of the implementation?

I think it is good. As others have already looked into it, I just read enough
to figure out pieces I was missing to understand the system under review,
and won't comment it here.

> Additionally, stating your knowledge of redis and asio would be helpful for
> me.

I have written a similar client library using Asio (Boost.MySQL). I have a
shallow knowledge of Redis. I've read the documentation and played with it
these days to get a refresher, but I'm far from an expert.

I have invested around 2 days in playing with the library and writing this
review.

> Please explicitly state that you either *accept* or *reject* the inclusion
> of this library into boost.
> Also please indicate the time & effort spent on the evaluation and give the
> reasons for your decision.

I vote to REJECT the library in its current state. I think we need to make sure
proper tests, documentation and error handling strategies are in place
before proceeding.

I think the library has gone in the right direction overall since I last looked
at it. I would encourage Marcelo to keep working on it and resubmit it soon.
I don't think a full review will be required then, but a mini-review should
be enough.

Some minor things:
* resp3::node< String >: "String: A std::string-like type" - type requirements
like this should be properly documented - what does that mean? Concept
checking here would be great.
* Would be great to provide concept checking for adapt()
"The types T1, T2, etc can be any STL container, any integer type and
std::string." can it not be aedis::ignore, too?
* request::push and request::push_range: the reference contains no info on
type requirements.
* I would consider renaming the library to Boost.Redis, as it would make purpose
clearer.
* async_exec reference: "Multiple concurrent calls to this function
will be automatically queued by the implementation." this can lead the user
think the connection class is thread-safe, which is not.
* As a user I wouldn't know if I can really use the low-level API or not.
* #include <boost/asio.hpp> in examples makes the compiler cry ;)
* The CMakeLists aren't compatible with the Boost superproject.
* B2 support should be added at some point.
* I think this library shows that Klemen's Asem should be part of Boost.

Thanks again Marcelo for taking your time and effort.

Regards,
Ruben.

On Sat, 21 Jan 2023 at 19:40, Marcelo Zimbres Silva <mzimbres_at_[hidden]> wrote:
>
> On Sat, 21 Jan 2023 at 19:04, Jakob Lövhall <lovhall_at_[hidden]> wrote:
> >
> > making the error text available is not only logging. it can be
> > passed on to a user in interactive sessions giving the user (human)
> > ability to act on it right away.  I wouldn't call that "only useful
> > for logging" even though I guess it is logging in some sense.
>
> Good point. I hadn't thought about interactive sessions.
>
> Regards,
> Marcelo