Merge #273: proxy-client: tolerate exceptions from remote destroy during cleanup

6de92e1 proxy-client: tolerate exceptions from remote destroy during cleanup (xyzconstant)
90be835 test: regression for ~ProxyClient destroy after peer disconnect (xyzconstant)

Pull request description:

  Fixes #219.

  Catches exceptions from `Sub::destroy(*this)` in `~ProxyClientBase`'s cleanup lambda so a failed remote destroy call doesn't crash the process. Regression test included.

ACKs for top commit:
  ryanofsky:
    Code review ACK 6de92e1. Nice fix and test.
  enirox001:
    Code review ACK 6de92e1.

Tree-SHA512: 1b588670503d339b528d16fd14345243d7c4da0d9714360b43f5c8d92da89387b90f9fab817317603f2ae9c8b1c6eadfb9112fb3cc8f55bf240be13539b41bcf

Author: ryanofsky

include/mp/proxy-io.h

@@ -539,7 +539,12 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
         // the remote object, waiting for it to be deleted server side. If the
         // capnp interface does not define a destroy method, this will just call
         // an empty stub defined in the ProxyClientBase class and do nothing.
-        Sub::destroy(*this);
+        // Exceptions are caught and logged rather than propagated because
+        // ~ProxyClientBase is noexcept and the peer may be gone by the time
+        // this runs.
+        if (kj::runCatchingExceptions([&]{ Sub::destroy(*this); }) != nullptr) {
+            MP_LOG(*m_context.loop, Log::Warning) << "Remote destroy call failed during cleanup. Continuing.";
+        }
 
         // FIXME: Could just invoke removed addCleanup fn here instead of duplicating code
         m_context.loop->sync([&]() {

test/mp/test/test.cpp

@@ -434,6 +434,32 @@ KJ_TEST("Calling async IPC method, with server disconnect after cleanup")
     }
 }
 
+KJ_TEST("Destroying ProxyClient<> with destroy method after peer disconnect")
+{
+    // Regression test for bitcoin-core/libmultiprocess#219 where
+    // ~ProxyClientBase would call std::terminate if the remote destroy RPC
+    // failed during teardown.
+    //
+    // Save a callback on the server so it holds a ProxyClient<FooCallback>
+    // pointing back to this side, then disconnect. When the server is torn
+    // down, the ProxyClient<FooCallback> destructor issues a destroy RPC over
+    // the now dead connection; without the bugfix the exception escapes the
+    // noexcept destructor and aborts the process.
+
+    TestSetup setup{/*client_owns_connection=*/false};
+    ProxyClient<messages::FooInterface>* foo = setup.client.get();
+    foo->initThreadMap();
+
+    class Callback : public FooCallback
+    {
+    public:
+        int call(int arg) override { return arg; }
+    };
+
+    foo->saveCallback(std::make_shared<Callback>());
+    setup.client_disconnect();
+}
+
 KJ_TEST("Make simultaneous IPC calls on single remote thread")
 {
     TestSetup setup;