example: Remove manual client adding #107

Opening this more as a question, than a change request, because I am not sure what is going on here, since removing it does not seem to change anything in the program's functionality.

This is probably needed, but it is not clear from the example, or the documentation for what. If it is indeed needed, could the documentation be improved a bit, a comment added to the example or could it alternatively be moved into the EventLoop constructor instead? From my understanding addClient is eventually called internally when a new connection or proxy is created.

Good question. I think you are right both that this code can be removed but also that there should be better documentation here.

The event loop is supposed to be an infinite loop that processes events forever as long as an internal reference count m_num_clients is greater than 0. The idea is that you can start the event loop in a thread, and then create new connections, new proxy client objects, and new proxy server objects in other threads that use the event loop. Each of these objects increment m_num_clients when they are created and decrement m_num_clients when they are destroyed so the event loop will automatically exit when the last object is destroyed and let the program shut down cleanly.

I think the unnecessary loop.addClient call got added in this example because it was copied from bitcoin core code. IPC code in bitcoin core was written not to just spawn new processes communicating over file descriptors also but to bind to unix sockets and listen for incoming connections. When it does this, there are interim periods before the first incoming connection, and in gaps between later connections, where no connection objects and no proxy objects exist at all, so the m_num_clients reference count would be 0. To keep it from actually being 0 and prevent the event loop from exiting during these periods, the bitcoin core CapnpProtocol::startLoop function calls loop.addClient before starting the event loop to prevent it from exiting. And the ~CapnpProtocol destructor calls removeClient to exit the event loop and stop listening for new connections.

The code in this example appears to have incorrectly copied the addClient call without copying the corresponding removeClient call, so it looks like the example could not actually shut down correctly because of this imbalance.

I can think of a number of improvments to make here:

• addClient call in example should be dropped
• Some way of exiting the example should be implemented to verify it can actually exit cleanly. You should be able to do something like type "exit" to exit.
• Probably the word "client" should be replaced with "use count" here (in num_clients addClient removeClient). The word "client" is supposed to mean "client of the event loop" but that seems unnecessarily confusing because the word client is overloaded so much.
• Documentation about the reference counts should be improved. It is probably worth mentioning everything I wrote above, and also the fact that the even loop won't just exit immediately when it starts and the reference count is 0. It will intentionally wait for work to be posted to the event loop before checking the reference count, or for the count to transition from 1 to 0.

Thanks for the detailed reply! I'm really enjoying finally getting into the details of this library.

Some way of exiting the example should be implemented to verify it can actually exit cleanly. You should be able to do something like type "exit" to exit.

This seems to be broken, i.e. adding

diff --git a/example/example.cpp b/example/example.cpp
index ecaabaf..dc996a2 100644
--- a/example/example.cpp
+++ b/example/example.cpp
@@ -57,0 +58 @@ int main(int argc, char** argv)
+        if (eqn == "exit") break

Will result in a terminate called without an active exception.

and in gaps between later connections, where no connection objects and no proxy objects exist at all, so the m_num_clients reference count would be 0.

Ah, I was not thinking of these interim periods, that makes sense. How about adding a "listen" boolean to the EventLoop constructor that does this internally then? Making the loop stop by calling removeClient manually feels weird to me, maybe a "StopListening" method would be better?

Probably the word "client" should be replaced with "use count" here (in num_clients addClient removeClient). The word "client" is supposed to mean "client of the event loop" but that seems unnecessarily confusing because the word client is overloaded so much.

I didn't find this that confusing to be honest, and just by looking at the names was assuming "client of the event loop".

Code review ACK 34998101a3cd6f7cb4764625d88437251a9ba723. This code change makes sense, and it definitely doesn't make sense to have an addClient call without a corresponding removeClient call here.

I should follow up with other changes mentioned https://github.com/chaincodelabs/libmultiprocess/pull/107#issuecomment-2299818498, especially getting example to shut down cleanly, and it is good to know "client" name is not too confusing.

The idea of adding listening bool to constructor to avoid need for an addClient call seems good and could be convenient.

I think I'd be reluctant to add a new stopListening method without a corresponding startListening method, but there could be reason to add both of these because right now the existing mp::ListenConnections function only lets you start listening and never stop. It takes ownership of the file descriptor and does not close it until the kj::TaskSet that accepts connections is destroyed, so there is no way to stop listening on a socket without stopping the event loop and destroying the entire event loop object, which is pretty inflexible.