From 33d37f3c35efaac136863253b91799bf2711fd46 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 30 Apr 2025 08:39:29 -0400 Subject: [PATCH] ipc, refactor: Drop connect/listen/serve exe_name parameters Pass exe_name parameter to ipc::Protocol class constructor instead. It never really made sense to have exe parameters as part of the protocol interface and removing them makes adding new features like windows support easier. The exe name values are only used for logging and debuggging purposes to distinguish log messages from different processes. --- src/ipc/capnp/protocol.cpp | 22 ++++++++++++---------- src/ipc/capnp/protocol.h | 2 +- src/ipc/interfaces.cpp | 10 +++++----- src/ipc/protocol.h | 6 +++--- src/ipc/test/ipc_tests.cpp | 12 ++++++------ 5 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index e39c5039da9..7fc893fc385 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -71,34 +71,35 @@ void IpcLogFn(mp::LogMessage message) class CapnpProtocol : public Protocol { public: + CapnpProtocol(const char* exe_name) : m_exe_name{exe_name} {} ~CapnpProtocol() noexcept(true) { m_loop_ref.reset(); if (m_loop_thread.joinable()) m_loop_thread.join(); assert(!m_loop); }; - std::unique_ptr connect(int fd, const char* exe_name) override + std::unique_ptr connect(int fd) override { - startLoop(exe_name); + startLoop(); return mp::ConnectStream(*m_loop, fd); } - void listen(int listen_fd, const char* exe_name, interfaces::Init& init) override + void listen(int listen_fd, interfaces::Init& init) override { - startLoop(exe_name); + startLoop(); if (::listen(listen_fd, /*backlog=*/5) != 0) { throw std::system_error(errno, std::system_category()); } mp::ListenConnections(*m_loop, listen_fd, init); } - void serve(int fd, const char* exe_name, interfaces::Init& init, const std::function& ready_fn = {}) override + void serve(int fd, interfaces::Init& init, const std::function& ready_fn = {}) override { assert(!m_loop); - mp::g_thread_context.thread_name = mp::ThreadName(exe_name); + mp::g_thread_context.thread_name = mp::ThreadName(m_exe_name); mp::LogOptions opts = { .log_fn = IpcLogFn, .log_level = GetRequestedIPCLogLevel() }; - m_loop.emplace(exe_name, std::move(opts), &m_context); + m_loop.emplace(m_exe_name, std::move(opts), &m_context); if (ready_fn) ready_fn(); mp::ServeStream(*m_loop, fd, init); m_parent_connection = &m_loop->m_incoming_connections.back(); @@ -120,7 +121,7 @@ public: mp::ProxyTypeRegister::types().at(type)(iface).cleanup_fns.emplace_back(std::move(cleanup)); } Context& context() override { return m_context; } - void startLoop(const char* exe_name) + void startLoop() { if (m_loop) return; std::promise promise; @@ -130,7 +131,7 @@ public: .log_fn = IpcLogFn, .log_level = GetRequestedIPCLogLevel() }; - m_loop.emplace(exe_name, std::move(opts), &m_context); + m_loop.emplace(m_exe_name, std::move(opts), &m_context); m_loop_ref.emplace(*m_loop); promise.set_value(); m_loop->loop(); @@ -138,6 +139,7 @@ public: }); promise.get_future().wait(); } + const char* m_exe_name; Context m_context; std::thread m_loop_thread; //! EventLoop object which manages I/O events for all connections. @@ -151,6 +153,6 @@ public: }; } // namespace -std::unique_ptr MakeCapnpProtocol() { return std::make_unique(); } +std::unique_ptr MakeCapnpProtocol(const char* exe_name) { return std::make_unique(exe_name); } } // namespace capnp } // namespace ipc diff --git a/src/ipc/capnp/protocol.h b/src/ipc/capnp/protocol.h index 54a8ae6efc2..ef828cd4ef5 100644 --- a/src/ipc/capnp/protocol.h +++ b/src/ipc/capnp/protocol.h @@ -10,7 +10,7 @@ namespace ipc { class Protocol; namespace capnp { -std::unique_ptr MakeCapnpProtocol(); +std::unique_ptr MakeCapnpProtocol(const char* exe_name); } // namespace capnp } // namespace ipc diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 32febd35526..3345a92172e 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -54,7 +54,7 @@ class IpcImpl : public interfaces::Ipc public: IpcImpl(const char* exe_name, const char* process_argv0, interfaces::Init& init) : m_exe_name(exe_name), m_process_argv0(process_argv0), m_init(init), - m_protocol(ipc::capnp::MakeCapnpProtocol()), m_process(ipc::MakeProcess()) + m_protocol(ipc::capnp::MakeCapnpProtocol(exe_name)), m_process(ipc::MakeProcess()) { } std::unique_ptr spawnProcess(const char* new_exe_name) override @@ -62,7 +62,7 @@ public: int pid; int fd = m_process->spawn(new_exe_name, m_process_argv0, pid); LogDebug(::BCLog::IPC, "Process %s pid %i launched\n", new_exe_name, pid); - auto init = m_protocol->connect(fd, m_exe_name); + auto init = m_protocol->connect(fd); Ipc::addCleanup(*init, [this, new_exe_name, pid] { int status = m_process->waitSpawned(pid); LogDebug(::BCLog::IPC, "Process %s pid %i exited with status %i\n", new_exe_name, pid, status); @@ -77,7 +77,7 @@ public: return false; } IgnoreCtrlC(strprintf("[%s] SIGINT received — waiting for parent to shut down.\n", m_exe_name)); - m_protocol->serve(fd, m_exe_name, m_init); + m_protocol->serve(fd, m_init); exit_status = EXIT_SUCCESS; return true; } @@ -106,12 +106,12 @@ public: } else { fd = m_process->connect(gArgs.GetDataDirNet(), "bitcoin-node", address); } - return m_protocol->connect(fd, m_exe_name); + return m_protocol->connect(fd); } void listenAddress(std::string& address) override { int fd = m_process->bind(gArgs.GetDataDirNet(), m_exe_name, address); - m_protocol->listen(fd, m_exe_name, m_init); + m_protocol->listen(fd, m_init); } void disconnectIncoming() override { diff --git a/src/ipc/protocol.h b/src/ipc/protocol.h index e7d66887a1a..d14c7f20013 100644 --- a/src/ipc/protocol.h +++ b/src/ipc/protocol.h @@ -32,12 +32,12 @@ public: //! up its own state (calling ProxyServer destructors, etc) on disconnect, //! and any client calls will just throw ipc::Exception errors after a //! disconnect. - virtual std::unique_ptr connect(int fd, const char* exe_name) = 0; + virtual std::unique_ptr connect(int fd) = 0; //! Listen for connections on provided socket descriptor, accept them, and //! handle requests on accepted connections. This method doesn't block, and //! performs I/O on a background thread. - virtual void listen(int listen_fd, const char* exe_name, interfaces::Init& init) = 0; + virtual void listen(int listen_fd, interfaces::Init& init) = 0; //! Handle requests on provided socket descriptor, forwarding them to the //! provided Init interface. Socket communication is handled on the @@ -56,7 +56,7 @@ public: //! client connections from another thread as soon as the event loop is //! available, but should not be necessary in normal code which starts //! clients and servers independently. - virtual void serve(int fd, const char* exe_name, interfaces::Init& init, const std::function& ready_fn = {}) = 0; + virtual void serve(int fd, interfaces::Init& init, const std::function& ready_fn = {}) = 0; //! Disconnect any incoming connections that are still connected. virtual void disconnectIncoming() = 0; diff --git a/src/ipc/test/ipc_tests.cpp b/src/ipc/test/ipc_tests.cpp index b261792058e..75b9544a147 100644 --- a/src/ipc/test/ipc_tests.cpp +++ b/src/ipc/test/ipc_tests.cpp @@ -132,13 +132,13 @@ void IpcSocketPairTest() int fds[2]; BOOST_CHECK_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); std::unique_ptr init{std::make_unique()}; - std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; + std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol("IpcSocketPairTest")}; std::promise promise; std::thread thread([&]() { - protocol->serve(fds[0], "test-serve", *init, [&] { promise.set_value(); }); + protocol->serve(fds[0], *init, [&] { promise.set_value(); }); }); promise.get_future().wait(); - std::unique_ptr remote_init{protocol->connect(fds[1], "test-connect")}; + std::unique_ptr remote_init{protocol->connect(fds[1])}; std::unique_ptr remote_echo{remote_init->makeEcho()}; BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); remote_echo.reset(); @@ -150,7 +150,7 @@ void IpcSocketPairTest() void IpcSocketTest(const fs::path& datadir) { std::unique_ptr init{std::make_unique()}; - std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; + std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol("IpcSocketTest")}; std::unique_ptr process{ipc::MakeProcess()}; std::string invalid_bind{"invalid:"}; @@ -162,14 +162,14 @@ void IpcSocketTest(const fs::path& datadir) int serve_fd = process->bind(datadir, "test_bitcoin", address); BOOST_CHECK_GE(serve_fd, 0); BOOST_CHECK_EQUAL(address, bind_address); - protocol->listen(serve_fd, "test-serve", *init); + protocol->listen(serve_fd, *init); }}; auto connect_and_test{[&](const std::string& connect_address) { std::string address{connect_address}; int connect_fd{process->connect(datadir, "test_bitcoin", address)}; BOOST_CHECK_EQUAL(address, connect_address); - std::unique_ptr remote_init{protocol->connect(connect_fd, "test-connect")}; + std::unique_ptr remote_init{protocol->connect(connect_fd)}; std::unique_ptr remote_echo{remote_init->makeEcho()}; BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); }};