ref: #183
This PR adds test coverage for the client method ConnectStream, as suggested by @ryanofsky.
The following cases are tested:
- Connecting to a socket serving a valid init interface
- Passing a disconnected socket.
- Passing a disconnected socket with an Init interface with no
construct()method. - Passing a disconnected socket with an Init interface with no
construct()method and making no calls. - Passing a live socket that disconnects after some data is received.
- Passing a socket from a listening socket (
accept()) that disconnects after some data arrives
Additionally, a new FooInit interface is added, and the UnixListener class introduced in #269 to set up listening tests is extracted to a shared file so the new connect_tests.cpp file can consume it.
Bugs found
The added tests exposed a couple of bugs, whose fixes are included in this PR as the first 2 commits. Details about them are below:
Fix 1: Use-after-free race found in the NetBSD 9.4 job
The NetBSD 9.4 job was crashingin the new test connect_tests.cpp:94 with:
assertion "m_loop" failed: file ".../include/mp/proxy.h", line 59,
function "mp::EventLoop& mp::EventLoopRef::operator*() const"
*** Received signal [#6](/bitcoin-core-multiprocess/6/): Abort trap
Asked Claude to try to reproduce it locally and came up with placing (in ConnectStream)
std::this_thread::sleep_for(std::chrono::milliseconds(100));
between the loop.sync(...) block and the client construction to widen the race window so that the event loop thread wins the race (as it consistently did in the failing job), thereby exposing the issue.
The sequence of events was as follows: passing a disconnected socket fd causes the RPC system to hit EOF on the first read. The event loop resumes right after the loop.sync(...) block, resolves m_network.onDisconnect(), whose handler (registered inside the same sync block) runs delete connection_ptr. Connection::~Connection resets its EventLoopRef m_loop member, and the memory is freed.
The caller thread then wakes up and constructs the ProxyClient<InitInterface>, whose ProxyContext constructor reads the freed Connection: https://github.com/bitcoin-core/libmultiprocess/blob/8412fcdc659e1379f9b4dea896c26bc1c5f3afa8/src/mp/proxy.cpp#L82
The fix is included in this PR as the first commit. It moves the onDisconnect handler registration to a second loop.sync(...) call just after the proxy client has been constructed.
Fix 2: Connection leak on failed construct() prevents EventLoop::loop() from exiting
When a capnp init interface declares a construct() method (as in example/init.capnp and Bitcoin Core's Init interface), the generated ProxyClient constructor calls it automatically, making a blocking IPC call while the client is being constructed inside ConnectStream.
If that call fails, such as when connecting to a disconnected socket, the exception escapes ProxyClientBase's constructor. Normally, cleanup occurs in the destructor but if the constructor throws no object is created, and the destructor doesn't run. As a result, cleanup functions are not called, the Connection object is leaked, and its EventLoopRef prevents EventLoop::loop() from exiting.
The second commit addresses this by running cleanup functions before rethrowing, which releases the client capability, unregisters the disconnect callback, and deletes the connection when it is owned by the client.
Alternatively, the Sub::construct() call could be removed from the ProxyClientBase constructor and called by ConnectStream after client creation. I am open to switching to this approach if preferred.
