Fix startup race in example

[xyzconstant]

Since #274, connection setup has been split into two calls: MakeStream followed by ConnectStream. Between these calls, the event loop's reference count can temporarily drop to zero, allowing EventLoop::loop() to exit (and the EventLoop to be destroyed) before ConnectStream posts its work. This crashes mpexample on startup.

Fix this by keeping an EventLoopRef alive in main() so the loop stays running while it is in use. This same pattern is already used in Bitcoin Core (m_loop_ref member of CapnpProtocol).

A second commit documents the reference-counted EventLoop lifetime in the EventLoopRef class comment and in doc/usage.md.

NOTE: On macOS, mpexample was crashing intermittently on startup (sometimes failing the m_post_fn == nullptr assert in the EventLoop destructor, sometimes with a mutex lock failure), depending on timing. With this change, the crashes are gone.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	ryanofsky
Stale ACK	ViniciusCestarii

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

[xyzconstant]

failed CI job seems unrelated

[ViniciusCestarii]

tACK f2fabf6 nice catch! I was able to reproduce on linux and it also intermittently fails without this fix. I liked the doc explanation too.

nit: a more surgical fix for against against master could look like:

+#include "mp/proxy.h"
 #include <init.capnp.h>
 #include <init.capnp.proxy.h>
 
int main(int argc, char** argv)
         loop.loop();
     });
     mp::EventLoop* loop = promise.get_future().get();
+    mp::EventLoopRef loop_ref{*loop};
 
     auto [printer_init, printer_pid] = Spawn(*loop, argv[0], "mpprinter");
     auto [calc_init, calc_pid] = Spawn(*loop, argv[0], "mpcalculator");
int main(int argc, char** argv)
     mp::WaitProcess(calc_pid);
     printer_init.reset();
     mp::WaitProcess(printer_pid);
+    loop_ref.reset();
     loop_thread.join();
     std::cout << "Bye!" << std::endl;
     return 0;

[ryanofsky]

Code review ACK 265302b. Nice catch and this fix looks correct, but it seems a little more complicated than it needs to be and I suggested a simpler version below.

I was initially confused how #274 causes this bug, but it happens specifically because of the loop.sync call in MakeStream. Each time loop.sync is called it causes the event loop to spin and check the reference count and potentially exit. The sync call in MakeStream is actually not necessary in practice, but was added as precaution in case the the wrapFd call was changed to update shared state. But because creating the stream object and connecting used to happen in one sync call and now happens in two calls, adding an extra reference to the event loop is necessary.

[ryanofsky]

In commit "Fix startup race in mpexample" (f433a3f)

I don't think it actually makes sense to change this function and pass in a loop ref object when this isn't going to actually change the reference count. Would be better to revert changes to this function.

Similarly I think most of the changes below are not needed. Would suggest a simpler fix:

--- a/example/example.cpp
+++ b/example/example.cpp
@@ -55,6 +55,7 @@ int main(int argc, char** argv)
         loop.loop();
     });
     mp::EventLoop* loop = promise.get_future().get();
+    mp::EventLoopRef loop_ref{*loop};
 
     auto [printer_init, printer_pid] = Spawn(*loop, argv[0], "mpprinter");
     auto [calc_init, calc_pid] = Spawn(*loop, argv[0], "mpcalculator");
@@ -71,6 +72,7 @@ int main(int argc, char** argv)
     mp::WaitProcess(calc_pid);
     printer_init.reset();
     mp::WaitProcess(printer_pid);
+    loop_ref.reset();
     loop_thread.join();
     std::cout << "Bye!" << std::endl;
     return 0;

[xyzconstant]

Done at 3a997e1

Thanks!

[ryanofsky]

In commit "Fix startup race in mpexample" (f433a3f)

Suggested an alternative in diff above, and I think creating an EventLoopRef here and then moving it into another EventLoopRef variable is too complicated and not necessary. It should only be necessary to increment the event loop usage count before using the event loop, and doesn't have to be done when creating it.

[xyzconstant]

Thanks for the reviews!

The same diff has been suggested twice, so I just applied it in the latest push (36c6c63).

With #323 now merged, CI jobs should be green.

[ryanofsky]

Code review ACK 36c6c63. Thanks for the fix, documentation, and simplification!