Fix startup race in example #315

xyzconstant commented at 2:53 AM on July 22, 2026: contributor

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 commented at 2:53 AM on July 22, 2026: none

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 <code></code> into the comment that the bot should ignore.

xyzconstant force-pushed on Jul 22, 2026

xyzconstant commented at 4:10 AM on July 22, 2026: contributor

failed CI job seems unrelated

xyzconstant force-pushed on Jul 22, 2026

allibmb1997-collab approved

ViniciusCestarii commented at 2:46 PM on July 22, 2026: contributor

tACK f2fabf668c4bfb1c55fd41f993b7a4bd754a8653 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;

xyzconstant force-pushed on Jul 23, 2026

in example/example.cpp:28 in 265302bc41

  24 | @@ -24,14 +25,15 @@
  25 |  
  26 |  namespace fs = std::filesystem;
  27 |  
  28 | -static auto Spawn(mp::EventLoop& loop, const std::string& process_argv0, const std::string& new_exe_name)
  29 | +static auto Spawn(const mp::EventLoopRef& loop_ref, const std::string& process_argv0, const std::string& new_exe_name)

ryanofsky commented at 4:42 AM on July 30, 2026:

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

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 commented at 2:24 PM on July 30, 2026:

Done at 3a997e113cf781222022d2a61ce15e05efd43cdd

Thanks!

in example/example.cpp:56 in 265302bc41

  49 | @@ -48,16 +50,16 @@ int main(int argc, char** argv)
  50 |          return 1;
  51 |      }
  52 |  
  53 | -    std::promise<mp::EventLoop*> promise;
  54 | +    std::promise<mp::EventLoopRef> promise;
  55 |      std::thread loop_thread([&] {
  56 |          mp::EventLoop loop("mpexample", LogPrint);
  57 | -        promise.set_value(&loop);
  58 | +        promise.set_value(mp::EventLoopRef(loop));

ryanofsky commented at 4:50 AM on July 30, 2026:

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

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.

ryanofsky approved

ryanofsky commented at 5:01 AM on July 30, 2026: collaborator

Code review ACK 265302bc418136e072e37aa5de70ec7791679076. 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.

DrahtBot requested review from ViniciusCestarii on Jul 30, 2026

Fix startup race in mpexample

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
before `ConnectStream` posts its work, crashing the example on startup.

Keep an `EventLoopRef` alive in main() so the loop stays running while it
is in use.

3a997e113c

doc: Document reference-counted EventLoop lifetime

`EventLoop::loop()` exits when the last `EventLoopRef` is released, so
code making multiple calls against the loop needs to hold its own
reference. State this in the `EventLoopRef` comment and usage.md.

36c6c63520

xyzconstant force-pushed on Jul 30, 2026

xyzconstant commented at 2:23 PM on July 30, 2026: contributor

Thanks for the reviews!

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

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

ryanofsky approved

ryanofsky commented at 2:49 PM on July 30, 2026: collaborator

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

ryanofsky merged this on Jul 30, 2026

ryanofsky closed this on Jul 30, 2026