From e886c65b6b37aaaf5d22ca68bc14e55d8ec78212 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 4 Aug 2025 13:38:26 -0400 Subject: [PATCH 1/7] Squashed 'src/ipc/libmultiprocess/' changes from 27c7e8e5a581..b4120d34bad2 b4120d34bad2 Merge bitcoin-core/libmultiprocess#192: doc: fix typos 6ecbdcd35a93 doc: fix typos a11e6905c238 Merge bitcoin-core/libmultiprocess#186: Fix mptest failures in bitcoin CI 6f340a583f2b doc: fix DrahtBot LLM Linter error c6f7fdf17350 type-context: revert client disconnect workaround e09143d2ea2f proxy-types: fix UndefinedBehaviorSanitizer: null-pointer-use 84b292fcc4db mptest: fix MemorySanitizer: use-of-uninitialized-value fe4a188803c6 proxy-io: fix race conditions in disconnect callback code d8011c83608e proxy-io: fix race conditions in ProxyClientBase cleanup handler 97e82ce19c47 doc: Add note about Waiter::m_mutex and interaction with the EventLoop::m_mutex 81d58f5580e8 refactor: Rename ProxyClient cleanup_it variable 07230f259f55 refactor: rename ProxyClient::m_cleanup_it c0efaa5e8cb1 Merge chaincodelabs/libmultiprocess#187: ci: have bash scripts explicitly opt out of locale dependence. 0d986ff144cd mptest: fix race condition in TestSetup constructor d2f6aa2e84ef ci: add thread sanitizer job 3a6db38e561f ci: rename configs to .bash 401e0ce1d9c3 ci: add copyright to bash scripts e956467ae464 ci: export LC_ALL 8954cc0377d8 Merge chaincodelabs/libmultiprocess#184: Add CI jobs and fix clang-tidy and iwyu errors 757e13a75546 ci: add gnu32 cross-compiled 32-bit build 15bf349000eb doc: fix typo found by DrahtBot 1a598d5905f7 clang-tidy: drop 'bitcoin-*' check cbb1e43fdc6e ci: test libc++ instead of libstdc++ in one job 76313450c2c4 type-context: disable clang-tidy UndefinedBinaryOperatorResult error 4896e7fe51ba proxy-types: fix clang-tidy EnumCastOutOfRange error 060a73926956 proxy-types: fix clang-tidy StackAddressEscape error 977d721020f6 ci: add github actions jobs testing gcc, clang-20, clang-tidy, and iwyu 0d5f1faae5da iwyu: fix add/remove include errors 753d2b10cc27 util: fix clang-tidy modernize-use-equals-default error ae4f1dc2bb1a type-number: fix clang-tidy modernize-use-nullptr error 07a741bf6946 proxy-types: fix clang-tidy bugprone-use-after-move error 3673114bc9d9 proxy-types: fix clang-tidy bugprone-use-after-move error 422923f38485 proxy-types: fix clang-tidy bugprone-use-after-move error c6784c6adefa mpgen: disable clang-tidy misc-no-recursion error c5498aa11ba6 tidy: copy clang-tidy file from bitcoin core 258a617c1eec Merge chaincodelabs/libmultiprocess#160: refactor: EventLoop locking cleanups + client disconnect exception 84cf56a0b5f4 test: Test disconnects during IPC calls 949573da8411 Prevent IPC server crash if disconnected during IPC call 019839758085 Merge chaincodelabs/libmultiprocess#179: scripted-diff: Remove copyright year (ranges) ea38392960e1 Prevent EventLoop async cleanup thread early exit during shutdown 616d9a75d20a doc: Document ProxyClientBase destroy_connection option 56fff76f940b Improve IPC client disconnected exceptions 9b8ed3dc5f87 refactor: Add clang thread safety annotations to EventLoop 52256e730f51 refactor: Remove DestructorCatcher and AsyncCallable f24894794adf refactor: Drop addClient/removeClient methods 2b830e558e61 refactor: Use EventLoopRef instead of addClient/removeClient 315ff537fb65 refactor: Add ProxyContext EventLoop* member 9aaeec3678d3 proxy-io.h: Add EventLoopRef RAII class handle addClient/removeClient refcounting f58c8d8ba2f0 proxy-io.h: Add more detailed EventLoop comment 5108445e5d16 test: Add test coverage for client & server disconnections 59030c68cb5f Merge chaincodelabs/libmultiprocess#181: type-function.h: Fix CustomBuildField overload 688140b1dffc test: Add coverage for type-function.h 8b96229da58e type-function.h: Fix CustomBuildField overload fa2ff9a66842 scripted-diff: Remove copyright year (ranges) git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: b4120d34bad2de28141c5770f6e8df8e54898987 --- .clang-tidy | 67 +++++----- .github/workflows/ci.yml | 29 +++++ CMakeLists.txt | 30 ++++- ci/README.md | 25 ++++ ci/configs/default.bash | 5 + ci/configs/gnu32.bash | 9 ++ ci/configs/llvm.bash | 11 ++ ci/configs/sanitize.bash | 7 + ci/scripts/ci.sh | 22 ++++ ci/scripts/run.sh | 13 ++ cmake/Config.cmake.in | 2 +- cmake/TargetCapnpSources.cmake | 2 + cmake/compat_config.cmake | 2 +- cmake/compat_find.cmake | 10 +- cmake/pthread_checks.cmake | 2 +- example/CMakeLists.txt | 2 +- example/calculator.capnp | 2 +- example/calculator.cpp | 16 ++- example/calculator.h | 2 +- example/example.cpp | 11 +- example/init.capnp | 5 +- example/init.h | 2 +- example/printer.capnp | 2 +- example/printer.cpp | 18 ++- example/printer.h | 2 +- example/types.h | 11 +- include/mp/config.h.in | 2 +- include/mp/proxy-io.h | 173 ++++++++++++++----------- include/mp/proxy-types.h | 105 ++++++++------- include/mp/proxy.capnp | 2 +- include/mp/proxy.h | 53 ++++++-- include/mp/type-char.h | 2 +- include/mp/type-chrono.h | 2 +- include/mp/type-context.h | 36 ++---- include/mp/type-data.h | 2 +- include/mp/type-decay.h | 2 +- include/mp/type-exception.h | 2 +- include/mp/type-function.h | 4 +- include/mp/type-interface.h | 2 +- include/mp/type-map.h | 2 +- include/mp/type-message.h | 2 +- include/mp/type-number.h | 10 +- include/mp/type-optional.h | 2 +- include/mp/type-pair.h | 2 +- include/mp/type-pointer.h | 2 +- include/mp/type-set.h | 2 +- include/mp/type-string.h | 2 +- include/mp/type-struct.h | 2 +- include/mp/type-threadmap.h | 2 +- include/mp/type-tuple.h | 2 +- include/mp/type-vector.h | 2 +- include/mp/type-void.h | 2 +- include/mp/util.h | 104 ++++++++------- shell.nix | 28 ++++ src/mp/gen.cpp | 51 ++++++-- src/mp/proxy.cpp | 184 ++++++++++++++------------ src/mp/util.cpp | 6 +- test/CMakeLists.txt | 4 +- test/mp/test/foo-types.h | 18 ++- test/mp/test/foo.capnp | 10 +- test/mp/test/foo.h | 8 +- test/mp/test/test.cpp | 230 ++++++++++++++++++++++++++++----- 62 files changed, 933 insertions(+), 440 deletions(-) create mode 100644 .github/workflows/ci.yml create mode 100644 ci/README.md create mode 100644 ci/configs/default.bash create mode 100644 ci/configs/gnu32.bash create mode 100644 ci/configs/llvm.bash create mode 100644 ci/configs/sanitize.bash create mode 100755 ci/scripts/ci.sh create mode 100755 ci/scripts/run.sh create mode 100644 shell.nix diff --git a/.clang-tidy b/.clang-tidy index 2d29f120ae4..9a2afcc5248 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,41 +1,40 @@ Checks: ' -*, -bugprone-*, --bugprone-easily-swappable-parameters, --bugprone-exception-escape, --bugprone-move-forwarding-reference, --bugprone-narrowing-conversions, --bugprone-reserved-identifier, -misc-*, --misc-non-private-member-variables-in-classes, --misc-no-recursion, --misc-unconventional-assign-operator, --misc-unused-parameters, --misc-use-anonymous-namespace, -modernize-*, --modernize-avoid-c-arrays, --modernize-concat-nested-namespaces, --modernize-deprecated-headers, --modernize-use-nodiscard, --modernize-use-trailing-return-type, --modernize-use-using, +bugprone-argument-comment, +bugprone-move-forwarding-reference, +bugprone-string-constructor, +bugprone-use-after-move, +bugprone-lambda-function-name, +bugprone-unhandled-self-assignment, +misc-unused-using-decls, +misc-no-recursion, +modernize-deprecated-headers, +modernize-use-default-member-init, +modernize-use-emplace, +modernize-use-equals-default, +modernize-use-noexcept, +modernize-use-nullptr, +modernize-use-starts-ends-with, performance-*, -performance-avoid-endl, +-performance-enum-size, +-performance-inefficient-string-concatenation, +-performance-no-int-to-ptr, -performance-noexcept-move-constructor, -readability-*, --readability-braces-around-statements, --readability-convert-member-functions-to-static, --readability-else-after-return, --readability-function-cognitive-complexity, --readability-identifier-length, --readability-implicit-bool-conversion, --readability-inconsistent-declaration-parameter-name, --readability-magic-numbers, --readability-named-parameter, --readability-uppercase-literal-suffix, --readability-use-anyofallof, +-performance-unnecessary-value-param, +readability-const-return-type, +readability-redundant-declaration, +readability-redundant-string-init, +clang-analyzer-core.*, +-clang-analyzer-core.UndefinedBinaryOperatorResult, +clang-analyzer-optin.core.*, ' +HeaderFilterRegex: '.' +WarningsAsErrors: '*' CheckOptions: - - key: modernize-use-override.IgnoreDestructors - value: true -HeaderFilterRegex: 'example/calculator.h|example/init.h|example/printer.h|include/mp/proxy-io.h|include/mp/proxy-types.h|include/mp/proxy.h|include/mp/util.h|test/mp/test/foo-types.h|test/mp/test/foo.h' + - key: modernize-deprecated-headers.CheckHeaderFile + value: false + - key: performance-move-const-arg.CheckTriviallyCopyableMove + value: false + - key: bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField + value: false diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000000..2e751c5fd18 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,29 @@ +name: CI + +on: + push: + pull_request: + +jobs: + build: + runs-on: ubuntu-latest + + strategy: + fail-fast: false + matrix: + config: [default, llvm, gnu32, sanitize] + + name: build • ${{ matrix.config }} + + steps: + - uses: actions/checkout@v4 + + - name: Install Nix + uses: cachix/install-nix-action@v31 # 2025-05-27, from https://github.com/cachix/install-nix-action/tags + with: + nix_path: nixpkgs=channel:nixos-25.05 # latest release + + - name: Run CI script + env: + CI_CONFIG: ci/configs/${{ matrix.config }}.bash + run: ci/scripts/run.sh diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ade99338ce..d29eb490d96 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,4 +1,4 @@ -# Copyright (c) 2019 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -15,16 +15,35 @@ include("cmake/compat_find.cmake") find_package(CapnProto REQUIRED) find_package(Threads REQUIRED) -option(Libmultiprocess_ENABLE_CLANG_TIDY "Run clang-tidy with the compiler." OFF) -if(Libmultiprocess_ENABLE_CLANG_TIDY) +set(MPGEN_EXECUTABLE "" CACHE FILEPATH "If specified, should be full path to an external mpgen binary to use rather than the one built internally.") + +option(MP_ENABLE_CLANG_TIDY "Run clang-tidy with the compiler." OFF) +if(MP_ENABLE_CLANG_TIDY) find_program(CLANG_TIDY_EXECUTABLE NAMES clang-tidy) if(NOT CLANG_TIDY_EXECUTABLE) - message(FATAL_ERROR "Libmultiprocess_ENABLE_CLANG_TIDY is ON but clang-tidy is not found.") + message(FATAL_ERROR "MP_ENABLE_CLANG_TIDY is ON but clang-tidy is not found.") endif() set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}") + + # Workaround for nix from https://gitlab.kitware.com/cmake/cmake/-/issues/20912#note_793338 + # Nix injects header paths via $NIX_CFLAGS_COMPILE; CMake tags these as + # CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES and omits them from the compile + # database, so clang-tidy, which ignores $NIX_CFLAGS_COMPILE, can't find capnp + # headers. Setting them as standard passes them to clang-tidy. + set(CMAKE_CXX_STANDARD_INCLUDE_DIRECTORIES ${CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES}) endif() -set(MPGEN_EXECUTABLE "" CACHE FILEPATH "If specified, should be full path to an external mpgen binary to use rather than the one built internally.") +option(MP_ENABLE_IWYU "Run include-what-you-use with the compiler." OFF) +if(MP_ENABLE_IWYU) + find_program(IWYU_EXECUTABLE NAMES include-what-you-use iwyu) + if(NOT IWYU_EXECUTABLE) + message(FATAL_ERROR "MP_ENABLE_IWYU is ON but include-what-you-use was not found.") + endif() + set(CMAKE_CXX_INCLUDE_WHAT_YOU_USE "${IWYU_EXECUTABLE};-Xiwyu;--error") + if(DEFINED ENV{IWYU_MAPPING_FILE}) + list(APPEND CMAKE_CXX_INCLUDE_WHAT_YOU_USE "-Xiwyu" "--mapping_file=$ENV{IWYU_MAPPING_FILE}") + endif() +endif() include("cmake/compat_config.cmake") include("cmake/pthread_checks.cmake") @@ -51,6 +70,7 @@ configure_file(include/mp/config.h.in "${CMAKE_CURRENT_BINARY_DIR}/include/mp/co # Generated C++ Capn'Proto schema files capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp) +set_source_files_properties("${MP_PROXY_SRCS}" PROPERTIES SKIP_LINTING TRUE) # Ignored before cmake 3.27 # util library add_library(mputil OBJECT src/mp/util.cpp) diff --git a/ci/README.md b/ci/README.md new file mode 100644 index 00000000000..85eb467c1fe --- /dev/null +++ b/ci/README.md @@ -0,0 +1,25 @@ +### CI quick-reference + +All CI is just bash and nix. + +* **Workflow**: + - `.github/workflows/ci.yml` – lists the jobs (`default`, `llvm`, …). +* **Scripts**: + - `ci/scripts/run.sh` – spins up the Nix shell then calls… + - `ci/scripts/ci.sh` – …to configure, build, and test. +* **Configuration**: + - `ci/configs/*.sh` – defines flags for each job. + - `shell.nix` – defines build environment (compilers, tools, libraries). +* **Build directories**: + - `build-*/` – separate build directories (like `build-default`, `build-llvm`) will be created for each job. + +To run jobs locally: + +```bash +CI_CONFIG=ci/configs/default.bash ci/scripts/run.sh +CI_CONFIG=ci/configs/llvm.bash ci/scripts/run.sh +CI_CONFIG=ci/configs/gnu32.bash ci/scripts/run.sh +CI_CONFIG=ci/configs/sanitize.bash ci/scripts/run.sh +``` + +By default CI jobs will reuse their build directories. `CI_CLEAN=1` can be specified to delete them before running instead. diff --git a/ci/configs/default.bash b/ci/configs/default.bash new file mode 100644 index 00000000000..56231228d4f --- /dev/null +++ b/ci/configs/default.bash @@ -0,0 +1,5 @@ +CI_DESC="CI job using default libraries and tools, and running IWYU" +CI_DIR=build-default +export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter" +CMAKE_ARGS=(-DMP_ENABLE_IWYU=ON) +BUILD_ARGS=(-k) diff --git a/ci/configs/gnu32.bash b/ci/configs/gnu32.bash new file mode 100644 index 00000000000..961821ce8ce --- /dev/null +++ b/ci/configs/gnu32.bash @@ -0,0 +1,9 @@ +CI_DESC="CI job cross-compiling to 32-bit" +CI_DIR=build-gnu32 +NIX_ARGS=( + --arg minimal true + --arg crossPkgs 'import { crossSystem = { config = "i686-unknown-linux-gnu"; }; }' +) +export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter" +CMAKE_ARGS=(-G Ninja) +BUILD_ARGS=(-k 0) diff --git a/ci/configs/llvm.bash b/ci/configs/llvm.bash new file mode 100644 index 00000000000..afa957ed86e --- /dev/null +++ b/ci/configs/llvm.bash @@ -0,0 +1,11 @@ +CI_DESC="CI job using LLVM-based libraries and tools (clang, libc++, clang-tidy, iwyu) and testing Ninja" +CI_DIR=build-llvm +NIX_ARGS=(--arg enableLibcxx true) +export CXX=clang++ +export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wthread-safety-analysis -Wno-unused-parameter" +CMAKE_ARGS=( + -G Ninja + -DMP_ENABLE_CLANG_TIDY=ON + -DMP_ENABLE_IWYU=ON +) +BUILD_ARGS=(-k 0) diff --git a/ci/configs/sanitize.bash b/ci/configs/sanitize.bash new file mode 100644 index 00000000000..ce920f44279 --- /dev/null +++ b/ci/configs/sanitize.bash @@ -0,0 +1,7 @@ +CI_DESC="CI job running ThreadSanitizer" +CI_DIR=build-sanitize +export CXX=clang++ +export CXXFLAGS="-ggdb -Werror -Wall -Wextra -Wpedantic -Wthread-safety-analysis -Wno-unused-parameter -fsanitize=thread" +CMAKE_ARGS=() +BUILD_ARGS=(-k -j4) +BUILD_TARGETS=(mptest) diff --git a/ci/scripts/ci.sh b/ci/scripts/ci.sh new file mode 100755 index 00000000000..baf21700f6a --- /dev/null +++ b/ci/scripts/ci.sh @@ -0,0 +1,22 @@ +#!/usr/bin/env bash +# +# Copyright (c) The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +export LC_ALL=C.UTF-8 + +set -o errexit -o nounset -o pipefail -o xtrace + +[ "${CI_CONFIG+x}" ] && source "$CI_CONFIG" + +: "${CI_DIR:=build}" +if ! [ -v BUILD_TARGETS ]; then + BUILD_TARGETS=(all tests mpexamples) +fi + +[ -n "${CI_CLEAN-}" ] && rm -rf "${CI_DIR}" + +cmake -B "$CI_DIR" "${CMAKE_ARGS[@]+"${CMAKE_ARGS[@]}"}" +cmake --build "$CI_DIR" -t "${BUILD_TARGETS[@]}" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" +ctest --test-dir "$CI_DIR" --output-on-failure diff --git a/ci/scripts/run.sh b/ci/scripts/run.sh new file mode 100755 index 00000000000..11b91845e12 --- /dev/null +++ b/ci/scripts/run.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash +# +# Copyright (c) The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +export LC_ALL=C.UTF-8 + +set -o errexit -o nounset -o pipefail -o xtrace + +[ "${CI_CONFIG+x}" ] && source "$CI_CONFIG" + +nix-shell --pure --keep CI_CONFIG --keep CI_CLEAN "${NIX_ARGS[@]+"${NIX_ARGS[@]}"}" --run ci/scripts/ci.sh shell.nix diff --git a/cmake/Config.cmake.in b/cmake/Config.cmake.in index edff7d143be..60187f08f4d 100644 --- a/cmake/Config.cmake.in +++ b/cmake/Config.cmake.in @@ -17,7 +17,7 @@ if ("Bin" IN_LIST ${CMAKE_FIND_PACKAGE_NAME}_FIND_COMPONENTS) endif() if ("Lib" IN_LIST ${CMAKE_FIND_PACKAGE_NAME}_FIND_COMPONENTS) - # Setting FOUND_LIBATOMIC is needed on debian & ubuntu systems to work around bug in + # Setting FOUND_LIBATOMIC is needed on Debian & Ubuntu systems to work around bug in # their capnproto packages. See compat_find.cmake for a more complete explanation. set(FOUND_LIBATOMIC TRUE) include(CMakeFindDependencyMacro) diff --git a/cmake/TargetCapnpSources.cmake b/cmake/TargetCapnpSources.cmake index cf7d20feb96..347ef4a010a 100644 --- a/cmake/TargetCapnpSources.cmake +++ b/cmake/TargetCapnpSources.cmake @@ -81,6 +81,8 @@ function(target_capnp_sources target include_prefix) DEPENDS ${capnp_file} VERBATIM ) + # Skip linting for capnp-generated files but keep it for mpgen-generated ones + set_source_files_properties(${capnp_file}.c++ PROPERTIES SKIP_LINTING TRUE) # Ignored before cmake 3.27 target_sources(${target} PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/${capnp_file}.c++ ${CMAKE_CURRENT_BINARY_DIR}/${capnp_file}.proxy-client.c++ diff --git a/cmake/compat_config.cmake b/cmake/compat_config.cmake index 283cd38c49e..f9d3004f05c 100644 --- a/cmake/compat_config.cmake +++ b/cmake/compat_config.cmake @@ -1,4 +1,4 @@ -# Copyright (c) 2019 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/cmake/compat_find.cmake b/cmake/compat_find.cmake index e1d4f7d427a..d3d7bc6d3f8 100644 --- a/cmake/compat_find.cmake +++ b/cmake/compat_find.cmake @@ -1,18 +1,18 @@ -# Copyright (c) 2024 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. # compat_find.cmake -- compatibility workarounds meant to be included before # cmake find_package() calls are made -# Set FOUND_LIBATOMIC to work around bug in debian capnproto package that is -# debian-specific and does not happpen upstream. Debian includes a patch +# Set FOUND_LIBATOMIC to work around bug in Debian capnproto package that is +# Debian-specific and does not happen upstream. Debian includes a patch # https://sources.debian.org/patches/capnproto/1.0.1-4/07_libatomic.patch/ which # uses check_library_exists(atomic __atomic_load_8 ...) and it fails because the -# symbol name conflicts with a compiler instrinsic as described +# symbol name conflicts with a compiler intrinsic as described # https://github.com/bitcoin-core/libmultiprocess/issues/68#issuecomment-1135150171. # This could be fixed by improving the check_library_exists function as -# described in the github comment, or by changing the debian patch to check for +# described in the github comment, or by changing the Debian patch to check for # the symbol a different way, but simplest thing to do is work around the # problem by setting FOUND_LIBATOMIC. This problem has probably not # been noticed upstream because it only affects CMake packages depending on diff --git a/cmake/pthread_checks.cmake b/cmake/pthread_checks.cmake index b54c0b45b8d..241978d5677 100644 --- a/cmake/pthread_checks.cmake +++ b/cmake/pthread_checks.cmake @@ -1,4 +1,4 @@ -# Copyright (c) 2024 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/example/CMakeLists.txt b/example/CMakeLists.txt index 333462b8249..0e758d57764 100644 --- a/example/CMakeLists.txt +++ b/example/CMakeLists.txt @@ -1,4 +1,4 @@ -# Copyright (c) 2021 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/example/calculator.capnp b/example/calculator.capnp index 8f546552f70..94551883601 100644 --- a/example/calculator.capnp +++ b/example/calculator.capnp @@ -1,4 +1,4 @@ -# Copyright (c) 2021 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/example/calculator.cpp b/example/calculator.cpp index ae69ce8a626..016a04863f8 100644 --- a/example/calculator.cpp +++ b/example/calculator.cpp @@ -1,19 +1,23 @@ -// Copyright (c) 2021 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // 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 // NOLINT(misc-include-cleaner) -#include +#include // NOLINT(misc-include-cleaner) // IWYU pragma: keep + +#include +#include +#include #include +#include +#include +#include #include #include -#include #include #include +#include #include class CalculatorImpl : public Calculator diff --git a/example/calculator.h b/example/calculator.h index 749e435547d..342a3c1d154 100644 --- a/example/calculator.h +++ b/example/calculator.h @@ -1,4 +1,4 @@ -// Copyright (c) 2021 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/example/example.cpp b/example/example.cpp index a4f84c55a75..5088f79649f 100644 --- a/example/example.cpp +++ b/example/example.cpp @@ -1,13 +1,18 @@ -// Copyright (c) 2021 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include +#include + +#include // IWYU pragma: keep #include #include #include -#include -#include #include +#include +#include +#include #include #include #include diff --git a/example/init.capnp b/example/init.capnp index 2b0b5113972..01897f13dca 100644 --- a/example/init.capnp +++ b/example/init.capnp @@ -1,4 +1,4 @@ -# Copyright (c) 2021 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -12,8 +12,7 @@ using Printer = import "printer.capnp"; $Proxy.include("calculator.h"); $Proxy.include("init.h"); $Proxy.include("printer.h"); -$Proxy.includeTypes("calculator.capnp.proxy-types.h"); -$Proxy.includeTypes("printer.capnp.proxy-types.h"); +$Proxy.includeTypes("types.h"); interface InitInterface $Proxy.wrap("Init") { construct @0 (threadMap: Proxy.ThreadMap) -> (threadMap :Proxy.ThreadMap); diff --git a/example/init.h b/example/init.h index 314d5d7f238..54e36da8db1 100644 --- a/example/init.h +++ b/example/init.h @@ -1,4 +1,4 @@ -// Copyright (c) 2021 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/example/printer.capnp b/example/printer.capnp index e27ce412048..0f407b77141 100644 --- a/example/printer.capnp +++ b/example/printer.capnp @@ -1,4 +1,4 @@ -# Copyright (c) 2021 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/example/printer.cpp b/example/printer.cpp index 9f85d450b66..eb384018baa 100644 --- a/example/printer.cpp +++ b/example/printer.cpp @@ -1,18 +1,24 @@ -// Copyright (c) 2021 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // 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 // NOLINT(misc-include-cleaner) -#include +#include // NOLINT(misc-include-cleaner) // IWYU pragma: keep + +#include +#include +#include #include +#include +#include +#include #include #include -#include #include #include +#include class PrinterImpl : public Printer { diff --git a/example/printer.h b/example/printer.h index 066facf1e26..fbdb35c057e 100644 --- a/example/printer.h +++ b/example/printer.h @@ -1,4 +1,4 @@ -// Copyright (c) 2021 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/example/types.h b/example/types.h index 0c0bd9342b9..c926a00b40d 100644 --- a/example/types.h +++ b/example/types.h @@ -1,14 +1,23 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #ifndef EXAMPLE_TYPES_H #define EXAMPLE_TYPES_H +#include +#include + +// IWYU pragma: begin_exports #include #include #include #include #include +// IWYU pragma: end_exports + +struct InitInterface; // IWYU pragma: export +struct CalculatorInterface; // IWYU pragma: export +struct PrinterInterface; // IWYU pragma: export #endif // EXAMPLE_TYPES_H diff --git a/include/mp/config.h.in b/include/mp/config.h.in index 79ebc4790b8..9d3c62409ae 100644 --- a/include/mp/config.h.in +++ b/include/mp/config.h.in @@ -1,4 +1,4 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h index dff8c2a63a4..367a9bebbc3 100644 --- a/include/mp/proxy-io.h +++ b/include/mp/proxy-io.h @@ -1,4 +1,4 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -13,12 +13,15 @@ #include #include +#include #include -#include +#include #include #include +#include #include #include +#include namespace mp { struct ThreadContext; @@ -63,16 +66,18 @@ struct ProxyClient : public ProxyClientBase ProxyClient(const ProxyClient&) = delete; ~ProxyClient(); - void setCleanup(const std::function& fn); + void setDisconnectCallback(const 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_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_it; + //! Reference to callback function that is run if there is a sudden + //! disconnect and the Connection object is destroyed before this + //! ProxyClient object. The callback will destroy this object and + //! remove its entry from the thread's request_threads or callback_threads + //! map. It will also reset m_disconnect_cb so the destructor does not + //! access it. In the normal case where there is no sudden disconnect, the + //! destructor will unregister m_disconnect_cb so the callback is never run. + //! Since this variable is accessed from multiple threads, accesses should + //! be guarded with the associated Waiter::m_mutex. + std::optional m_disconnect_cb; }; template <> @@ -129,6 +134,28 @@ std::string LongThreadName(const char* exe_name); //! Event loop implementation. //! +//! Cap'n Proto threading model is very simple: all I/O operations are +//! asynchronous and must be performed on a single thread. This includes: +//! +//! - Code starting an asynchronous operation (calling a function that returns a +//! promise object) +//! - Code notifying that an asynchronous operation is complete (code using a +//! fulfiller object) +//! - Code handling a completed operation (code chaining or waiting for a promise) +//! +//! All of this code needs to access shared state, and there is no mutex that +//! can be acquired to lock this state because Cap'n Proto +//! assumes it will only be accessed from one thread. So all this code needs to +//! actually run on one thread, and the EventLoop::loop() method is the entry point for +//! this thread. ProxyClient and ProxyServer objects that use other threads and +//! need to perform I/O operations post to this thread using EventLoop::post() +//! and EventLoop::sync() methods. +//! +//! Specifically, because ProxyClient methods can be called from arbitrary +//! threads, and ProxyServer methods can run on arbitrary threads, ProxyClient +//! methods use the EventLoop thread to send requests, and ProxyServer methods +//! use the thread to return results. +//! //! Based on https://groups.google.com/d/msg/capnproto/TuQFF1eH2-M/g81sHaTAAQAJ class EventLoop { @@ -144,7 +171,7 @@ public: //! Run function on event loop thread. Does not return until function completes. //! Must be called while the loop() function is active. - void post(const std::function& fn); + void post(kj::Function fn); //! Wrapper around EventLoop::post that takes advantage of the //! fact that callable will not go out of scope to avoid requirement that it @@ -152,9 +179,13 @@ public: template void sync(Callable&& callable) { - post(std::ref(callable)); + post(std::forward(callable)); } + //! Register cleanup function to run on asynchronous worker thread without + //! blocking the event loop thread. + void addAsyncCleanup(std::function fn); + //! Start asynchronous worker thread if necessary. This is only done if //! there are ProxyServerBase::m_impl objects that need to be destroyed //! asynchronously, without tying up the event loop thread. This can happen @@ -166,13 +197,10 @@ public: //! is important that ProxyServer::m_impl destructors do not run on the //! eventloop thread because they may need it to do I/O if they perform //! other IPC calls. - void startAsyncThread(std::unique_lock& lock); + void startAsyncThread() MP_REQUIRES(m_mutex); - //! Add/remove remote client reference counts. - void addClient(std::unique_lock& lock); - bool removeClient(std::unique_lock& lock); //! Check if loop should exit. - bool done(std::unique_lock& lock) const; + bool done() const MP_REQUIRES(m_mutex); Logger log() { @@ -195,10 +223,10 @@ public: std::thread m_async_thread; //! Callback function to run on event loop thread during post() or sync() call. - const std::function* m_post_fn = nullptr; + kj::Function* m_post_fn MP_GUARDED_BY(m_mutex) = nullptr; //! Callback functions to run on async thread. - CleanupList m_async_fns; + std::optional m_async_fns MP_GUARDED_BY(m_mutex); //! Pipe read handle used to wake up the event loop thread. int m_wait_fd = -1; @@ -208,11 +236,11 @@ public: //! Number of clients holding references to ProxyServerBase objects that //! reference this event loop. - int m_num_clients = 0; + int m_num_clients MP_GUARDED_BY(m_mutex) = 0; //! Mutex and condition variable used to post tasks to event loop and async //! thread. - std::mutex m_mutex; + Mutex m_mutex; std::condition_variable m_cv; //! Capnp IO context. @@ -263,20 +291,25 @@ struct Waiter // in the case where a capnp response is sent and a brand new // request is immediately received. while (m_fn) { - auto fn = std::move(m_fn); - m_fn = nullptr; - lock.unlock(); - fn(); - lock.lock(); + auto fn = std::move(*m_fn); + m_fn.reset(); + Unlock(lock, fn); } const bool done = pred(); return done; }); } + //! Mutex mainly used internally by waiter class, but also used externally + //! to guard access to related state. Specifically, since the thread_local + //! ThreadContext struct owns a Waiter, the Waiter::m_mutex is used to guard + //! access to other parts of the struct to avoid needing to deal with more + //! mutexes than necessary. This mutex can be held at the same time as + //! EventLoop::m_mutex as long as Waiter::mutex is locked first and + //! EventLoop::m_mutex is locked second. std::mutex m_mutex; std::condition_variable m_cv; - std::function m_fn; + std::optional> m_fn; }; //! Object holding network & rpc state associated with either an incoming server @@ -290,21 +323,13 @@ public: Connection(EventLoop& loop, kj::Own&& stream_) : m_loop(loop), m_stream(kj::mv(stream_)), m_network(*m_stream, ::capnp::rpc::twoparty::Side::CLIENT, ::capnp::ReaderOptions()), - m_rpc_system(::capnp::makeRpcClient(m_network)) - { - std::unique_lock lock(m_loop.m_mutex); - m_loop.addClient(lock); - } + m_rpc_system(::capnp::makeRpcClient(m_network)) {} Connection(EventLoop& loop, kj::Own&& stream_, const std::function<::capnp::Capability::Client(Connection&)>& make_client) : m_loop(loop), m_stream(kj::mv(stream_)), m_network(*m_stream, ::capnp::rpc::twoparty::Side::SERVER, ::capnp::ReaderOptions()), - m_rpc_system(::capnp::makeRpcServer(m_network, make_client(*this))) - { - std::unique_lock lock(m_loop.m_mutex); - m_loop.addClient(lock); - } + m_rpc_system(::capnp::makeRpcServer(m_network, make_client(*this))) {} //! Run cleanup functions. Must be called from the event loop thread. First //! calls synchronous cleanup functions while blocked (to free capnp @@ -319,10 +344,6 @@ public: CleanupIt addSyncCleanup(std::function fn); void removeSyncCleanup(CleanupIt it); - //! Register asynchronous cleanup function to run on worker thread when - //! disconnect() is called. - void addAsyncCleanup(std::function fn); - //! Add disconnect handler. template void onDisconnect(F&& f) @@ -333,12 +354,12 @@ public: // to the EventLoop TaskSet to avoid "Promise callback destroyed itself" // error in cases where f deletes this Connection object. m_on_disconnect.add(m_network.onDisconnect().then( - [f = std::forward(f), this]() mutable { m_loop.m_task_set->add(kj::evalLater(kj::mv(f))); })); + [f = std::forward(f), this]() mutable { m_loop->m_task_set->add(kj::evalLater(kj::mv(f))); })); } - EventLoop& m_loop; + EventLoopRef m_loop; kj::Own m_stream; - LoggingErrorHandler m_error_handler{m_loop}; + LoggingErrorHandler m_error_handler{*m_loop}; kj::TaskSet m_on_disconnect{m_error_handler}; ::capnp::TwoPartyVatNetwork m_network; std::optional<::capnp::RpcSystem<::capnp::rpc::twoparty::VatId>> m_rpc_system; @@ -351,11 +372,10 @@ public: //! ThreadMap.makeThread) used to service requests to clients. ::capnp::CapabilityServerSet m_threads; - //! Cleanup functions to run if connection is broken unexpectedly. - //! Lists will be empty if all ProxyClient and ProxyServer objects are - //! destroyed cleanly before the connection is destroyed. + //! Cleanup functions to run if connection is broken unexpectedly. List + //! will be empty if all ProxyClient are destroyed cleanly before the + //! connection is destroyed. CleanupList m_sync_cleanup_fns; - CleanupList m_async_cleanup_fns; }; //! Vat id for server side of connection. Required argument to RpcSystem::bootStrap() @@ -381,21 +401,13 @@ ProxyClientBase::ProxyClientBase(typename Interface::Client cli : m_client(std::move(client)), m_context(connection) { - { - std::unique_lock lock(m_context.connection->m_loop.m_mutex); - m_context.connection->m_loop.addClient(lock); - } - // Handler for the connection getting destroyed before this client object. - auto cleanup_it = m_context.connection->addSyncCleanup([this]() { + auto disconnect_cb = m_context.connection->addSyncCleanup([this]() { // Release client capability by move-assigning to temporary. { typename Interface::Client(std::move(m_client)); } - { - std::unique_lock lock(m_context.connection->m_loop.m_mutex); - m_context.connection->m_loop.removeClient(lock); - } + Lock lock{m_context.loop->m_mutex}; m_context.connection = nullptr; }); @@ -408,14 +420,10 @@ ProxyClientBase::ProxyClientBase(typename Interface::Client cli // down while external code is still holding client references. // // 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_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_it); - + // second case is handled by the disconnect_cb function, which sets + // m_context.connection to null so nothing happens here. + m_context.cleanup_fns.emplace_front([this, destroy_connection, disconnect_cb]{ + { // 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 // capnp interface does not define a destroy method, this will just call @@ -423,16 +431,19 @@ ProxyClientBase::ProxyClientBase(typename Interface::Client cli Sub::destroy(*this); // FIXME: Could just invoke removed addCleanup fn here instead of duplicating code - m_context.connection->m_loop.sync([&]() { + m_context.loop->sync([&]() { + // Remove disconnect callback on cleanup so it doesn't run and try + // to access this object after it's destroyed. This call needs to + // run inside loop->sync() on the event loop thread because + // otherwise, if there were an ill-timed disconnect, the + // onDisconnect handler could fire and delete the Connection object + // before the removeSyncCleanup call. + if (m_context.connection) m_context.connection->removeSyncCleanup(disconnect_cb); + // Release client capability by move-assigning to temporary. { typename Interface::Client(std::move(m_client)); } - { - std::unique_lock lock(m_context.connection->m_loop.m_mutex); - m_context.connection->m_loop.removeClient(lock); - } - if (destroy_connection) { delete m_context.connection; m_context.connection = nullptr; @@ -454,12 +465,20 @@ ProxyServerBase::ProxyServerBase(std::shared_ptr impl, Co : m_impl(std::move(impl)), m_context(&connection) { assert(m_impl); - std::unique_lock lock(m_context.connection->m_loop.m_mutex); - m_context.connection->m_loop.addClient(lock); } //! ProxyServer destructor, called from the EventLoop thread by Cap'n Proto //! garbage collection code after there are no more references to this object. +//! This will typically happen when the corresponding ProxyClient object on the +//! other side of the connection is destroyed. It can also happen earlier if the +//! connection is broken or destroyed. In the latter case this destructor will +//! typically be called inside m_rpc_system.reset() call in the ~Connection +//! destructor while the Connection object still exists. However, because +//! ProxyServer objects are refcounted, and the Connection object could be +//! destroyed while asynchronous IPC calls are still in-flight, it's possible +//! for this destructor to be called after the Connection object no longer +//! exists, so it is NOT valid to dereference the m_context.connection pointer +//! from this function. template ProxyServerBase::~ProxyServerBase() { @@ -483,14 +502,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), fns=std::move(m_context.cleanup_fns)]() mutable { + m_context.loop->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable { impl.reset(); CleanupRun(fns); }); } assert(m_context.cleanup_fns.empty()); - std::unique_lock lock(m_context.connection->m_loop.m_mutex); - m_context.connection->m_loop.removeClient(lock); } //! If the capnp interface defined a special "destroy" method, as described the diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index a74c6de0b99..de96d134c21 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -1,4 +1,4 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -92,9 +92,9 @@ struct StructField template struct ReadDestEmplace { - ReadDestEmplace(TypeList, EmplaceFn&& emplace_fn) : m_emplace_fn(emplace_fn) {} + ReadDestEmplace(TypeList, EmplaceFn emplace_fn) : m_emplace_fn(std::move(emplace_fn)) {} - //! Simple case. If ReadField impementation calls this construct() method + //! Simple case. If ReadField implementation calls this construct() method //! with constructor arguments, just pass them on to the emplace function. template decltype(auto) construct(Args&&... args) @@ -123,7 +123,7 @@ struct ReadDestEmplace return temp; } } - EmplaceFn& m_emplace_fn; + EmplaceFn m_emplace_fn; }; //! Helper function to create a ReadDestEmplace object that constructs a @@ -131,7 +131,7 @@ struct ReadDestEmplace template auto ReadDestTemp() { - return ReadDestEmplace{TypeList(), [&](auto&&... args) -> decltype(auto) { + return ReadDestEmplace{TypeList(), [](auto&&... args) -> decltype(auto) { return LocalType{std::forward(args)...}; }}; } @@ -191,7 +191,7 @@ void ThrowField(TypeList, InvokeContext& invoke_context, Input&& } template -bool CustomHasValue(InvokeContext& invoke_context, Values&&... value) +bool CustomHasValue(InvokeContext& invoke_context, const Values&... value) { return true; } @@ -199,7 +199,7 @@ bool CustomHasValue(InvokeContext& invoke_context, Values&&... value) template void BuildField(TypeList, Context& context, Output&& output, Values&&... values) { - if (CustomHasValue(context, std::forward(values)...)) { + if (CustomHasValue(context, values...)) { CustomBuildField(TypeList(), Priority<3>(), context, std::forward(values)..., std::forward(output)); } @@ -274,7 +274,7 @@ void MaybeReadField(std::false_type, Args&&...) } template -void MaybeSetWant(TypeList, Priority<1>, Value&& value, Output&& output) +void MaybeSetWant(TypeList, Priority<1>, const Value& value, Output&& output) { if (value) { output.setWant(); @@ -282,7 +282,7 @@ void MaybeSetWant(TypeList, Priority<1>, Value&& value, Output&& out } template -void MaybeSetWant(LocalTypes, Priority<0>, Args&&...) +void MaybeSetWant(LocalTypes, Priority<0>, const Args&...) { } @@ -326,18 +326,18 @@ template struct IterateFieldsHelper { template - void handleChain(Arg1&& arg1, Arg2&& arg2, ParamList, NextFn&& next_fn, NextFnArgs&&... next_fn_args) + void handleChain(Arg1& arg1, Arg2& arg2, ParamList, NextFn&& next_fn, NextFnArgs&&... next_fn_args) { using S = Split; - handleChain(std::forward(arg1), std::forward(arg2), typename S::First()); - next_fn.handleChain(std::forward(arg1), std::forward(arg2), typename S::Second(), + handleChain(arg1, arg2, typename S::First()); + next_fn.handleChain(arg1, arg2, typename S::Second(), std::forward(next_fn_args)...); } template - void handleChain(Arg1&& arg1, Arg2&& arg2, ParamList) + void handleChain(Arg1& arg1, Arg2& arg2, ParamList) { - static_cast(this)->handleField(std::forward(arg1), std::forward(arg2), ParamList()); + static_cast(this)->handleField(arg1, arg2, ParamList()); } private: IterateFieldsHelper() = default; @@ -393,10 +393,10 @@ struct ClientParam void handleField(ClientInvokeContext& invoke_context, Params& params, ParamList) { auto const fun = [&](Values&&... values) { + MaybeSetWant( + ParamList(), Priority<1>(), values..., Make(params)); MaybeBuildField(std::integral_constant(), ParamList(), invoke_context, Make(params), std::forward(values)...); - MaybeSetWant( - ParamList(), Priority<1>(), std::forward(values)..., Make(params)); }; // Note: The m_values tuple just consists of lvalue and rvalue @@ -568,7 +568,7 @@ template void clientDestroy(Client& client) { if (client.m_context.connection) { - client.m_context.connection->m_loop.log() << "IPC client destroy " << typeid(client).name(); + client.m_context.loop->log() << "IPC client destroy " << typeid(client).name(); } else { KJ_LOG(INFO, "IPC interrupted client destroy", typeid(client).name()); } @@ -577,7 +577,7 @@ void clientDestroy(Client& client) template void serverDestroy(Server& server) { - server.m_context.connection->m_loop.log() << "IPC server destroy " << typeid(server).name(); + server.m_context.loop->log() << "IPC server destroy " << typeid(server).name(); } //! Entry point called by generated client code that looks like: @@ -592,12 +592,9 @@ void serverDestroy(Server& server) template void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, FieldObjs&&... fields) { - if (!proxy_client.m_context.connection) { - throw std::logic_error("clientInvoke call made after disconnect"); - } if (!g_thread_context.waiter) { assert(g_thread_context.thread_name.empty()); - g_thread_context.thread_name = ThreadName(proxy_client.m_context.connection->m_loop.m_exe_name); + g_thread_context.thread_name = ThreadName(proxy_client.m_context.loop->m_exe_name); // If next assert triggers, it means clientInvoke is being called from // the capnp event loop thread. This can happen when a ProxyServer // method implementation that runs synchronously on the event loop @@ -608,52 +605,68 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel // declaration so the server method runs in a dedicated thread. assert(!g_thread_context.loop_thread); g_thread_context.waiter = std::make_unique(); - proxy_client.m_context.connection->m_loop.logPlain() + proxy_client.m_context.loop->logPlain() << "{" << g_thread_context.thread_name << "} IPC client first request from current thread, constructing waiter"; } - ClientInvokeContext invoke_context{*proxy_client.m_context.connection, g_thread_context}; + ThreadContext& thread_context{g_thread_context}; + std::optional invoke_context; // Must outlive waiter->wait() call below std::exception_ptr exception; std::string kj_exception; bool done = false; - proxy_client.m_context.connection->m_loop.sync([&]() { + const char* disconnected = nullptr; + proxy_client.m_context.loop->sync([&]() { + if (!proxy_client.m_context.connection) { + const std::unique_lock lock(thread_context.waiter->m_mutex); + done = true; + disconnected = "IPC client method called after disconnect."; + thread_context.waiter->m_cv.notify_all(); + return; + } + auto request = (proxy_client.m_client.*get_request)(nullptr); using Request = CapRequestTraits; using FieldList = typename ProxyClientMethodTraits::Fields; - IterateFields().handleChain(invoke_context, request, FieldList(), typename FieldObjs::BuildParams{&fields}...); - proxy_client.m_context.connection->m_loop.logPlain() - << "{" << invoke_context.thread_context.thread_name << "} IPC client send " + invoke_context.emplace(*proxy_client.m_context.connection, thread_context); + IterateFields().handleChain(*invoke_context, request, FieldList(), typename FieldObjs::BuildParams{&fields}...); + proxy_client.m_context.loop->logPlain() + << "{" << thread_context.thread_name << "} IPC client send " << TypeName() << " " << LogEscape(request.toString()); - proxy_client.m_context.connection->m_loop.m_task_set->add(request.send().then( + proxy_client.m_context.loop->m_task_set->add(request.send().then( [&](::capnp::Response&& response) { - proxy_client.m_context.connection->m_loop.logPlain() - << "{" << invoke_context.thread_context.thread_name << "} IPC client recv " + proxy_client.m_context.loop->logPlain() + << "{" << thread_context.thread_name << "} IPC client recv " << TypeName() << " " << LogEscape(response.toString()); try { IterateFields().handleChain( - invoke_context, response, FieldList(), typename FieldObjs::ReadResults{&fields}...); + *invoke_context, response, FieldList(), typename FieldObjs::ReadResults{&fields}...); } catch (...) { exception = std::current_exception(); } - const std::unique_lock lock(invoke_context.thread_context.waiter->m_mutex); + const std::unique_lock lock(thread_context.waiter->m_mutex); done = true; - invoke_context.thread_context.waiter->m_cv.notify_all(); + thread_context.waiter->m_cv.notify_all(); }, [&](const ::kj::Exception& e) { - kj_exception = kj::str("kj::Exception: ", e).cStr(); - proxy_client.m_context.connection->m_loop.logPlain() - << "{" << invoke_context.thread_context.thread_name << "} IPC client exception " << kj_exception; - const std::unique_lock lock(invoke_context.thread_context.waiter->m_mutex); + if (e.getType() == ::kj::Exception::Type::DISCONNECTED) { + disconnected = "IPC client method call interrupted by disconnect."; + } else { + kj_exception = kj::str("kj::Exception: ", e).cStr(); + proxy_client.m_context.loop->logPlain() + << "{" << thread_context.thread_name << "} IPC client exception " << kj_exception; + } + const std::unique_lock lock(thread_context.waiter->m_mutex); done = true; - invoke_context.thread_context.waiter->m_cv.notify_all(); + thread_context.waiter->m_cv.notify_all(); })); }); - std::unique_lock lock(invoke_context.thread_context.waiter->m_mutex); - invoke_context.thread_context.waiter->wait(lock, [&done]() { return done; }); + std::unique_lock lock(thread_context.waiter->m_mutex); + thread_context.waiter->wait(lock, [&done]() { return done; }); if (exception) std::rethrow_exception(exception); - if (!kj_exception.empty()) proxy_client.m_context.connection->m_loop.raise() << kj_exception; + if (!kj_exception.empty()) proxy_client.m_context.loop->raise() << kj_exception; + if (disconnected) proxy_client.m_context.loop->raise() << disconnected; } //! Invoke callable `fn()` that may return void. If it does return void, replace @@ -687,7 +700,7 @@ kj::Promise serverInvoke(Server& server, CallContext& call_context, Fn fn) using Results = typename decltype(call_context.getResults())::Builds; int req = ++server_reqs; - server.m_context.connection->m_loop.log() << "IPC server recv request #" << req << " " + server.m_context.loop->log() << "IPC server recv request #" << req << " " << TypeName() << " " << LogEscape(params.toString()); try { @@ -704,14 +717,14 @@ kj::Promise serverInvoke(Server& server, CallContext& call_context, Fn fn) return ReplaceVoid([&]() { return fn.invoke(server_context, ArgList()); }, [&]() { return kj::Promise(kj::mv(call_context)); }) .then([&server, req](CallContext call_context) { - server.m_context.connection->m_loop.log() << "IPC server send response #" << req << " " << TypeName() + server.m_context.loop->log() << "IPC server send response #" << req << " " << TypeName() << " " << LogEscape(call_context.getResults().toString()); }); } catch (const std::exception& e) { - server.m_context.connection->m_loop.log() << "IPC server unhandled exception: " << e.what(); + server.m_context.loop->log() << "IPC server unhandled exception: " << e.what(); throw; } catch (...) { - server.m_context.connection->m_loop.log() << "IPC server unhandled exception"; + server.m_context.loop->log() << "IPC server unhandled exception"; throw; } } diff --git a/include/mp/proxy.capnp b/include/mp/proxy.capnp index abd02e437fc..386f8f7abe0 100644 --- a/include/mp/proxy.capnp +++ b/include/mp/proxy.capnp @@ -1,4 +1,4 @@ -# Copyright (c) 2019 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/proxy.h b/include/mp/proxy.h index e7faad9a666..fff511fde06 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -1,4 +1,4 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -7,34 +7,31 @@ #include -#include +#include #include #include +#include #include #include #include #include +#include // IWYU pragma: keep namespace mp { class Connection; class EventLoop; //! Mapping from capnp interface type to proxy client implementation (specializations are generated by //! proxy-codegen.cpp). -template -struct ProxyClient; +template struct ProxyClient; // IWYU pragma: export //! Mapping from capnp interface type to proxy server implementation (specializations are generated by //! proxy-codegen.cpp). -template -struct ProxyServer; +template struct ProxyServer; // IWYU pragma: export //! Mapping from capnp method params type to method traits (specializations are generated by proxy-codegen.cpp). -template -struct ProxyMethod; +template struct ProxyMethod; // IWYU pragma: export //! Mapping from capnp struct type to struct traits (specializations are generated by proxy-codegen.cpp). -template -struct ProxyStruct; +template struct ProxyStruct; // IWYU pragma: export //! Mapping from local c++ type to capnp type and traits (specializations are generated by proxy-codegen.cpp). -template -struct ProxyType; +template struct ProxyType; // IWYU pragma: export using CleanupList = std::list>; using CleanupIt = typename CleanupList::iterator; @@ -47,13 +44,34 @@ inline void CleanupRun(CleanupList& fns) { } } +//! Event loop smart pointer automatically managing m_num_clients. +//! If a lock pointer argument is passed, the specified lock will be used, +//! otherwise EventLoop::m_mutex will be locked when needed. +class EventLoopRef +{ +public: + explicit EventLoopRef(EventLoop& loop, Lock* lock = nullptr); + EventLoopRef(EventLoopRef&& other) noexcept : m_loop(other.m_loop) { other.m_loop = nullptr; } + EventLoopRef(const EventLoopRef&) = delete; + EventLoopRef& operator=(const EventLoopRef&) = delete; + EventLoopRef& operator=(EventLoopRef&&) = delete; + ~EventLoopRef() { reset(); } + EventLoop& operator*() const { assert(m_loop); return *m_loop; } + EventLoop* operator->() const { assert(m_loop); return m_loop; } + void reset(bool relock=false); + + EventLoop* m_loop{nullptr}; + Lock* m_lock{nullptr}; +}; + //! Context data associated with proxy client and server classes. struct ProxyContext { Connection* connection; + EventLoopRef loop; CleanupList cleanup_fns; - ProxyContext(Connection* connection) : connection(connection) {} + ProxyContext(Connection* connection); }; //! Base class for generated ProxyClient classes that implement a C++ interface @@ -67,6 +85,15 @@ public: using Sub = ProxyClient; using Super = ProxyClientBase; + //! Construct libmultiprocess client object wrapping Cap'n Proto client + //! object with a reference to the associated mp::Connection object. + //! + //! The destroy_connection option determines whether destroying this client + //! object closes the connection. It is set to true for the + //! ProxyClient object returned by ConnectStream, to let IPC + //! clients close the connection by freeing the object. It is false for + //! other client objects so they can be destroyed without affecting the + //! connection. ProxyClientBase(typename Interface::Client client, Connection* connection, bool destroy_connection); ~ProxyClientBase() noexcept; diff --git a/include/mp/type-char.h b/include/mp/type-char.h index d1d27b62414..b51ffeb1928 100644 --- a/include/mp/type-char.h +++ b/include/mp/type-char.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-chrono.h b/include/mp/type-chrono.h index a17d9a994bf..d71549864c1 100644 --- a/include/mp/type-chrono.h +++ b/include/mp/type-chrono.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-context.h b/include/mp/type-context.h index 7c12afe2ff0..894daadb36a 100644 --- a/include/mp/type-context.h +++ b/include/mp/type-context.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -64,13 +64,11 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& auto future = kj::newPromiseAndFulfiller(); auto& server = server_context.proxy_server; int req = server_context.req; - auto invoke = MakeAsyncCallable( - [fulfiller = kj::mv(future.fulfiller), + auto invoke = [fulfiller = kj::mv(future.fulfiller), call_context = kj::mv(server_context.call_context), &server, req, fn, args...]() mutable { const auto& params = call_context.getParams(); Context::Reader context_arg = Accessor::get(params); ServerContext server_context{server, call_context, req}; - bool disconnected{false}; { // Before invoking the function, store a reference to the // callbackThread provided by the client in the @@ -102,7 +100,7 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& // recursive call (IPC call calling back to the caller which // makes another IPC call), so avoid modifying the map. const bool erase_thread{inserted}; - KJ_DEFER({ + KJ_DEFER(if (erase_thread) { std::unique_lock lock(thread_context.waiter->m_mutex); // Call erase here with a Connection* argument instead // of an iterator argument, because the `request_thread` @@ -113,54 +111,40 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& // erases the thread from the map, and also because the // ProxyServer destructor calls // request_threads.clear(). - if (erase_thread) { - disconnected = !request_threads.erase(server.m_context.connection); - } else { - disconnected = !request_threads.count(server.m_context.connection); - } + request_threads.erase(server.m_context.connection); }); fn.invoke(server_context, args...); } - if (disconnected) { - // If disconnected is true, the Connection object was - // destroyed during the method call. Deal with this by - // returning without ever fulfilling the promise, which will - // cause the ProxyServer object to leak. This is not ideal, - // but fixing the leak will require nontrivial code changes - // because there is a lot of code assuming ProxyServer - // objects are destroyed before Connection objects. - return; - } KJ_IF_MAYBE(exception, kj::runCatchingExceptions([&]() { - server.m_context.connection->m_loop.sync([&] { + server.m_context.loop->sync([&] { auto fulfiller_dispose = kj::mv(fulfiller); fulfiller_dispose->fulfill(kj::mv(call_context)); }); })) { - server.m_context.connection->m_loop.sync([&]() { + server.m_context.loop->sync([&]() { auto fulfiller_dispose = kj::mv(fulfiller); fulfiller_dispose->reject(kj::mv(*exception)); }); } - }); + }; // Lookup Thread object specified by the client. The specified thread should // be a local Thread::Server object, but it needs to be looked up // asynchronously with getLocalServer(). auto thread_client = context_arg.getThread(); return server.m_context.connection->m_threads.getLocalServer(thread_client) - .then([&server, invoke, req](const kj::Maybe& perhaps) { + .then([&server, invoke = kj::mv(invoke), req](const kj::Maybe& perhaps) mutable { // Assuming the thread object is found, pass it a pointer to the // `invoke` lambda above which will invoke the function on that // thread. KJ_IF_MAYBE (thread_server, perhaps) { const auto& thread = static_cast&>(*thread_server); - server.m_context.connection->m_loop.log() + server.m_context.loop->log() << "IPC server post request #" << req << " {" << thread.m_thread_context.thread_name << "}"; thread.m_thread_context.waiter->post(std::move(invoke)); } else { - server.m_context.connection->m_loop.log() + server.m_context.loop->log() << "IPC server error request #" << req << ", missing thread to execute request"; throw std::runtime_error("invalid thread handle"); } diff --git a/include/mp/type-data.h b/include/mp/type-data.h index 46a2b2fc725..5da4cfce5f4 100644 --- a/include/mp/type-data.h +++ b/include/mp/type-data.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-decay.h b/include/mp/type-decay.h index 7b203c8628c..65934372e4a 100644 --- a/include/mp/type-decay.h +++ b/include/mp/type-decay.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-exception.h b/include/mp/type-exception.h index 3e2fcac2737..3f04d7a97f5 100644 --- a/include/mp/type-exception.h +++ b/include/mp/type-exception.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-function.h b/include/mp/type-function.h index bf00c581197..47d3b30be29 100644 --- a/include/mp/type-function.h +++ b/include/mp/type-function.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -24,7 +24,7 @@ template void CustomBuildField(TypeList>, Priority<1>, InvokeContext& invoke_context, - Value& value, + Value&& value, Output&& output) { if (value) { diff --git a/include/mp/type-interface.h b/include/mp/type-interface.h index 8a89ac24fca..7a98b4afab4 100644 --- a/include/mp/type-interface.h +++ b/include/mp/type-interface.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-map.h b/include/mp/type-map.h index bc1b22769df..6fa5623e510 100644 --- a/include/mp/type-map.h +++ b/include/mp/type-map.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-message.h b/include/mp/type-message.h index d80f43c8d37..baa46eb0cad 100644 --- a/include/mp/type-message.h +++ b/include/mp/type-message.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-number.h b/include/mp/type-number.h index 9d269be60a8..5c997f54bc8 100644 --- a/include/mp/type-number.h +++ b/include/mp/type-number.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -50,8 +50,14 @@ decltype(auto) CustomReadField(TypeList, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest, - typename std::enable_if::value>::type* enable = 0) + typename std::enable_if::value>::type* enable = nullptr) { + // Disable clang-tidy out-of-range enum value check which triggers when + // using an enum type that does not have a 0 value. The check correctly + // triggers when it detects that Cap'n Proto returns 0 when reading an + // integer field that is unset. But the warning is spurious because the + // corresponding BuildField call should never leave the field unset. + // NOLINTNEXTLINE(clang-analyzer-optin.core.EnumCastOutOfRange) return read_dest.construct(static_cast(input.get())); } diff --git a/include/mp/type-optional.h b/include/mp/type-optional.h index 822508d5533..6f23fd4d521 100644 --- a/include/mp/type-optional.h +++ b/include/mp/type-optional.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-pair.h b/include/mp/type-pair.h index 3af9c9313de..b1914c9d9c6 100644 --- a/include/mp/type-pair.h +++ b/include/mp/type-pair.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-pointer.h b/include/mp/type-pointer.h index 5c79e8d2e8a..98b7aa817fe 100644 --- a/include/mp/type-pointer.h +++ b/include/mp/type-pointer.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-set.h b/include/mp/type-set.h index ea60dc4d1a0..699c6e9e03a 100644 --- a/include/mp/type-set.h +++ b/include/mp/type-set.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-string.h b/include/mp/type-string.h index 77d04acb280..d4c3383bdfe 100644 --- a/include/mp/type-string.h +++ b/include/mp/type-string.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-struct.h b/include/mp/type-struct.h index d282e20e9a4..6d396387ffe 100644 --- a/include/mp/type-struct.h +++ b/include/mp/type-struct.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-threadmap.h b/include/mp/type-threadmap.h index 683586fbc6e..3005d9de0fc 100644 --- a/include/mp/type-threadmap.h +++ b/include/mp/type-threadmap.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-tuple.h b/include/mp/type-tuple.h index 5083887258f..597ffbfb270 100644 --- a/include/mp/type-tuple.h +++ b/include/mp/type-tuple.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-vector.h b/include/mp/type-vector.h index e4996e93043..90605ddf864 100644 --- a/include/mp/type-vector.h +++ b/include/mp/type-vector.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/type-void.h b/include/mp/type-void.h index 0a887680529..ed733985b29 100644 --- a/include/mp/type-void.h +++ b/include/mp/type-void.h @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. diff --git a/include/mp/util.h b/include/mp/util.h index 8b802abc9f3..d9f3ca3e9b5 100644 --- a/include/mp/util.h +++ b/include/mp/util.h @@ -1,4 +1,4 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -6,18 +6,17 @@ #define MP_UTIL_H #include +#include #include +#include #include -#include -#include -#include #include -#include -#include +#include #include #include #include #include +#include #include namespace mp { @@ -130,6 +129,59 @@ const char* TypeName() return short_name ? short_name + 1 : display_name; } +//! Convenient wrapper around std::variant +template +struct PtrOrValue { + std::variant data; + + template + PtrOrValue(T* ptr, Args&&... args) : data(ptr ? ptr : std::variant{std::in_place_type, std::forward(args)...}) {} + + T& operator*() { return data.index() ? std::get(data) : *std::get(data); } + T* operator->() { return &**this; } + T& operator*() const { return data.index() ? std::get(data) : *std::get(data); } + T* operator->() const { return &**this; } +}; + +// Annotated mutex and lock class (https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) +#if defined(__clang__) && (!defined(SWIG)) +#define MP_TSA(x) __attribute__((x)) +#else +#define MP_TSA(x) // no-op +#endif + +#define MP_CAPABILITY(x) MP_TSA(capability(x)) +#define MP_SCOPED_CAPABILITY MP_TSA(scoped_lockable) +#define MP_REQUIRES(x) MP_TSA(requires_capability(x)) +#define MP_ACQUIRE(...) MP_TSA(acquire_capability(__VA_ARGS__)) +#define MP_RELEASE(...) MP_TSA(release_capability(__VA_ARGS__)) +#define MP_ASSERT_CAPABILITY(x) MP_TSA(assert_capability(x)) +#define MP_GUARDED_BY(x) MP_TSA(guarded_by(x)) +#define MP_NO_TSA MP_TSA(no_thread_safety_analysis) + +class MP_CAPABILITY("mutex") Mutex { +public: + void lock() MP_ACQUIRE() { m_mutex.lock(); } + void unlock() MP_RELEASE() { m_mutex.unlock(); } + + std::mutex m_mutex; +}; + +class MP_SCOPED_CAPABILITY Lock { +public: + explicit Lock(Mutex& m) MP_ACQUIRE(m) : m_lock(m.m_mutex) {} + ~Lock() MP_RELEASE() = default; + void unlock() MP_RELEASE() { m_lock.unlock(); } + void lock() MP_ACQUIRE() { m_lock.lock(); } + void assert_locked(Mutex& mutex) MP_ASSERT_CAPABILITY() MP_ASSERT_CAPABILITY(mutex) + { + assert(m_lock.mutex() == &mutex.m_mutex); + assert(m_lock); + } + + std::unique_lock m_lock; +}; + //! Analog to std::lock_guard that unlocks instead of locks. template struct UnlockGuard @@ -146,46 +198,6 @@ void Unlock(Lock& lock, Callback&& callback) callback(); } -//! Needed for libc++/macOS compatibility. Lets code work with shared_ptr nothrow declaration -//! https://github.com/capnproto/capnproto/issues/553#issuecomment-328554603 -template -struct DestructorCatcher -{ - T value; - template - DestructorCatcher(Params&&... params) : value(kj::fwd(params)...) - { - } - ~DestructorCatcher() noexcept try { - } catch (const kj::Exception& e) { // NOLINT(bugprone-empty-catch) - } -}; - -//! Wrapper around callback function for compatibility with std::async. -//! -//! std::async requires callbacks to be copyable and requires noexcept -//! destructors, but this doesn't work well with kj types which are generally -//! move-only and not noexcept. -template -struct AsyncCallable -{ - AsyncCallable(Callable&& callable) : m_callable(std::make_shared>(std::move(callable))) - { - } - AsyncCallable(const AsyncCallable&) = default; - AsyncCallable(AsyncCallable&&) = default; - ~AsyncCallable() noexcept = default; - ResultOf operator()() const { return (m_callable->value)(); } - mutable std::shared_ptr> m_callable; -}; - -//! Construct AsyncCallable object. -template -AsyncCallable> MakeAsyncCallable(Callable&& callable) -{ - return std::forward(callable); -} - //! Format current thread name as "{exe_name}-{$pid}/{thread_name}-{$tid}". std::string ThreadName(const char* exe_name); diff --git a/shell.nix b/shell.nix new file mode 100644 index 00000000000..eacfdc2a853 --- /dev/null +++ b/shell.nix @@ -0,0 +1,28 @@ +{ pkgs ? import {} +, crossPkgs ? import {} +, enableLibcxx ? false # Whether to use libc++ toolchain and libraries instead of libstdc++ +, minimal ? false # Whether to create minimal shell without extra tools (faster when cross compiling) +}: + +let + lib = pkgs.lib; + llvm = crossPkgs.llvmPackages_20; + capnproto = crossPkgs.capnproto.override (lib.optionalAttrs enableLibcxx { clangStdenv = llvm.libcxxStdenv; }); + clang = if enableLibcxx then llvm.libcxxClang else llvm.clang; + clang-tools = llvm.clang-tools.override { inherit enableLibcxx; }; +in crossPkgs.mkShell { + buildInputs = [ + capnproto + ]; + nativeBuildInputs = with pkgs; [ + cmake + include-what-you-use + ninja + ] ++ lib.optionals (!minimal) [ + clang + clang-tools + ]; + + # Tell IWYU where its libc++ mapping lives + IWYU_MAPPING_FILE = if enableLibcxx then "${llvm.libcxx.dev}/include/c++/v1/libcxx.imp" else null; +} diff --git a/src/mp/gen.cpp b/src/mp/gen.cpp index 3d841a3f5e5..21a4d931399 100644 --- a/src/mp/gen.cpp +++ b/src/mp/gen.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -6,13 +6,15 @@ #include #include +#include #include +#include #include #include #include -#include #include #include +#include #include #include #include @@ -26,6 +28,7 @@ #include #include #include +#include #include #define PROXY_BIN "mpgen" @@ -76,7 +79,7 @@ static bool GetAnnotationInt32(const Reader& reader, uint64_t id, int32_t* resul return false; } -static void ForEachMethod(const capnp::InterfaceSchema& interface, const std::function& callback) +static void ForEachMethod(const capnp::InterfaceSchema& interface, const std::function& callback) // NOLINT(misc-no-recursion) { for (const auto super : interface.getSuperclasses()) { ForEachMethod(super, callback); @@ -198,19 +201,45 @@ static void Generate(kj::StringPtr src_prefix, std::ofstream cpp_server(output_path + ".proxy-server.c++"); cpp_server << "// Generated by " PROXY_BIN " from " << src_file << "\n\n"; + cpp_server << "// IWYU pragma: no_include \n"; + cpp_server << "// IWYU pragma: no_include \n"; + cpp_server << "// IWYU pragma: begin_keep\n"; + cpp_server << "#include <" << include_path << ".proxy.h>\n"; cpp_server << "#include <" << include_path << ".proxy-types.h>\n"; - cpp_server << "#include <" << PROXY_TYPES << ">\n\n"; + cpp_server << "#include \n"; + cpp_server << "#include \n"; + cpp_server << "#include \n"; + cpp_server << "#include \n"; + cpp_server << "#include \n"; + cpp_server << "#include \n"; + cpp_server << "#include \n"; + cpp_server << "#include <" << PROXY_TYPES << ">\n"; + cpp_server << "// IWYU pragma: end_keep\n\n"; cpp_server << "namespace mp {\n"; std::ofstream cpp_client(output_path + ".proxy-client.c++"); cpp_client << "// Generated by " PROXY_BIN " from " << src_file << "\n\n"; + cpp_client << "// IWYU pragma: no_include \n"; + cpp_client << "// IWYU pragma: no_include \n"; + cpp_client << "// IWYU pragma: begin_keep\n"; + cpp_client << "#include <" << include_path << ".h>\n"; + cpp_client << "#include <" << include_path << ".proxy.h>\n"; cpp_client << "#include <" << include_path << ".proxy-types.h>\n"; - cpp_client << "#include <" << PROXY_TYPES << ">\n\n"; + cpp_client << "#include \n"; + cpp_client << "#include \n"; + cpp_client << "#include \n"; + cpp_client << "#include \n"; + cpp_client << "#include \n"; + cpp_client << "#include <" << PROXY_TYPES << ">\n"; + cpp_client << "// IWYU pragma: end_keep\n\n"; cpp_client << "namespace mp {\n"; std::ofstream cpp_types(output_path + ".proxy-types.c++"); cpp_types << "// Generated by " PROXY_BIN " from " << src_file << "\n\n"; - cpp_types << "#include <" << include_path << ".proxy-types.h>\n"; + cpp_types << "// IWYU pragma: no_include \"mp/proxy.h\"\n"; + cpp_types << "// IWYU pragma: no_include \"mp/proxy-io.h\"\n"; + cpp_types << "#include <" << include_path << ".proxy.h>\n"; + cpp_types << "#include <" << include_path << ".proxy-types.h> // IWYU pragma: keep\n"; cpp_types << "#include <" << PROXY_TYPES << ">\n\n"; cpp_types << "namespace mp {\n"; @@ -226,10 +255,12 @@ static void Generate(kj::StringPtr src_prefix, inl << "// Generated by " PROXY_BIN " from " << src_file << "\n\n"; inl << "#ifndef " << guard << "_PROXY_TYPES_H\n"; inl << "#define " << guard << "_PROXY_TYPES_H\n\n"; - inl << "#include <" << include_path << ".proxy.h>\n"; + inl << "// IWYU pragma: no_include \"mp/proxy.h\"\n"; + inl << "#include // IWYU pragma: keep\n"; + inl << "#include <" << include_path << ".proxy.h> // IWYU pragma: keep\n"; for (const auto annotation : file_schema.getProto().getAnnotations()) { if (annotation.getId() == INCLUDE_TYPES_ANNOTATION_ID) { - inl << "#include <" << annotation.getValue().getText() << ">\n"; + inl << "#include \"" << annotation.getValue().getText() << "\" // IWYU pragma: export\n"; } } inl << "namespace mp {\n"; @@ -238,10 +269,10 @@ static void Generate(kj::StringPtr src_prefix, h << "// Generated by " PROXY_BIN " from " << src_file << "\n\n"; h << "#ifndef " << guard << "_PROXY_H\n"; h << "#define " << guard << "_PROXY_H\n\n"; - h << "#include <" << include_path << ".h>\n"; + h << "#include <" << include_path << ".h> // IWYU pragma: keep\n"; for (const auto annotation : file_schema.getProto().getAnnotations()) { if (annotation.getId() == INCLUDE_ANNOTATION_ID) { - h << "#include <" << annotation.getValue().getText() << ">\n"; + h << "#include \"" << annotation.getValue().getText() << "\" // IWYU pragma: export\n"; } } h << "#include <" << PROXY_DECL << ">\n\n"; diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp index b4255b60fea..c9fecf5cfb0 100644 --- a/src/mp/proxy.cpp +++ b/src/mp/proxy.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -10,23 +10,23 @@ #include #include -#include #include -#include #include +#include #include #include #include -#include #include +#include +#include #include #include -#include +#include #include #include #include #include -#include +#include #include #include #include @@ -37,9 +37,6 @@ namespace mp { -template -struct ProxyServer; - thread_local ThreadContext g_thread_context; void LoggingErrorHandler::taskFailed(kj::Exception&& exception) @@ -48,12 +45,49 @@ void LoggingErrorHandler::taskFailed(kj::Exception&& exception) m_loop.log() << "Uncaught exception in daemonized task."; } +EventLoopRef::EventLoopRef(EventLoop& loop, Lock* lock) : m_loop(&loop), m_lock(lock) +{ + auto loop_lock{PtrOrValue{m_lock, m_loop->m_mutex}}; + loop_lock->assert_locked(m_loop->m_mutex); + m_loop->m_num_clients += 1; +} + +// Due to the conditionals in this function, MP_NO_TSA is required to avoid +// error "error: mutex 'loop_lock' is not held on every path through here +// [-Wthread-safety-analysis]" +void EventLoopRef::reset(bool relock) MP_NO_TSA +{ + if (auto* loop{m_loop}) { + m_loop = nullptr; + auto loop_lock{PtrOrValue{m_lock, loop->m_mutex}}; + loop_lock->assert_locked(loop->m_mutex); + assert(loop->m_num_clients > 0); + loop->m_num_clients -= 1; + if (loop->done()) { + loop->m_cv.notify_all(); + int post_fd{loop->m_post_fd}; + loop_lock->unlock(); + char buffer = 0; + KJ_SYSCALL(write(post_fd, &buffer, 1)); // NOLINT(bugprone-suspicious-semicolon) + // By default, do not try to relock `loop_lock` after writing, + // because the event loop could wake up and destroy itself and the + // mutex might no longer exist. + if (relock) loop_lock->lock(); + } + } +} + +ProxyContext::ProxyContext(Connection* connection) : connection(connection), loop{*connection->m_loop} {} + Connection::~Connection() { - // Shut down RPC system first, since this will garbage collect Server - // objects that were not freed before the connection was closed, some of - // which may call addAsyncCleanup and add more cleanup callbacks which can - // run below. + // Shut down RPC system first, since this will garbage collect any + // ProxyServer objects that were not freed before the connection was closed. + // Typically all ProxyServer objects associated with this connection will be + // freed before this call returns. However that will not be the case if + // there are asynchronous IPC calls over this connection still currently + // executing. In that case, Cap'n Proto will destroy the ProxyServer objects + // after the calls finish. m_rpc_system.reset(); // ProxyClient cleanup handlers are in sync list, and ProxyServer cleanup @@ -98,23 +132,17 @@ Connection::~Connection() // on clean and unclean shutdowns. In unclean shutdown case when the // connection is broken, sync and async cleanup lists will filled with // callbacks. In the clean shutdown case both lists will be empty. + Lock lock{m_loop->m_mutex}; while (!m_sync_cleanup_fns.empty()) { - m_sync_cleanup_fns.front()(); - m_sync_cleanup_fns.pop_front(); + CleanupList fn; + fn.splice(fn.begin(), m_sync_cleanup_fns, m_sync_cleanup_fns.begin()); + Unlock(lock, fn.front()); } - while (!m_async_cleanup_fns.empty()) { - const std::unique_lock lock(m_loop.m_mutex); - m_loop.m_async_fns.emplace_back(std::move(m_async_cleanup_fns.front())); - m_async_cleanup_fns.pop_front(); - } - std::unique_lock lock(m_loop.m_mutex); - m_loop.startAsyncThread(lock); - m_loop.removeClient(lock); } CleanupIt Connection::addSyncCleanup(std::function fn) { - const std::unique_lock lock(m_loop.m_mutex); + const Lock lock(m_loop->m_mutex); // Add cleanup callbacks to the front of list, so sync cleanup functions run // in LIFO order. This is a good approach because sync cleanup functions are // added as client objects are created, and it is natural to clean up @@ -128,13 +156,13 @@ CleanupIt Connection::addSyncCleanup(std::function fn) void Connection::removeSyncCleanup(CleanupIt it) { - const std::unique_lock lock(m_loop.m_mutex); + const Lock lock(m_loop->m_mutex); m_sync_cleanup_fns.erase(it); } -void Connection::addAsyncCleanup(std::function fn) +void EventLoop::addAsyncCleanup(std::function fn) { - const std::unique_lock lock(m_loop.m_mutex); + const Lock lock(m_mutex); // Add async cleanup callbacks to the back of the list. Unlike the sync // cleanup list, this list order is more significant because it determines // the order server objects are destroyed when there is a sudden disconnect, @@ -151,7 +179,8 @@ void Connection::addAsyncCleanup(std::function fn) // process, otherwise shared pointer counts of the CWallet objects (which // inherit from Chain::Notification) will not be 1 when WalletLoader // destructor runs and it will wait forever for them to be released. - m_async_cleanup_fns.emplace(m_async_cleanup_fns.end(), std::move(fn)); + m_async_fns->emplace_back(std::move(fn)); + startAsyncThread(); } EventLoop::EventLoop(const char* exe_name, LogFn log_fn, void* context) @@ -170,9 +199,9 @@ EventLoop::EventLoop(const char* exe_name, LogFn log_fn, void* context) EventLoop::~EventLoop() { if (m_async_thread.joinable()) m_async_thread.join(); - const std::lock_guard lock(m_mutex); + const Lock lock(m_mutex); KJ_ASSERT(m_post_fn == nullptr); - KJ_ASSERT(m_async_fns.empty()); + KJ_ASSERT(!m_async_fns); KJ_ASSERT(m_wait_fd == -1); KJ_ASSERT(m_post_fd == -1); KJ_ASSERT(m_num_clients == 0); @@ -188,6 +217,12 @@ void EventLoop::loop() g_thread_context.loop_thread = true; KJ_DEFER(g_thread_context.loop_thread = false); + { + const Lock lock(m_mutex); + assert(!m_async_fns); + m_async_fns.emplace(); + } + kj::Own wait_stream{ m_io_context.lowLevelProvider->wrapSocketFd(m_wait_fd, kj::LowLevelAsyncIoProvider::TAKE_OWNERSHIP)}; int post_fd{m_post_fd}; @@ -195,14 +230,14 @@ void EventLoop::loop() for (;;) { const size_t read_bytes = wait_stream->read(&buffer, 0, 1).wait(m_io_context.waitScope); if (read_bytes != 1) throw std::logic_error("EventLoop wait_stream closed unexpectedly"); - std::unique_lock lock(m_mutex); + Lock lock(m_mutex); if (m_post_fn) { Unlock(lock, *m_post_fn); m_post_fn = nullptr; m_cv.notify_all(); - } else if (done(lock)) { + } else if (done()) { // Intentionally do not break if m_post_fn was set, even if done() - // would return true, to ensure that the removeClient write(post_fd) + // would return true, to ensure that the EventLoopRef write(post_fd) // call always succeeds and the loop does not exit between the time // that the done condition is set and the write call is made. break; @@ -213,76 +248,61 @@ void EventLoop::loop() log() << "EventLoop::loop bye."; wait_stream = nullptr; KJ_SYSCALL(::close(post_fd)); - const std::unique_lock lock(m_mutex); + const Lock lock(m_mutex); m_wait_fd = -1; m_post_fd = -1; + m_async_fns.reset(); + m_cv.notify_all(); } -void EventLoop::post(const std::function& fn) +void EventLoop::post(kj::Function fn) { if (std::this_thread::get_id() == m_thread_id) { fn(); return; } - std::unique_lock lock(m_mutex); - addClient(lock); - m_cv.wait(lock, [this] { return m_post_fn == nullptr; }); + Lock lock(m_mutex); + EventLoopRef ref(*this, &lock); + m_cv.wait(lock.m_lock, [this]() MP_REQUIRES(m_mutex) { return m_post_fn == nullptr; }); m_post_fn = &fn; int post_fd{m_post_fd}; Unlock(lock, [&] { char buffer = 0; KJ_SYSCALL(write(post_fd, &buffer, 1)); }); - m_cv.wait(lock, [this, &fn] { return m_post_fn != &fn; }); - removeClient(lock); + m_cv.wait(lock.m_lock, [this, &fn]() MP_REQUIRES(m_mutex) { return m_post_fn != &fn; }); } -void EventLoop::addClient(std::unique_lock& lock) { m_num_clients += 1; } - -bool EventLoop::removeClient(std::unique_lock& lock) -{ - m_num_clients -= 1; - if (done(lock)) { - m_cv.notify_all(); - int post_fd{m_post_fd}; - lock.unlock(); - char buffer = 0; - KJ_SYSCALL(write(post_fd, &buffer, 1)); // NOLINT(bugprone-suspicious-semicolon) - return true; - } - return false; -} - -void EventLoop::startAsyncThread(std::unique_lock& lock) +void EventLoop::startAsyncThread() { + assert (std::this_thread::get_id() == m_thread_id); if (m_async_thread.joinable()) { + // Notify to wake up the async thread if it is already running. m_cv.notify_all(); - } else if (!m_async_fns.empty()) { + } else if (!m_async_fns->empty()) { m_async_thread = std::thread([this] { - std::unique_lock lock(m_mutex); - while (true) { - if (!m_async_fns.empty()) { - addClient(lock); - const std::function fn = std::move(m_async_fns.front()); - m_async_fns.pop_front(); + Lock lock(m_mutex); + while (m_async_fns) { + if (!m_async_fns->empty()) { + EventLoopRef ref{*this, &lock}; + const std::function fn = std::move(m_async_fns->front()); + m_async_fns->pop_front(); Unlock(lock, fn); - if (removeClient(lock)) break; + // Important to relock because of the wait() call below. + ref.reset(/*relock=*/true); + // Continue without waiting in case there are more async_fns continue; - } else if (m_num_clients == 0) { - break; } - m_cv.wait(lock); + m_cv.wait(lock.m_lock); } }); } } -bool EventLoop::done(std::unique_lock& lock) const +bool EventLoop::done() const { assert(m_num_clients >= 0); - assert(lock.owns_lock()); - assert(lock.mutex() == &m_mutex); - return m_num_clients == 0 && m_async_fns.empty(); + return m_num_clients == 0 && m_async_fns->empty(); } std::tuple SetThread(ConnThreads& threads, std::mutex& mutex, Connection* connection, const std::function& make_thread) @@ -293,18 +313,18 @@ std::tuple SetThread(ConnThreads& threads, std::mutex& mutex, thread = threads.emplace( std::piecewise_construct, std::forward_as_tuple(connection), std::forward_as_tuple(make_thread(), connection, /* destroy_connection= */ false)).first; - thread->second.setCleanup([&threads, &mutex, thread] { + thread->second.setDisconnectCallback([&threads, &mutex, thread] { // Note: it is safe to use the `thread` iterator in this cleanup // function, because the iterator would only be invalid if the map entry // was removed, and if the map entry is removed the ProxyClient // destructor unregisters the cleanup. // Connection is being destroyed before thread client is, so reset - // thread client m_cleanup_it member so thread client destructor does not - // try unregister this callback after connection is destroyed. - thread->second.m_cleanup_it.reset(); + // thread client m_disconnect_cb member so thread client destructor does not + // try to unregister this callback after connection is destroyed. // Remove connection pointer about to be destroyed from the map const std::unique_lock lock(mutex); + thread->second.m_disconnect_cb.reset(); threads.erase(thread); }); return {thread, true}; @@ -315,16 +335,16 @@ ProxyClient::~ProxyClient() // 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); + if (m_disconnect_cb) { + m_context.connection->removeSyncCleanup(*m_disconnect_cb); } } -void ProxyClient::setCleanup(const std::function& fn) +void ProxyClient::setDisconnectCallback(const std::function& fn) { assert(fn); - assert(!m_cleanup_it); - m_cleanup_it = m_context.connection->addSyncCleanup(fn); + assert(!m_disconnect_cb); + m_disconnect_cb = m_context.connection->addSyncCleanup(fn); } ProxyServer::ProxyServer(ThreadContext& thread_context, std::thread&& thread) @@ -375,7 +395,7 @@ kj::Promise ProxyServer::makeThread(MakeThreadContext context) const std::string from = context.getParams().getName(); std::promise thread_context; std::thread thread([&thread_context, from, this]() { - g_thread_context.thread_name = ThreadName(m_connection.m_loop.m_exe_name) + " (from " + from + ")"; + g_thread_context.thread_name = ThreadName(m_connection.m_loop->m_exe_name) + " (from " + from + ")"; g_thread_context.waiter = std::make_unique(); thread_context.set_value(&g_thread_context); std::unique_lock lock(g_thread_context.waiter->m_mutex); diff --git a/src/mp/util.cpp b/src/mp/util.cpp index 309bb922352..a9485399a2a 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -1,16 +1,16 @@ -// Copyright (c) 2018-2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // 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 diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 997a0289b4c..2a1a7e9e7c2 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,4 +1,4 @@ -# Copyright (c) 2020 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -13,7 +13,7 @@ add_custom_target(mptests) add_custom_target(mpcheck COMMAND ${CMAKE_CTEST_COMMAND} DEPENDS mptests) # Only add more convenient tests and check targets if project is being built -# standlone, to prevent clashes with external projects. +# standalone, to prevent clashes with external projects. if (MP_STANDALONE) add_custom_target(tests DEPENDS mptests) add_custom_target(check DEPENDS mpcheck) diff --git a/test/mp/test/foo-types.h b/test/mp/test/foo-types.h index 246b1948058..e70bc4c100b 100644 --- a/test/mp/test/foo-types.h +++ b/test/mp/test/foo-types.h @@ -1,13 +1,20 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #ifndef MP_TEST_FOO_TYPES_H #define MP_TEST_FOO_TYPES_H +#include #include + +// IWYU pragma: begin_exports +#include +#include +#include #include #include +#include #include #include #include @@ -17,9 +24,18 @@ #include #include #include +#include +#include +// IWYU pragma: end_exports namespace mp { namespace test { +namespace messages { +struct ExtendedCallback; // IWYU pragma: export +struct FooCallback; // IWYU pragma: export +struct FooFn; // IWYU pragma: export +struct FooInterface; // IWYU pragma: export +} // namespace messages template void CustomBuildField(TypeList, Priority<1>, InvokeContext& invoke_context, const FooCustom& value, Output&& output) diff --git a/test/mp/test/foo.capnp b/test/mp/test/foo.capnp index df0d436114d..75e4617da84 100644 --- a/test/mp/test/foo.capnp +++ b/test/mp/test/foo.capnp @@ -1,4 +1,4 @@ -# Copyright (c) 2019 The Bitcoin Core developers +# Copyright (c) The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -28,6 +28,9 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") { passMessage @13 (arg :FooMessage) -> (result :FooMessage); passMutable @14 (arg :FooMutable) -> (arg :FooMutable); passEnum @15 (arg :Int32) -> (result :Int32); + passFn @16 (context :Proxy.Context, fn :FooFn) -> (result :Int32); + callFn @17 () -> (); + callFnAsync @18 (context :Proxy.Context) -> (); } interface FooCallback $Proxy.wrap("mp::test::FooCallback") { @@ -39,6 +42,11 @@ interface ExtendedCallback extends(FooCallback) $Proxy.wrap("mp::test::ExtendedC callExtended @0 (context :Proxy.Context, arg :Int32) -> (result :Int32); } +interface FooFn $Proxy.wrap("ProxyCallback>") { + destroy @0 (context :Proxy.Context) -> (); + call @1 (context :Proxy.Context) -> (result :Int32); +} + struct FooStruct $Proxy.wrap("mp::test::FooStruct") { name @0 :Text; setint @1 :List(Int32); diff --git a/test/mp/test/foo.h b/test/mp/test/foo.h index 1c5ee79f211..70bf4ff171b 100644 --- a/test/mp/test/foo.h +++ b/test/mp/test/foo.h @@ -1,10 +1,12 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #ifndef MP_TEST_FOO_H #define MP_TEST_FOO_H +#include +#include #include #include #include @@ -75,7 +77,11 @@ public: FooMessage passMessage(FooMessage foo) { foo.message += " call"; return foo; } void passMutable(FooMutable& foo) { foo.message += " call"; } FooEnum passEnum(FooEnum foo) { return foo; } + int passFn(std::function fn) { return fn(); } std::shared_ptr m_callback; + void callFn() { assert(m_fn); m_fn(); } + void callFnAsync() { assert(m_fn); m_fn(); } + std::function m_fn; }; } // namespace test diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp index 7fc64f6741d..225d00d5b17 100644 --- a/test/mp/test/test.cpp +++ b/test/mp/test/test.cpp @@ -1,54 +1,117 @@ -// Copyright (c) 2019 The Bitcoin Core developers +// Copyright (c) The Bitcoin Core developers // 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 #include #include +#include +#include +#include +#include +#include +#include #include +#include #include #include +#include namespace mp { namespace test { +/** + * Test setup class creating a two way connection between a + * ProxyServer object and a ProxyClient. + * + * Provides client_disconnect and server_disconnect lambdas that can be used to + * trigger disconnects and test handling of broken and closed connections. + * + * Accepts a client_owns_connection option to test different ProxyClient + * destroy_connection values and control whether destroying the ProxyClient + * object destroys the client Connection object. Normally it makes sense for + * this to be true to simplify shutdown and avoid needing to call + * client_disconnect manually, but false allows testing more ProxyClient + * behavior and the "IPC client method called after disconnect" code path. + */ +class TestSetup +{ +public: + std::function server_disconnect; + std::function client_disconnect; + std::promise>> client_promise; + std::unique_ptr> client; + ProxyServer* server{nullptr}; + //! Thread variable should be after other struct members so the thread does + //! not start until the other members are initialized. + std::thread thread; + + TestSetup(bool client_owns_connection = true) + : thread{[&] { + EventLoop loop("mptest", [](bool raise, const std::string& log) { + std::cout << "LOG" << raise << ": " << log << "\n"; + if (raise) throw std::runtime_error(log); + }); + auto pipe = loop.m_io_context.provider->newTwoWayPipe(); + + auto server_connection = + std::make_unique(loop, kj::mv(pipe.ends[0]), [&](Connection& connection) { + auto server_proxy = kj::heap>( + std::make_shared(), connection); + server = server_proxy; + return capnp::Capability::Client(kj::mv(server_proxy)); + }); + server_disconnect = [&] { loop.sync([&] { server_connection.reset(); }); }; + // Set handler to destroy the server when the client disconnects. This + // is ignored if server_disconnect() is called instead. + server_connection->onDisconnect([&] { server_connection.reset(); }); + + auto client_connection = std::make_unique(loop, kj::mv(pipe.ends[1])); + auto client_proxy = std::make_unique>( + client_connection->m_rpc_system->bootstrap(ServerVatId().vat_id).castAs(), + client_connection.get(), /* destroy_connection= */ client_owns_connection); + if (client_owns_connection) { + client_connection.release(); + } else { + client_disconnect = [&] { loop.sync([&] { client_connection.reset(); }); }; + } + + client_promise.set_value(std::move(client_proxy)); + loop.loop(); + }} + { + client = client_promise.get_future().get(); + } + + ~TestSetup() + { + // Test that client cleanup_fns are executed. + bool destroyed = false; + client->m_context.cleanup_fns.emplace_front([&destroyed] { destroyed = true; }); + client.reset(); + KJ_EXPECT(destroyed); + + thread.join(); + } +}; + KJ_TEST("Call FooInterface methods") { - std::promise>> foo_promise; - std::function disconnect_client; - std::thread thread([&]() { - EventLoop loop("mptest", [](bool raise, const std::string& log) { - std::cout << "LOG" << raise << ": " << log << "\n"; - }); - auto pipe = loop.m_io_context.provider->newTwoWayPipe(); + TestSetup setup; + ProxyClient* foo = setup.client.get(); - auto connection_client = std::make_unique(loop, kj::mv(pipe.ends[0])); - auto foo_client = std::make_unique>( - connection_client->m_rpc_system->bootstrap(ServerVatId().vat_id).castAs(), - connection_client.get(), /* destroy_connection= */ false); - foo_promise.set_value(std::move(foo_client)); - disconnect_client = [&] { loop.sync([&] { connection_client.reset(); }); }; - - auto connection_server = std::make_unique(loop, kj::mv(pipe.ends[1]), [&](Connection& connection) { - auto foo_server = kj::heap>(std::make_shared(), connection); - return capnp::Capability::Client(kj::mv(foo_server)); - }); - connection_server->onDisconnect([&] { connection_server.reset(); }); - loop.loop(); - }); - - auto foo = foo_promise.get_future().get(); KJ_EXPECT(foo->add(1, 2) == 3); FooStruct in; @@ -128,13 +191,110 @@ KJ_TEST("Call FooInterface methods") foo->passMutable(mut); KJ_EXPECT(mut.message == "init build pass call return read"); - disconnect_client(); - thread.join(); + KJ_EXPECT(foo->passFn([]{ return 10; }) == 10); +} - bool destroyed = false; - foo->m_context.cleanup_fns.emplace_front([&destroyed]{ destroyed = true; }); - foo.reset(); - KJ_EXPECT(destroyed); +KJ_TEST("Call IPC method after client connection is closed") +{ + TestSetup setup{/*client_owns_connection=*/false}; + ProxyClient* foo = setup.client.get(); + KJ_EXPECT(foo->add(1, 2) == 3); + setup.client_disconnect(); + + bool disconnected{false}; + try { + foo->add(1, 2); + } catch (const std::runtime_error& e) { + KJ_EXPECT(std::string_view{e.what()} == "IPC client method called after disconnect."); + disconnected = true; + } + KJ_EXPECT(disconnected); +} + +KJ_TEST("Calling IPC method after server connection is closed") +{ + TestSetup setup; + ProxyClient* foo = setup.client.get(); + KJ_EXPECT(foo->add(1, 2) == 3); + setup.server_disconnect(); + + bool disconnected{false}; + try { + foo->add(1, 2); + } catch (const std::runtime_error& e) { + KJ_EXPECT(std::string_view{e.what()} == "IPC client method call interrupted by disconnect."); + disconnected = true; + } + KJ_EXPECT(disconnected); +} + +KJ_TEST("Calling IPC method and disconnecting during the call") +{ + TestSetup setup{/*client_owns_connection=*/false}; + ProxyClient* foo = setup.client.get(); + KJ_EXPECT(foo->add(1, 2) == 3); + + // Set m_fn to initiate client disconnect when server is in the middle of + // handling the callFn call to make sure this case is handled cleanly. + setup.server->m_impl->m_fn = setup.client_disconnect; + + bool disconnected{false}; + try { + foo->callFn(); + } catch (const std::runtime_error& e) { + KJ_EXPECT(std::string_view{e.what()} == "IPC client method call interrupted by disconnect."); + disconnected = true; + } + KJ_EXPECT(disconnected); +} + +KJ_TEST("Calling IPC method, disconnecting and blocking during the call") +{ + // This test is similar to last test, except that instead of letting the IPC + // call return immediately after triggering a disconnect, make it disconnect + // & wait so server is forced to deal with having a disconnection and call + // in flight at the same time. + // + // Test uses callFnAsync() instead of callFn() to implement this. Both of + // these methods have the same implementation, but the callFnAsync() capnp + // method declaration takes an mp.Context argument so the method executes on + // an asynchronous thread instead of executing in the event loop thread, so + // it is able to block without deadlocking the event lock thread. + // + // This test adds important coverage because it causes the server Connection + // object to be destroyed before ProxyServer object, which is not a + // condition that usually happens because the m_rpc_system.reset() call in + // the ~Connection destructor usually would immediately free all remaining + // ProxyServer objects associated with the connection. Having an in-progress + // RPC call requires keeping the ProxyServer longer. + + std::promise signal; + TestSetup setup{/*client_owns_connection=*/false}; + ProxyClient* foo = setup.client.get(); + KJ_EXPECT(foo->add(1, 2) == 3); + + foo->initThreadMap(); + setup.server->m_impl->m_fn = [&] { + EventLoopRef loop{*setup.server->m_context.loop}; + setup.client_disconnect(); + signal.get_future().get(); + }; + + bool disconnected{false}; + try { + foo->callFnAsync(); + } catch (const std::runtime_error& e) { + KJ_EXPECT(std::string_view{e.what()} == "IPC client method call interrupted by disconnect."); + disconnected = true; + } + KJ_EXPECT(disconnected); + + // Now that the disconnect has been detected, set signal allowing the + // callFnAsync() IPC call to return. Since signalling may not wake up the + // thread right away, it is important for the signal variable to be declared + // *before* the TestSetup variable so is not destroyed while + // signal.get_future().get() is called. + signal.set_value(); } } // namespace test From 9a9fb19536fa2f89c3c96860c1882b79b68c9e64 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Sun, 9 Feb 2025 14:21:37 -0500 Subject: [PATCH 2/7] ipc: Use EventLoopRef instead of addClient/removeClient Use EventLoopRef to avoid reference counting bugs and be more exception safe and deal with removal of addClient/removeClient methods in https://github.com/bitcoin-core/libmultiprocess/pull/160 A test update is also required due to https://github.com/bitcoin-core/libmultiprocess/pull/160 to deal with changed reference count semantics. In IpcPipeTest(), it is now necessary to destroy the client Proxy object instead of just the client Connection object to decrease the event loop reference count and allow the loop to exit so the test does not hang on shutdown. --- src/ipc/capnp/protocol.cpp | 15 +++++++-------- src/test/ipc_test.cpp | 9 ++++----- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index 3326f709314..7bc653d25ce 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -41,10 +41,7 @@ class CapnpProtocol : public Protocol public: ~CapnpProtocol() noexcept(true) { - if (m_loop) { - std::unique_lock lock(m_loop->m_mutex); - m_loop->removeClient(lock); - } + m_loop_ref.reset(); if (m_loop_thread.joinable()) m_loop_thread.join(); assert(!m_loop); }; @@ -83,10 +80,7 @@ public: m_loop_thread = std::thread([&] { util::ThreadRename("capnp-loop"); m_loop.emplace(exe_name, &IpcLogFn, &m_context); - { - std::unique_lock lock(m_loop->m_mutex); - m_loop->addClient(lock); - } + m_loop_ref.emplace(*m_loop); promise.set_value(); m_loop->loop(); m_loop.reset(); @@ -95,7 +89,12 @@ public: } Context m_context; std::thread m_loop_thread; + //! EventLoop object which manages I/O events for all connections. std::optional m_loop; + //! Reference to the same EventLoop. Increments the loop’s refcount on + //! creation, decrements on destruction. The loop thread exits when the + //! refcount reaches 0. Other IPC objects also hold their own EventLoopRef. + std::optional m_loop_ref; }; } // namespace diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp index b4d7ad354cc..eeaf348812a 100644 --- a/src/test/ipc_test.cpp +++ b/src/test/ipc_test.cpp @@ -55,7 +55,6 @@ void IpcPipeTest() { // Setup: create FooImplementation object and listen for FooInterface requests std::promise>> foo_promise; - std::function disconnect_client; std::thread thread([&]() { 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(); @@ -63,9 +62,9 @@ void IpcPipeTest() auto connection_client = std::make_unique(loop, kj::mv(pipe.ends[0])); auto foo_client = std::make_unique>( connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs(), - connection_client.get(), /* destroy_connection= */ false); + connection_client.get(), /* destroy_connection= */ true); + connection_client.release(); foo_promise.set_value(std::move(foo_client)); - disconnect_client = [&] { loop.sync([&] { connection_client.reset(); }); }; auto connection_server = std::make_unique(loop, kj::mv(pipe.ends[1]), [&](mp::Connection& connection) { auto foo_server = kj::heap>(std::make_shared(), connection); @@ -106,8 +105,8 @@ void IpcPipeTest() auto script2{foo->passScript(script1)}; BOOST_CHECK_EQUAL(HexStr(script1), HexStr(script2)); - // Test cleanup: disconnect pipe and join thread - disconnect_client(); + // Test cleanup: disconnect and join thread + foo.reset(); thread.join(); } From 6eb09fd6141f4c96dae3e1fe1a1f1946c91d0131 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 18 Apr 2025 18:12:46 -0400 Subject: [PATCH 3/7] test: Add unit test coverage for Init and Shutdown code Currently this code is not called in unit tests. Calling should make it possible to write tests for things like IPC exceptions being thrown during shutdown. --- src/common/args.cpp | 8 ++++++ src/common/args.h | 6 +--- src/net.cpp | 6 ++++ src/net.h | 1 + src/netbase.h | 31 ++++++++++++++------- src/rpc/server.cpp | 6 ++++ src/rpc/server.h | 1 + src/test/CMakeLists.txt | 1 + src/test/node_init_tests.cpp | 51 ++++++++++++++++++++++++++++++++++ src/test/util/setup_common.cpp | 11 ++++++++ 10 files changed, 107 insertions(+), 15 deletions(-) create mode 100644 src/test/node_init_tests.cpp diff --git a/src/common/args.cpp b/src/common/args.cpp index 71dcd1ac10c..6a79b6c6f17 100644 --- a/src/common/args.cpp +++ b/src/common/args.cpp @@ -589,6 +589,14 @@ void ArgsManager::AddHiddenArgs(const std::vector& names) } } +void ArgsManager::ClearArgs() +{ + LOCK(cs_args); + m_settings = {}; + m_available_args.clear(); + m_network_only_args.clear(); +} + void ArgsManager::CheckMultipleCLIArgs() const { LOCK(cs_args); diff --git a/src/common/args.h b/src/common/args.h index 6c5ac48ae3e..da19cbda66f 100644 --- a/src/common/args.h +++ b/src/common/args.h @@ -359,11 +359,7 @@ protected: /** * Clear available arguments */ - void ClearArgs() { - LOCK(cs_args); - m_available_args.clear(); - m_network_only_args.clear(); - } + void ClearArgs(); /** * Check CLI command args diff --git a/src/net.cpp b/src/net.cpp index 217d9a89037..77ea23e6657 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -266,6 +266,12 @@ std::optional GetLocalAddrForPeer(CNode& node) return std::nullopt; } +void ClearLocal() +{ + LOCK(g_maplocalhost_mutex); + return mapLocalHost.clear(); +} + // learn a new local address bool AddLocal(const CService& addr_, int nScore) { diff --git a/src/net.h b/src/net.h index 4cb4cb906e2..06bd7b1e875 100644 --- a/src/net.h +++ b/src/net.h @@ -158,6 +158,7 @@ enum /** Returns a local address that we should advertise to this peer. */ std::optional GetLocalAddrForPeer(CNode& node); +void ClearLocal(); bool AddLocal(const CService& addr, int nScore = LOCAL_NONE); bool AddLocal(const CNetAddr& addr, int nScore = LOCAL_NONE); void RemoveLocal(const CService& addr); diff --git a/src/netbase.h b/src/netbase.h index b2cc172e536..41b3ca8fdb0 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -121,6 +121,13 @@ public: m_reachable.clear(); } + void Reset() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + { + AssertLockNotHeld(m_mutex); + LOCK(m_mutex); + m_reachable = DefaultNets(); + } + [[nodiscard]] bool Contains(Network net) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { AssertLockNotHeld(m_mutex); @@ -142,17 +149,21 @@ public: } private: - mutable Mutex m_mutex; - - std::unordered_set m_reachable GUARDED_BY(m_mutex){ - NET_UNROUTABLE, - NET_IPV4, - NET_IPV6, - NET_ONION, - NET_I2P, - NET_CJDNS, - NET_INTERNAL + static std::unordered_set DefaultNets() + { + return { + NET_UNROUTABLE, + NET_IPV4, + NET_IPV6, + NET_ONION, + NET_I2P, + NET_CJDNS, + NET_INTERNAL + }; }; + + mutable Mutex m_mutex; + std::unordered_set m_reachable GUARDED_BY(m_mutex){DefaultNets()}; }; extern ReachableNets g_reachable_nets; diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 8631ae0adfe..722577cc218 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -327,6 +327,12 @@ void SetRPCWarmupStatus(const std::string& newStatus) rpcWarmupStatus = newStatus; } +void SetRPCWarmupStarting() +{ + LOCK(g_rpc_warmup_mutex); + fRPCInWarmup = true; +} + void SetRPCWarmupFinished() { LOCK(g_rpc_warmup_mutex); diff --git a/src/rpc/server.h b/src/rpc/server.h index d4b48f2418f..19bd54dfc4f 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -29,6 +29,7 @@ void RpcInterruptionPoint(); * immediately with RPC_IN_WARMUP. */ void SetRPCWarmupStatus(const std::string& newStatus); +void SetRPCWarmupStarting(); /* Mark warmup as done. RPC calls will be processed from now on. */ void SetRPCWarmupFinished(); diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index 6ce33621af8..e891566bc1f 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -64,6 +64,7 @@ add_executable(test_bitcoin net_peer_eviction_tests.cpp net_tests.cpp netbase_tests.cpp + node_init_tests.cpp node_warnings_tests.cpp orphanage_tests.cpp pcp_tests.cpp diff --git a/src/test/node_init_tests.cpp b/src/test/node_init_tests.cpp new file mode 100644 index 00000000000..802e3176099 --- /dev/null +++ b/src/test/node_init_tests.cpp @@ -0,0 +1,51 @@ +// Copyright (c) 2025 The Bitcoin Core developers +// 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 + +using node::NodeContext; + +BOOST_FIXTURE_TEST_SUITE(node_init_tests, BasicTestingSetup) + +//! Custom implementation of interfaces::Init for testing. +class TestInit : public interfaces::Init +{ +public: + TestInit(NodeContext& node) : m_node(node) + { + InitContext(m_node); + m_node.init = this; + } + std::unique_ptr makeChain() override { return interfaces::MakeChain(m_node); } + std::unique_ptr makeWalletLoader(interfaces::Chain& chain) override + { + return MakeWalletLoader(chain, *Assert(m_node.args)); + } + NodeContext& m_node; +}; + +BOOST_AUTO_TEST_CASE(init_test) +{ + // Clear state set by BasicTestingSetup that AppInitMain assumes is unset. + LogInstance().DisconnectTestLogger(); + m_node.args->SetConfigFilePath({}); + + // Prevent the test from trying to listen on ports 8332 and 8333. + m_node.args->ForceSetArg("-server", "0"); + m_node.args->ForceSetArg("-listen", "0"); + + // Run through initialization and shutdown code. + TestInit init{m_node}; + BOOST_CHECK(AppInitInterfaces(m_node)); + BOOST_CHECK(AppInitMain(m_node)); + Interrupt(m_node); + Shutdown(m_node); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 76a42d19ea2..f3ccab0580e 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -115,6 +115,14 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts) if (!EnableFuzzDeterminism()) { SeedRandomForTest(SeedRand::FIXED_SEED); } + + // Reset globals + fDiscover = true; + fListen = true; + SetRPCWarmupStarting(); + g_reachable_nets.Reset(); + ClearLocal(); + m_node.shutdown_signal = &m_interrupt; m_node.shutdown_request = [this]{ return m_interrupt(); }; m_node.args = &gArgs; @@ -214,7 +222,10 @@ BasicTestingSetup::~BasicTestingSetup() } else { fs::remove_all(m_path_root); } + // Clear all arguments except for -datadir, which GUI tests currently rely + // on to be set even after the testing setup is destroyed. gArgs.ClearArgs(); + gArgs.ForceSetArg("-datadir", fs::PathToString(m_path_root)); } ChainTestingSetup::ChainTestingSetup(const ChainType chainType, TestOpts opts) From 7f65aac78b95357e00e1c0cd996f05e944ea9d2e Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 24 Apr 2025 15:02:19 -0400 Subject: [PATCH 4/7] ipc: Avoid waiting for clients to disconnect when shutting down This fixes behavior reported by Antoine Poinsot https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2546088852 where if an IPC client is connected, the node will wait forever for it to disconnect before exiting. --- src/init.cpp | 6 ++++++ src/interfaces/ipc.h | 3 +++ src/ipc/capnp/protocol.cpp | 13 +++++++++++++ src/ipc/interfaces.cpp | 4 ++++ src/ipc/protocol.h | 3 +++ 5 files changed, 29 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index 1f72c22ec2f..cae52675525 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -374,6 +374,12 @@ void Shutdown(NodeContext& node) client->stop(); } + // If any -ipcbind clients are still connected, disconnect them now so they + // do not block shutdown. + if (interfaces::Ipc* ipc = node.init->ipc()) { + ipc->disconnectIncoming(); + } + #ifdef ENABLE_ZMQ if (g_zmq_notification_interface) { if (node.validation_signals) node.validation_signals->UnregisterValidationInterface(g_zmq_notification_interface.get()); diff --git a/src/interfaces/ipc.h b/src/interfaces/ipc.h index fb340552c5c..15e92d1050d 100644 --- a/src/interfaces/ipc.h +++ b/src/interfaces/ipc.h @@ -70,6 +70,9 @@ public: //! using provided callback. Throws an exception if there was an error. virtual void listenAddress(std::string& address) = 0; + //! Disconnect any incoming connections that are still connected. + virtual void disconnectIncoming() = 0; + //! Add cleanup callback to remote interface that will run when the //! interface is deleted. template diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index 7bc653d25ce..4150f9f4664 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -65,9 +65,20 @@ public: m_loop.emplace(exe_name, &IpcLogFn, &m_context); if (ready_fn) ready_fn(); mp::ServeStream(*m_loop, fd, init); + m_parent_connection = &m_loop->m_incoming_connections.back(); m_loop->loop(); m_loop.reset(); } + void disconnectIncoming() override + { + if (!m_loop) return; + // Delete incoming connections, except the connection to a parent + // process (if there is one), since a parent process should be able to + // monitor and control this process, even during shutdown. + m_loop->sync([&] { + m_loop->m_incoming_connections.remove_if([this](mp::Connection& c) { return &c != m_parent_connection; }); + }); + } void addCleanup(std::type_index type, void* iface, std::function cleanup) override { mp::ProxyTypeRegister::types().at(type)(iface).cleanup_fns.emplace_back(std::move(cleanup)); @@ -95,6 +106,8 @@ public: //! creation, decrements on destruction. The loop thread exits when the //! refcount reaches 0. Other IPC objects also hold their own EventLoopRef. std::optional m_loop_ref; + //! Connection to parent, if this is a child process spawned by a parent process. + mp::Connection* m_parent_connection{nullptr}; }; } // namespace diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 1d9c3992607..50cef794aa5 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -86,6 +86,10 @@ public: int fd = m_process->bind(gArgs.GetDataDirNet(), m_exe_name, address); m_protocol->listen(fd, m_exe_name, m_init); } + void disconnectIncoming() override + { + m_protocol->disconnectIncoming(); + } void addCleanup(std::type_index type, void* iface, std::function cleanup) override { m_protocol->addCleanup(type, iface, std::move(cleanup)); diff --git a/src/ipc/protocol.h b/src/ipc/protocol.h index cb964d802fb..335ffddc0b1 100644 --- a/src/ipc/protocol.h +++ b/src/ipc/protocol.h @@ -58,6 +58,9 @@ public: //! clients and servers independently. virtual void serve(int fd, const char* exe_name, interfaces::Init& init, const std::function& ready_fn = {}) = 0; + //! Disconnect any incoming connections that are still connected. + virtual void disconnectIncoming() = 0; + //! Add cleanup callback to interface that will run when the interface is //! deleted. virtual void addCleanup(std::type_index type, void* iface, std::function cleanup) = 0; From 0c28068ceb7b95885a5abb2685a89bb7c03c1689 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 24 Apr 2025 15:13:05 -0400 Subject: [PATCH 5/7] doc: Improve IPC interface comments Fix some comments that were referring to previous versions of these methods and did not make sense. --- src/interfaces/ipc.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/interfaces/ipc.h b/src/interfaces/ipc.h index 15e92d1050d..8f441118ead 100644 --- a/src/interfaces/ipc.h +++ b/src/interfaces/ipc.h @@ -59,15 +59,15 @@ public: //! true. If this is not a spawned child process, return false. virtual bool startSpawnedProcess(int argc, char* argv[], int& exit_status) = 0; - //! Connect to a socket address and make a client interface proxy object - //! using provided callback. connectAddress returns an interface pointer if - //! the connection was established, returns null if address is empty ("") or - //! disabled ("0") or if a connection was refused but not required ("auto"), - //! and throws an exception if there was an unexpected error. + //! Connect to a socket address and return a pointer to its Init interface. + //! Returns a non-null pointer if the connection was established, returns + //! null if address is empty ("") or disabled ("0") or if a connection was + //! refused but not required ("auto"), and throws an exception if there was + //! an unexpected error. virtual std::unique_ptr connectAddress(std::string& address) = 0; - //! Connect to a socket address and make a client interface proxy object - //! using provided callback. Throws an exception if there was an error. + //! Listen on a socket address exposing this process's init interface to + //! clients. Throws an exception if there was an error. virtual void listenAddress(std::string& address) = 0; //! Disconnect any incoming connections that are still connected. From 216099591632dc8a57cc1a3b1ad08e909f8c73cc Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 24 Apr 2025 15:15:08 -0400 Subject: [PATCH 6/7] ipc: Add Ctrl-C handler for spawned subprocesses This fixes an error reported by Antoine Poinsot in https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102 applied, where if Ctrl-C is pressed when `bitcoin-node` is started, it is handled by both `bitcoin-node` and `bitcoin-wallet` processes, causing the wallet to shutdown abruptly instead of waiting for the node and shutting down cleanly. This change fixes the problem by having the wallet process print to stdout when it receives a Ctrl-C signal but not otherwise react, letting the node shut everything down cleanly. --- src/ipc/interfaces.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/ipc/interfaces.cpp b/src/ipc/interfaces.cpp index 50cef794aa5..d6b078e61b0 100644 --- a/src/ipc/interfaces.cpp +++ b/src/ipc/interfaces.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -26,6 +27,28 @@ namespace ipc { namespace { +#ifndef WIN32 +std::string g_ignore_ctrl_c; + +void HandleCtrlC(int) +{ + // (void)! needed to suppress -Wunused-result warning from GCC + (void)!write(STDOUT_FILENO, g_ignore_ctrl_c.data(), g_ignore_ctrl_c.size()); +} +#endif + +void IgnoreCtrlC(std::string message) +{ +#ifndef WIN32 + g_ignore_ctrl_c = std::move(message); + struct sigaction sa{}; + sa.sa_handler = HandleCtrlC; + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESTART; + sigaction(SIGINT, &sa, nullptr); +#endif +} + class IpcImpl : public interfaces::Ipc { public: @@ -53,6 +76,7 @@ public: if (!m_process->checkSpawned(argc, argv, fd)) { 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); exit_status = EXIT_SUCCESS; return true; From 2581258ec200efb173ea6449ad09b2e7f1cc02e0 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 24 Apr 2025 15:20:58 -0400 Subject: [PATCH 7/7] ipc: Handle bitcoin-wallet disconnections This fixes an error reported by Antoine Poinsot in https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102 applied, where if the child bitcoin-wallet process is killed (either by an external signal or by Ctrl-C as reported in the issue) the bitcoin-node process will not shutdown cleanly after that because chain client stop() calls will fail. This change fixes the problem by handling ipc::Exception errors thrown during the stop() calls, and it relies on the fixes to disconnect detection implemented in https://github.com/bitcoin-core/libmultiprocess/pull/160 to work effectively. --- src/init.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index cae52675525..7952495c093 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -298,6 +299,14 @@ void Shutdown(NodeContext& node) StopREST(); StopRPC(); StopHTTPServer(); + for (auto& client : node.chain_clients) { + try { + client->stop(); + } catch (const ipc::Exception& e) { + LogDebug(BCLog::IPC, "Chain client did not disconnect cleanly: %s", e.what()); + client.reset(); + } + } StopMapPort(); // Because these depend on each-other, we make sure that neither can be @@ -370,9 +379,6 @@ void Shutdown(NodeContext& node) } } } - for (const auto& client : node.chain_clients) { - client->stop(); - } // If any -ipcbind clients are still connected, disconnect them now so they // do not block shutdown.