diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h index 29cd3a45..37656021 100644 --- a/include/mp/proxy-io.h +++ b/include/mp/proxy-io.h @@ -63,16 +63,16 @@ struct ProxyClient : public ProxyClientBase ProxyClient(const ProxyClient&) = delete; ~ProxyClient(); - void setCleanup(std::function cleanup); + void setCleanup(std::function fn); //! Cleanup function to run when the connection is closed. If the Connection //! gets destroyed before this ProxyClient object, this cleanup //! callback lets it destroy this object and remove its entry in the //! thread's request_threads or callback_threads map (after resetting - //! m_cleanup so the destructor does not try to access it). But if this + //! m_cleanup_it so the destructor does not try to access it). But if this //! object gets destroyed before the Connection, there's no need to run the //! cleanup function and the destructor will unregister it. - std::optional m_cleanup; + std::optional m_cleanup_it; }; template <> @@ -379,7 +379,7 @@ ProxyClientBase::ProxyClientBase(typename Interface::Client cli } // Handler for the connection getting destroyed before this client object. - auto cleanup = m_context.connection->addSyncCleanup([this]() { + auto cleanup_it = m_context.connection->addSyncCleanup([this]() { // Release client capability by move-assigning to temporary. { typename Interface::Client(std::move(self().m_client)); @@ -402,11 +402,11 @@ ProxyClientBase::ProxyClientBase(typename Interface::Client cli // The first case is handled here when m_context.connection is not null. The // second case is handled by the cleanup function, which sets m_context.connection to // null so nothing happens here. - m_context.cleanup.emplace_front([this, destroy_connection, cleanup]{ + m_context.cleanup_fns.emplace_front([this, destroy_connection, cleanup_it]{ if (m_context.connection) { // Remove cleanup callback so it doesn't run and try to access // this object after it's already destroyed. - m_context.connection->removeSyncCleanup(cleanup); + m_context.connection->removeSyncCleanup(cleanup_it); // If the capnp interface defines a destroy method, call it to destroy // the remote object, waiting for it to be deleted server side. If the @@ -434,14 +434,6 @@ ProxyClientBase::ProxyClientBase(typename Interface::Client cli }); } -template -ProxyClientBase::~ProxyClientBase() noexcept -{ - for (auto& cleanup : m_context.cleanup) { - cleanup(); - } -} - template ProxyServerBase::ProxyServerBase(std::shared_ptr impl, Connection& connection) : m_impl(std::move(impl)), m_context(&connection) @@ -476,14 +468,12 @@ ProxyServerBase::~ProxyServerBase() // connection is broken). Probably some refactoring of the destructor // and invokeDestroy function is possible to make this cleaner and more // consistent. - m_context.connection->addAsyncCleanup([impl=std::move(m_impl), c=std::move(m_context.cleanup)]() mutable { + m_context.connection->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable { impl.reset(); - for (auto& cleanup : c) { - cleanup(); - } + CleanupRun(fns); }); } - assert(m_context.cleanup.size() == 0); + assert(m_context.cleanup_fns.size() == 0); std::unique_lock lock(m_context.connection->m_loop.m_mutex); m_context.connection->m_loop.removeClient(lock); } @@ -509,10 +499,7 @@ template void ProxyServerBase::invokeDestroy() { m_impl.reset(); - for (auto& cleanup : m_context.cleanup) { - cleanup(); - } - m_context.cleanup.clear(); + CleanupRun(m_context.cleanup_fns); } using ConnThreads = std::map>; diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 6e6d419a..9a121bb0 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -1532,17 +1532,18 @@ struct CapRequestTraits<::capnp::Request<_Params, _Results>> using Results = _Results; }; -//! Entry point called by all generated ProxyClient destructors. This only logs -//! the object destruction. The actual cleanup happens in the ProxyClient base -//! destructor. +//! Entry point called by all generated ProxyClient destructors. template void clientDestroy(Client& client) { + // Log that ProxyClient object is being destroyed. if (client.m_context.connection) { client.m_context.connection->m_loop.log() << "IPC client destroy " << typeid(client).name(); } else { KJ_LOG(INFO, "IPC interrupted client destroy", typeid(client).name()); } + // Call ProxyClientBase cleanup. + client.cleanup(); } template diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 1ee1c753..7855a024 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -39,17 +39,30 @@ struct ProxyType; using CleanupList = std::list>; using CleanupIt = typename CleanupList::iterator; +inline void CleanupRun(CleanupList& fns) { + for (auto& fn : fns) { + fn(); + } +} + //! Context data associated with proxy client and server classes. struct ProxyContext { Connection* connection; - std::list> cleanup; + CleanupList cleanup_fns; ProxyContext(Connection* connection) : connection(connection) {} }; //! Base class for generated ProxyClient classes that implement a C++ interface //! and forward calls to a capnp interface. +// +//! @note: All ProxyClient subclasses that inherit from this are responsible for +//! calling cleanup() in their destructors. The ProxyClientBase destructor +//! cannot call cleanup() itself, because it runs too late after the ProxyClient +//! subclass is destroyed, when the cleanup callback that is run would trigger +//! C++ undefined behavior by calling self() and using a pointer to a partially +//! destroyed object. template class ProxyClientBase : public Impl_ { @@ -58,7 +71,7 @@ class ProxyClientBase : public Impl_ using Impl = Impl_; ProxyClientBase(typename Interface::Client client, Connection* connection, bool destroy_connection); - ~ProxyClientBase() noexcept; + ~ProxyClientBase() noexcept = default; // construct/destroy methods called during client construction/destruction // that can optionally be defined in capnp interfaces to invoke code on the @@ -93,6 +106,7 @@ class ProxyClientBase : public Impl_ void destroy() {} ProxyClient& self() { return static_cast&>(*this); } + void cleanup() { CleanupRun(m_context.cleanup_fns); } typename Interface::Client m_client; ProxyContext m_context; @@ -147,7 +161,7 @@ struct ProxyServerBase : public virtual Interface_::Server //! state can be destroyed without blocking, because ProxyServer destructors are //! called from the EventLoop thread, and if they block, it could deadlock the //! program. One way to do avoid blocking is to clean up the state by pushing -//! cleanup callbacks to the m_context.cleanup list, which run after the server +//! cleanup callbacks to the m_context.cleanup_fns list, which run after the server //! m_impl object is destroyed on the same thread destroying it (which will //! either be an IPC worker thread if the ProxyServer is being explicitly //! destroyed by a client calling a destroy() method with a Context argument and diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp index bcfbde8b..4b323c0f 100644 --- a/src/mp/proxy.cpp +++ b/src/mp/proxy.cpp @@ -283,9 +283,9 @@ std::tuple SetThread(ConnThreads& threads, std::mutex& mutex, // destructor unregisters the cleanup. // Connection is being destroyed before thread client is, so reset - // thread client m_cleanup member so thread client destructor does not + // thread client m_cleanup_it member so thread client destructor does not // try unregister this callback after connection is destroyed. - thread->second.m_cleanup.reset(); + thread->second.m_cleanup_it.reset(); // Remove connection pointer about to be destroyed from the map std::unique_lock lock(mutex); threads.erase(thread); @@ -295,16 +295,21 @@ std::tuple SetThread(ConnThreads& threads, std::mutex& mutex, ProxyClient::~ProxyClient() { - if (m_cleanup) { - m_context.connection->removeSyncCleanup(*m_cleanup); + // If thread is being destroyed before connection is destroyed, remove the + // cleanup callback that was registered to handle the connection being + // destroyed before the thread being destroyed. + if (m_cleanup_it) { + m_context.connection->removeSyncCleanup(*m_cleanup_it); } + // Call ProxyClientBase cleanup. + cleanup(); } -void ProxyClient::setCleanup(std::function cleanup) +void ProxyClient::setCleanup(std::function fn) { - assert(cleanup); - assert(!m_cleanup); - m_cleanup = m_context.connection->addSyncCleanup(cleanup); + assert(fn); + assert(!m_cleanup_it); + m_cleanup_it = m_context.connection->addSyncCleanup(fn); } ProxyServer::ProxyServer(ThreadContext& thread_context, std::thread&& thread) diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp index fc74fc98..661ae693 100644 --- a/test/mp/test/test.cpp +++ b/test/mp/test/test.cpp @@ -122,7 +122,7 @@ KJ_TEST("Call FooInterface methods") thread.join(); bool destroyed = false; - foo->m_context.cleanup.emplace_front([&destroyed]{ destroyed = true; }); + foo->m_context.cleanup_fns.emplace_front([&destroyed]{ destroyed = true; }); foo.reset(); KJ_EXPECT(destroyed); }