From 73fe7d723084653671f2178ea1177a8627edfafa Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 17 Jul 2024 10:37:52 -0400 Subject: [PATCH] multiprocess: Add unit tests for connect, serve, and listen functions --- src/ipc/capnp/protocol.cpp | 3 +- src/ipc/protocol.h | 8 ++- src/test/CMakeLists.txt | 2 +- src/test/ipc_test.cpp | 99 +++++++++++++++++++++++++++++++++++++- src/test/ipc_test.h | 5 +- src/test/ipc_tests.cpp | 33 ++++++++++++- 6 files changed, 142 insertions(+), 8 deletions(-) diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index 9d18d62102b..4b67a5bd1e3 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -61,11 +61,12 @@ public: } mp::ListenConnections(*m_loop, listen_fd, init); } - void serve(int fd, const char* exe_name, interfaces::Init& init) override + void serve(int fd, const char* exe_name, interfaces::Init& init, const std::function& ready_fn = {}) override { assert(!m_loop); mp::g_thread_context.thread_name = mp::ThreadName(exe_name); m_loop.emplace(exe_name, &IpcLogFn, &m_context); + if (ready_fn) ready_fn(); mp::ServeStream(*m_loop, fd, init); m_loop->loop(); m_loop.reset(); diff --git a/src/ipc/protocol.h b/src/ipc/protocol.h index 1e355784ade..b2ebf99e8c8 100644 --- a/src/ipc/protocol.h +++ b/src/ipc/protocol.h @@ -50,7 +50,13 @@ public: //! created by them. This isn't really a problem because serve() is only //! called by spawned child processes that call it immediately to //! communicate back with parent processes. - virtual void serve(int fd, const char* exe_name, interfaces::Init& init) = 0; + // + //! The optional `ready_fn` callback will be called after the event loop is + //! created but before it is started. This can be useful in tests to trigger + //! client connections from another thread as soon as the event loop is + //! available, but should not be neccessary 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; //! Add cleanup callback to interface that will run when the interface is //! deleted. diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index 4e24b87e9b7..92b39f04976 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -177,7 +177,7 @@ if(WITH_MULTIPROCESS) PRIVATE ipc_tests.cpp ) - target_link_libraries(test_bitcoin bitcoin_ipc_test) + target_link_libraries(test_bitcoin bitcoin_ipc_test bitcoin_ipc) endif() function(add_boost_test source_file) diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp index ce4edaceb08..e6de6e3e477 100644 --- a/src/test/ipc_test.cpp +++ b/src/test/ipc_test.cpp @@ -2,19 +2,46 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include +#include +#include +#include #include #include #include #include #include +#include #include +#include #include #include #include +#include #include +//! Remote init class. +class TestInit : public interfaces::Init +{ +public: + std::unique_ptr makeEcho() override { return interfaces::MakeEcho(); } +}; + +//! Generate a temporary path with temp_directory_path and mkstemp +static std::string TempPath(std::string_view pattern) +{ + std::string temp{fs::PathToString(fs::path{fs::temp_directory_path()} / fs::PathFromString(std::string{pattern}))}; + temp.push_back('\0'); + int fd{mkstemp(temp.data())}; + BOOST_CHECK_GE(fd, 0); + BOOST_CHECK_EQUAL(close(fd), 0); + temp.resize(temp.size() - 1); + fs::remove(fs::PathFromString(temp)); + return temp; +} + //! Unit test that tests execution of IPC calls without actually creating a //! separate process. This test is primarily intended to verify behavior of type //! conversion code that converts C++ objects to Cap'n Proto messages and vice @@ -23,13 +50,13 @@ //! The test creates a thread which creates a FooImplementation object (defined //! in ipc_test.h) and a two-way pipe accepting IPC requests which call methods //! on the object through FooInterface (defined in ipc_test.capnp). -void IpcTest() +void IpcPipeTest() { // Setup: create FooImplemention object and listen for FooInterface requests std::promise>> foo_promise; std::function disconnect_client; std::thread thread([&]() { - mp::EventLoop loop("IpcTest", [](bool raise, const std::string& log) { LogPrintf("LOG%i: %s\n", raise, log); }); + mp::EventLoop loop("IpcPipeTest", [](bool raise, const std::string& log) { LogPrintf("LOG%i: %s\n", raise, log); }); auto pipe = loop.m_io_context.provider->newTwoWayPipe(); auto connection_client = std::make_unique(loop, kj::mv(pipe.ends[0])); @@ -65,3 +92,71 @@ void IpcTest() disconnect_client(); thread.join(); } + +//! Test ipc::Protocol connect() and serve() methods connecting over a socketpair. +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::promise promise; + std::thread thread([&]() { + protocol->serve(fds[0], "test-serve", *init, [&] { promise.set_value(); }); + }); + promise.get_future().wait(); + std::unique_ptr remote_init{protocol->connect(fds[1], "test-connect")}; + std::unique_ptr remote_echo{remote_init->makeEcho()}; + BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); + remote_echo.reset(); + remote_init.reset(); + thread.join(); +} + +//! Test ipc::Process bind() and connect() methods connecting over a unix socket. +void IpcSocketTest(const fs::path& datadir) +{ + std::unique_ptr init{std::make_unique()}; + std::unique_ptr protocol{ipc::capnp::MakeCapnpProtocol()}; + std::unique_ptr process{ipc::MakeProcess()}; + + std::string invalid_bind{"invalid:"}; + BOOST_CHECK_THROW(process->bind(datadir, "test_bitcoin", invalid_bind), std::invalid_argument); + BOOST_CHECK_THROW(process->connect(datadir, "test_bitcoin", invalid_bind), std::invalid_argument); + + auto bind_and_listen{[&](const std::string& bind_address) { + std::string address{bind_address}; + 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); + }}; + + 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_echo{remote_init->makeEcho()}; + BOOST_CHECK_EQUAL(remote_echo->echo("echo test"), "echo test"); + }}; + + // Need to specify explicit socket addresses outside the data directory, because the data + // directory path is so long that the default socket address and any other + // addresses in the data directory would fail with errors like: + // Address 'unix' path '"/tmp/test_common_Bitcoin Core/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/test_bitcoin.sock"' exceeded maximum socket path length + std::vector addresses{ + strprintf("unix:%s", TempPath("bitcoin_sock0_XXXXXX")), + strprintf("unix:%s", TempPath("bitcoin_sock1_XXXXXX")), + }; + + // Bind and listen on multiple addresses + for (const auto& address : addresses) { + bind_and_listen(address); + } + + // Connect and test each address multiple times. + for (int i : {0, 1, 0, 0, 1}) { + connect_and_test(addresses[i]); + } +} diff --git a/src/test/ipc_test.h b/src/test/ipc_test.h index bcfcc2125c6..2453bfa23c7 100644 --- a/src/test/ipc_test.h +++ b/src/test/ipc_test.h @@ -7,6 +7,7 @@ #include #include +#include class FooImplementation { @@ -16,6 +17,8 @@ public: UniValue passUniValue(UniValue v) { return v; } }; -void IpcTest(); +void IpcPipeTest(); +void IpcSocketPairTest(); +void IpcSocketTest(const fs::path& datadir); #endif // BITCOIN_TEST_IPC_TEST_H diff --git a/src/test/ipc_tests.cpp b/src/test/ipc_tests.cpp index 6e144b0f418..35a4f611177 100644 --- a/src/test/ipc_tests.cpp +++ b/src/test/ipc_tests.cpp @@ -2,12 +2,41 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include + +#include #include -BOOST_AUTO_TEST_SUITE(ipc_tests) +BOOST_FIXTURE_TEST_SUITE(ipc_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(ipc_tests) { - IpcTest(); + IpcPipeTest(); + IpcSocketPairTest(); + IpcSocketTest(m_args.GetDataDirNet()); } + +// Test address parsing. +BOOST_AUTO_TEST_CASE(parse_address_test) +{ + std::unique_ptr process{ipc::MakeProcess()}; + fs::path datadir{"/var/empty/notexist"}; + auto check_notexist{[](const std::system_error& e) { return e.code() == std::errc::no_such_file_or_directory; }}; + auto check_address{[&](std::string address, std::string expect_address, std::string expect_error) { + if (expect_error.empty()) { + BOOST_CHECK_EXCEPTION(process->connect(datadir, "test_bitcoin", address), std::system_error, check_notexist); + } else { + BOOST_CHECK_EXCEPTION(process->connect(datadir, "test_bitcoin", address), std::invalid_argument, HasReason(expect_error)); + } + BOOST_CHECK_EQUAL(address, expect_address); + }}; + check_address("unix", "unix:/var/empty/notexist/test_bitcoin.sock", ""); + check_address("unix:", "unix:/var/empty/notexist/test_bitcoin.sock", ""); + check_address("unix:path.sock", "unix:/var/empty/notexist/path.sock", ""); + check_address("unix:0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.sock", + "unix:/var/empty/notexist/0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.sock", + "Unix address path \"/var/empty/notexist/0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.sock\" exceeded maximum socket path length"); + check_address("invalid", "invalid", "Unrecognized address 'invalid'"); +} + BOOST_AUTO_TEST_SUITE_END()