Re: aedis review starts today

From: Marcelo Zimbres Silva (mzimbres_at_[hidden])
Date: 2023-01-17 18:39:27

On Tue, 17 Jan 2023 at 14:38, Vinnie Falco via Boost
<boost_at_[hidden]> wrote:
>
> I have questions. Why is async_run needed?

It was introduced to coordinate reads on the socket

1. When a server-push is received, async_run gives IO control to
connection::async_receive so it can read the push from the socket.

2. When the response to a request arrives it gives IO control to the
next pending connection::async_exec.

3. Notice that in order to support pushes, we need a way to keep
reading from the socket, the question that still must be answered is
why we need both async_run and async_receive (see below).

async_run is also used to coordinate writes socket

4. async_run will also spawn an operation that will write pending
requests to the socket when it becomes possible. For example, you call
async_exec to execute a command but there is an ongoing write from a
previous call or we are waiting for a response to arrive and can't
write yet. The implementation will suspend the async_exec call until
the request can be written and its response has arrived.

> Why doesn't the library just keep an operation running in the
> background automatically for the user?

5. Because there wouldn't be a good way to communicate write errors to
the user, for example, if async_write fails async_run will complete
with the error reported by async_write.

6. Because the connection can be reconnected: async_run can be called
in a loop by the user to proceed with the execution of the pending
async_execs after a disconnection. This is meant to support quick and
efficient failover. Offering this functionality as a built-in feature
might bloat the class.

Some questions that might arise are

* Why do we need a connection that can be reconnected?

Because a connection that is resilient against failovers and
disconnections in general makes user code a lot simpler, you won't
have to implement retries on user side, see [3] for example.

Because it offers better performance: Imagine we have a chat server
with 100k Websocket connections and the admin wants to force a
failover to another Redis instance. Without this feature we would have
100k sessions throwing, catching and retrying, which is concerning
IMO.

* Why don't you merge async_run and async_receive in a single function.

That is possible but would make the code more complex AFAICS. The
separation between async_run and async_receive makes it possible to
have a clean reconnect loop like [1] and a clean loop that receives
pushes like [2].

* Why don't you trigger writes from async_exec instead of spawning a
write operation from async_run?

I found it harder to implement it like this, perhaps I am missing some
async primitives like those from asem library.  At least, I think
there would be a performance hit, reasons we can discuss later.

[1] https://github.com/mzimbres/aedis/blob/c88fcfb9edbaca3cc90e761d8b3087c0eed4a9fb/examples/cpp20_subscriber.cpp#L58
[2] https://github.com/mzimbres/aedis/blob/c88fcfb9edbaca3cc90e761d8b3087c0eed4a9fb/examples/cpp20_subscriber.cpp#L40
[3] https://github.com/mzimbres/aedis/blob/c88fcfb9edbaca3cc90e761d8b3087c0eed4a9fb/examples/cpp20_echo_server.cpp#L21