diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml new file mode 100644 index 0000000..61b2b8c --- /dev/null +++ b/.gitlab-ci.yml @@ -0,0 +1,55 @@ +stages: + - build + - publish + - release + +.if-tag: &if-tag + if: "$CI_COMMIT_TAG =~ /^qi-python-v/" + +build-wheel: + stage: build + rules: + # Allow manually building on a Git commit on any branch. + - if: $CI_COMMIT_BRANCH + when: manual + # Always build on a Git tag. + - <<: *if-tag + tags: + - docker + image: python:3.8 + variables: + LIBQI_REPOSITORY_URL: "https://gitlab-ci-token:$CI_JOB_TOKEN@$CI_SERVER_HOST/qi/libqi" + script: + - curl -sSL https://get.docker.com/ | sh + - pip install cibuildwheel==2.14.1 + - cibuildwheel --output-dir wheelhouse + artifacts: + paths: + - wheelhouse/ + +publish-wheel: + stage: publish + image: python:latest + rules: + - <<: *if-tag + needs: + - build-wheel + script: + - pip install build twine + - python -m build + - TWINE_PASSWORD="${CI_JOB_TOKEN}" + TWINE_USERNAME=gitlab-ci-token + python -m twine upload + --repository-url "${CI_API_V4_URL}/projects/${CI_PROJECT_ID}/packages/pypi" + wheelhouse/* + +create-release: + stage: release + rules: + - <<: *if-tag + script: + - echo "Releasing $CI_COMMIT_TAG." + release: + tag_name: $CI_COMMIT_TAG + name: 'lib$CI_COMMIT_TITLE' + description: '$CI_COMMIT_TAG_MESSAGE' diff --git a/CMakeLists.txt b/CMakeLists.txt index b2a5f22..db1acd8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -66,16 +66,14 @@ endif() ############################################################################## # Find Python ############################################################################## -# First search for Python with Interpreter+Devel components, to allow the module -# to look for the interpreter that goes with the Python development library. -# Then if it could not find both, try looking for the development component -# only, but this time force the module to find it or fail (REQUIRED). -# Some of our toolchains (when crosscompiling for instance) have the Python -# library but not an interpreter that can run on the host. -find_package(Python COMPONENTS Development Interpreter) -if(NOT Python_FOUND) - find_package(Python REQUIRED COMPONENTS Development) -endif() +find_package( + Python REQUIRED + COMPONENTS + Development.Module + OPTIONAL_COMPONENTS + Interpreter + Development.Embed +) ############################################################################## # Find Boost @@ -117,6 +115,7 @@ target_sources( PUBLIC qipython/common.hpp + qipython/common.hxx qipython/pyapplication.hpp qipython/pyasync.hpp qipython/pyclock.hpp @@ -323,47 +322,55 @@ if(BUILD_TESTING) qi::qi ) - add_executable(test_qipython) - target_sources( - test_qipython - PRIVATE - tests/common.hpp - tests/test_qipython.cpp - tests/test_guard.cpp - tests/test_types.cpp - tests/test_signal.cpp - tests/test_property.cpp - tests/test_object.cpp - tests/test_module.cpp - ) - target_link_libraries( - test_qipython + if(Python_Development.Embed_FOUND) + add_executable(test_qipython) + target_sources( + test_qipython + PRIVATE + tests/common.hpp + tests/test_qipython.cpp + tests/test_guard.cpp + tests/test_types.cpp + tests/test_signal.cpp + tests/test_property.cpp + tests/test_object.cpp + tests/test_module.cpp + ) + target_link_libraries( + test_qipython + PRIVATE + qi_python_objects + cxx_standard + Python::Python + Boost::headers + pybind11::pybind11 + GTest::gmock + ) + gtest_discover_tests( + test_qipython + DISCOVERY_MODE PRE_TEST + ) + + add_executable(test_qipython_local_interpreter) + target_sources( + test_qipython_local_interpreter + PRIVATE + tests/test_qipython_local_interpreter.cpp + ) + target_link_libraries( + test_qipython_local_interpreter PRIVATE - qi_python_objects - cxx_standard Python::Python - Boost::headers pybind11::pybind11 + qi_python_objects + cxx_standard GTest::gmock - ) - gtest_discover_tests(test_qipython) - - add_executable(test_qipython_local_interpreter) - target_sources( - test_qipython_local_interpreter - PRIVATE - tests/test_qipython_local_interpreter.cpp - ) - target_link_libraries( - test_qipython_local_interpreter - PRIVATE - Python::Python - pybind11::pybind11 - qi_python_objects - cxx_standard - GTest::gmock - ) - gtest_discover_tests(test_qipython_local_interpreter) + ) + gtest_discover_tests( + test_qipython_local_interpreter + DISCOVERY_MODE PRE_TEST + ) + endif() if(NOT Python_Interpreter_FOUND) message(WARNING "tests: a compatible Python Interpreter was NOT found, Python tests are DISABLED.") diff --git a/README.rst b/README.rst index 2383130..8d35691 100644 --- a/README.rst +++ b/README.rst @@ -10,9 +10,30 @@ __ LibQi_repo_ Building ======== -The libqi-python project requires a compiler that supports C++17 to build. +To build the project, you need: -It is built with CMake >= 3.23. +- a compiler that supports C++17. + + - on Ubuntu: `apt-get install build-essential`. + +- CMake with at least version 3.23. + + - on PyPI (**recommended**): `pip install "cmake>=3.23"`. + - on Ubuntu: `apt-get install cmake`. + +- Python with at least version 3.7 and its development libraries. + + - On Ubuntu: `apt-get install libpython3-dev`. + +- a Python `virtualenv`. + + - On Ubuntu: + + .. code-block:: console + + apt-get install python3-venv + python3 -m venv ~/my-venv # Use the path of your convenience. + source ~/my-venv/bin/activate .. note:: The CMake project offers several configuration options and exports a set @@ -21,13 +42,13 @@ It is built with CMake >= 3.23. .. note:: The procedures described below assume that you have downloaded the project - sources and that your current working directory is the project sources root - directory. + sources, that your current working directory is the project sources root + directory and that you are working inside your virtualenv. Conan ^^^^^ -Additionally, libqi-python is available as a Conan 2 project, which means you +Additionally, `libqi-python` is available as a Conan 2 project, which means you can use Conan to fetch dependencies. You can install and/or upgrade Conan 2 and create a default profile in the @@ -46,11 +67,35 @@ Install dependencies from Conan and build with CMake The procedure to build the project using Conan to fetch dependencies is the following. -You must first install the project dependencies in Conan. +Most dependencies are available on Conan Center repository and should not +require any additional steps to make them available. However, you might need to +first get and export the `libqi` recipe into your local Conan cache. .. code-block:: console - conan install . --build=missing -s build_type=Debug + # GitHub is available, but you can also use internal GitLab. + QI_REPOSITORY="https://github.com/aldebaran/libqi.git" + QI_VERSION="4.0.1" # Checkout the version your project need. + QI_PATH="$HOME/libqi" # Or whatever path you want. + git clone \ + --depth=1 `# Only fetch one commit.` \ + --branch "qi-framework-v${QI_VERSION}" \ + "${QI_REPOSITORY}" \ + "${QI_PATH}" + conan export "${QI_PATH}" \ + --version "${QI_VERSION}" # Technically not required but some + # versions of libqi require it + # because of a bug. + +You can then install the `libqi-python` dependencies in Conan. + +.. code-block:: console + + conan install . \ + --build=missing `# Build dependencies binaries that are missing in Conan.` \ + -s build_type=Debug `# Build in debug mode.` \ + -c tools.build:skip_test=true `# Skip tests building for dependencies.` \ + -c '&:tools.build:skip_test=false' # Do not skip tests for the project. This will generate a build directory containing a configuration with a toolchain file that allows CMake to find dependencies inside the Conan cache. @@ -73,11 +118,14 @@ To start building, you need to configure with CMake and then build: cmake --preset conan-linux-x86_64-gcc-debug cmake --build --preset conan-linux-x86_64-gcc-debug -You can then invoke tests using CTest_: +Tests can now be invoked using CTest_, but they require a runtime environment +from Conan so that all dependencies are found: .. code-block:: console + source build/linux-x86_64-gcc-debug/generators/conanrun.sh ctest --preset conan-linux-x86_64-gcc-debug --output-on-failure + source build/linux-x86_64-gcc-debug/generators/deactivate_conanrun.sh Finally, you can install the project in the directory of your choice. @@ -85,8 +133,9 @@ The project defines a single install component, the ``Module`` component. .. code-block:: console - # "cmake --install" does not support preset sadly. - cmake --install build/linux-x86_64-gcc-debug + # `cmake --install` does not support presets sadly. + cmake \ + --install build/linux-x86_64-gcc-debug \ --component Module --prefix ~/my-libqi-python-install Wheel (PEP 517) @@ -101,21 +150,38 @@ dependencies, such as a toolchain generated by Conan: .. code-block:: console - conan install . --build=missing + conan install . \ + --build=missing `# Build dependencies binaries that are missing in Conan.` \ + -c tools.build:skip_test=true # Skip any test. You now can use the ``build`` Python module to build the wheel using PEP 517. .. code-block:: console - export CMAKE_TOOLCHAIN_FILE=$PWD/build/linux-x86_64-gcc-release/generators/conan_toolchain.cmake - python -m build + pip install -U build + python -m build \ + --config-setting cmake.define.CMAKE_TOOLCHAIN_FILE=$PWD/build/linux-x86_64-gcc-release/generators/conan_toolchain.cmake When built that way, the native libraries present in the wheel are most likely incomplete. You will need to use ``auditwheel`` or ``delocate`` to fix it. +.. note:: + `auditwheel` requires the `patchelf` utility program on Linux. You may need + to install it (on Ubuntu: `apt-get install patchelf`). + .. code-block:: console - auditwheel repair --plat manylinux_2_31_x86_64 dist/qi-*.whl + pip install -U auditwheel # or `delocate` on MacOS. + auditwheel repair \ + --strip `# Strip debugging symbols to get a lighter archive.` \ + `# The desired platform, which may differ depending on your build host.` \ + `# With Ubuntu 20.04, we can target manylinux_2_31. Newer versions of` \ + `# Ubuntu will have to target newer versions of manylinux.` \ + `# If you don't need a manylinux archive, you can also target the` \ + `# 'linux_x86_64' platform.` \ + --plat manylinux_2_31_x86_64 \ + `# Path to the wheel archive.` \ + dist/qi-*.whl # The wheel will be by default placed in a `./wheelhouse/` directory. Crosscompiling @@ -123,12 +189,11 @@ Crosscompiling The project supports cross-compiling as explained in the `CMake manual about toolchains`__. You may simply set the ``CMAKE_TOOLCHAIN_FILE`` variable to the -path of the CMake file in your toolchain. +path of the CMake file of your toolchain. __ CMake_toolchains_ .. _LibQi_repo: https://github.com/aldebaran/libqi .. _scikit-build: https://scikit-build.readthedocs.io/en/latest/ -.. _setuptools: https://setuptools.readthedocs.io/en/latest/setuptools.html .. _CMake_toolchains: https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html .. _CTest: https://cmake.org/cmake/help/latest/manual/ctest.1.html diff --git a/ci/cibuildwheel_linux_before_all.sh b/ci/cibuildwheel_linux_before_all.sh new file mode 100755 index 0000000..f033f37 --- /dev/null +++ b/ci/cibuildwheel_linux_before_all.sh @@ -0,0 +1,30 @@ +#!/bin/sh +set -x -e + +PACKAGE=$1 + +pip install 'conan>=2' 'cmake>=3.23' ninja + +# Perl dependencies required to build OpenSSL. +yum install -y perl-IPC-Cmd perl-Digest-SHA + +# Install Conan configuration. +conan config install "$PACKAGE/ci/conan" + +# Clone and export libqi to Conan cache. +QI_VERSION=$(sed -nE '/^\s*requires\s*=/,/^\s*]/{ s/\s*"qi\/([^"]+)".*/\1/p }' "$PACKAGE/conanfile.py") + +GIT_SSL_NO_VERIFY=true \ + git clone --depth=1 \ + --branch "qi-framework-v${QI_VERSION}" \ + "$LIBQI_REPOSITORY_URL" \ + /work/libqi +conan export /work/libqi --version="${QI_VERSION}" + +# Install dependencies of libqi-python from Conan, including libqi. +# +# Build everything from sources, so that we do not reuse precompiled binaries. +# This is because the GLIBC from the manylinux images are often older than the +# ones that were used to build the precompiled binaries, which means the binaries +# cannot by executed. +conan install "$PACKAGE" --build="*" diff --git a/ci/conan/global.conf b/ci/conan/global.conf new file mode 100644 index 0000000..7ad81c5 --- /dev/null +++ b/ci/conan/global.conf @@ -0,0 +1,7 @@ +core:default_profile=default +core:default_build_profile=default +tools.build:skip_test=true +tools.cmake.cmaketoolchain:generator=Ninja +# Only use the build_type as a variable for the build folder name, so +# that the generated CMake preset is named "conan-release". +tools.cmake.cmake_layout:build_folder_vars=["settings.build_type"] diff --git a/ci/conan/profiles/default b/ci/conan/profiles/default new file mode 100644 index 0000000..4ca0f95 --- /dev/null +++ b/ci/conan/profiles/default @@ -0,0 +1,8 @@ +[settings] +arch=x86_64 +build_type=Release +compiler=gcc +compiler.cppstd=gnu17 +compiler.libcxx=libstdc++ +compiler.version=10 +os=Linux diff --git a/conanfile.py b/conanfile.py index 6777ae0..9e16c4c 100644 --- a/conanfile.py +++ b/conanfile.py @@ -36,30 +36,31 @@ ] USED_BOOST_COMPONENTS = [ - "atomic", # required by thread - "chrono", # required by thread - "container", # required by thread - "date_time", # required by thread - "exception", # required by thread - "filesystem", # required by libqi - "locale", # required by libqi - "program_options", # required by libqi - "random", # required by libqi - "regex", # required by libqi - "system", # required by thread "thread", + # required by libqi + "filesystem", + "locale", + "program_options", + "random", + "regex", + # required by boost.thread + "atomic", + "chrono", + "container", + "date_time", + "exception", + "system", ] - class QiPythonConan(ConanFile): requires = [ "boost/[~1.78]", - "pybind11/[~2.9]", - "qi/4.0.1", + "pybind11/[^2.9]", + "qi/4.0.2", ] test_requires = [ - "gtest/cci.20210126", + "gtest/[~1.14]", ] generators = "CMakeToolchain", "CMakeDeps" diff --git a/pyproject.toml b/pyproject.toml index 6e048cc..794715e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,7 +7,7 @@ build-backend = "scikit_build_core.build" [project] name = "qi" description = "LibQi Python bindings" -version = "3.1.3" +version = "3.1.4" readme = "README.rst" requires-python = ">=3.7" license = { "file" = "COPYING" } @@ -47,3 +47,24 @@ maintainers = [ [project.urls] repository = "https://github.com/aldebaran/libqi-python" + +[tool.scikit-build] +# Rely only on CMake install, not on Python packages detection. +wheel.packages = [] + +[tool.scikit-build.cmake.define] +# Disable building tests by default when building a wheel. +BUILD_TESTING = "OFF" + +[tool.cibuildwheel] +build = "cp*manylinux*x86_64" +build-frontend = "build" + +environment-pass = ["LIBQI_REPOSITORY_URL"] + +[tool.cibuildwheel.linux] +manylinux-x86_64-image = "manylinux2014" +before-all = ["ci/cibuildwheel_linux_before_all.sh {package}"] + +[tool.cibuildwheel.linux.config-settings] +"cmake.args" = ["--preset=conan-release"] diff --git a/qipython/common.hpp b/qipython/common.hpp index 55fbf78..3335f66 100644 --- a/qipython/common.hpp +++ b/qipython/common.hpp @@ -125,6 +125,28 @@ inline boost::optional interpreterIsFinalizing() #endif } +/// Acquires the GIL, then invokes a function with the given arguments, but +/// catches any `pybind11::error_already_set` exception thrown during this +/// invocation, and throws a `std::runtime_error` instead. This is useful to +/// avoid the undefined behavior caused by the `what` member function of +/// `pybind11::error_already_set` when the GIL is not acquired. +template +auto invokeCatchPythonError(F&& f, Args&&... args); + +/// Exception type thrown when an operation fails because the interpreter is +/// finalizing. +/// +/// Some operations are forbidden during interpreter finalization as they may +/// terminate the thread they are called from. This exception allows stopping +/// the flow of execution in this case before such functions are called. +struct InterpreterFinalizingException : std::exception +{ + inline const char* what() const noexcept override + { + return "the interpreter is finalizing"; + } +}; + } // namespace py } // namespace qi @@ -203,4 +225,6 @@ namespace detail PYBIND11_DECLARE_HOLDER_TYPE(T, boost::shared_ptr); +#include + #endif // QIPYTHON_COMMON_HPP diff --git a/qipython/common.hxx b/qipython/common.hxx new file mode 100644 index 0000000..2618b47 --- /dev/null +++ b/qipython/common.hxx @@ -0,0 +1,37 @@ +/* +** Copyright (C) 2023 Aldebaran Robotics +** See COPYING for the license +*/ + +#pragma once + +#ifndef QIPYTHON_COMMON_HXX +#define QIPYTHON_COMMON_HXX + +#include +#include +#include + +namespace qi +{ +namespace py +{ + +template +auto invokeCatchPythonError(F&& f, Args&&... args) +{ + GILAcquire acquire; + try + { + return std::invoke(std::forward(f), std::forward(args)...); + } + catch (const pybind11::error_already_set& err) + { + throw std::runtime_error(err.what()); + } +} + +} // namespace py +} // namespace qi + +#endif // QIPYTHON_COMMON_HXX diff --git a/qipython/pyguard.hpp b/qipython/pyguard.hpp index 33ea618..f20ee1b 100644 --- a/qipython/pyguard.hpp +++ b/qipython/pyguard.hpp @@ -10,6 +10,7 @@ #include #include +#include #include namespace qi @@ -17,10 +18,25 @@ namespace qi namespace py { -/// Returns whether or not the GIL is currently held by the current thread. -inline bool currentThreadHoldsGil() +/// Returns whether or not the GIL is currently held by the current thread. If +/// the interpreter is not yet initialized or has been finalized, returns an +/// empty optional, as there is no GIL available. +inline boost::optional currentThreadHoldsGil() { - return PyGILState_Check() == 1; + // PyGILState_Check() returns 1 (success) before the creation of the GIL and + // after the destruction of the GIL. + if (Py_IsInitialized() == 1) { + const auto gilAcquired = PyGILState_Check() == 1; + return boost::make_optional(gilAcquired); + } + return boost::none; +} + +/// Returns whether or not the GIL exists (i.e the interpreter is initialized +/// and not finalizing) and is currently held by the current thread. +inline bool gilExistsAndCurrentThreadHoldsIt() +{ + return currentThreadHoldsGil().value_or(false); } /// DefaultConstructible Guard @@ -35,98 +51,6 @@ ka::ResultOf invokeGuarded(F&& f, Args&&... args) namespace detail { -// Guards the copy, construction and destruction of an object, and only when -// in case of copy construction the source object evaluates to true, and in case -// of destruction, the object itself evaluates to true. -// -// Explanation: -// pybind11::object only requires the GIL to be locked on copy and if the -// source is not null. Default construction does not require the GIL either. -template -class Guarded -{ - using Storage = - typename std::aligned_storage::type; - Storage _object; - - Object& object() { return reinterpret_cast(_object); } - const Object& object() const { return reinterpret_cast(_object); } - -public: - template::value, int> = 0> - explicit Guarded(Arg0&& arg0, Args&&... args) - { - Guard g; - new(&_object) Object(std::forward(arg0), std::forward(args)...); - } - - Guarded() - { - // Default construction, no GIL needed. - new(&_object) Object(); - } - - Guarded(Guarded& o) - { - boost::optional optGuard; - if (o.object()) - optGuard.emplace(); - new(&_object) Object(o.object()); - } - - Guarded(const Guarded& o) - { - boost::optional optGuard; - if (o.object()) - optGuard.emplace(); - new(&_object) Object(o.object()); - } - - Guarded(Guarded&& o) - { - new(&_object) Object(std::move(o.object())); - } - - ~Guarded() - { - boost::optional optGuard; - if (object()) - optGuard.emplace(); - object().~Object(); - } - - Guarded& operator=(const Guarded& o) - { - if (this == &o) - return *this; - - { - boost::optional optGuard; - if (o.object()) - optGuard.emplace(); - object() = o.object(); - } - return *this; - } - - Guarded& operator=(Guarded&& o) - { - if (this == &o) - return *this; - - object() = std::move(o.object()); - return *this; - } - - Object& operator*() { return object(); } - const Object& operator*() const { return object(); } - Object* operator->() { return &object(); } - const Object* operator->() const { return &object(); } - explicit operator Object&() { return object(); } - explicit operator const Object&() const { return object(); } -}; - /// G == pybind11::gil_scoped_acquire || G == pybind11::gil_scoped_release template void pybind11GuardDisarm(G& guard) @@ -141,37 +65,52 @@ void pybind11GuardDisarm(G& guard) } // namespace detail /// RAII utility type that guarantees that the GIL is locked for the scope of -/// the lifetime of the object. +/// the lifetime of the object. If the GIL cannot be acquired (for example, +/// because the interpreter is finalizing), throws an `InterpreterFinalizingException` +/// exception. /// /// Objects of this type (or objects composed of them) must not be kept alive /// after the hand is given back to the interpreter. /// /// This type is re-entrant. /// -/// postcondition: `GILAcquire acq;` establishes -/// `(interpreterIsFinalizing() && *interpreterIsFinalizing()) || currentThreadHoldsGil()` +/// postcondition: `GILAcquire acq;` establishes `gilExistsAndCurrentThreadHoldsIt()` struct GILAcquire { inline GILAcquire() { - const auto optIsFinalizing = interpreterIsFinalizing(); - const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing; - // `gil_scoped_acquire` is re-entrant by itself, so we don't need to check - // whether or not the GIL is already held by the current thread. - if (!definitelyFinalizing) - _acq.emplace(); - QI_ASSERT(definitelyFinalizing || currentThreadHoldsGil()); + if (gilExistsAndCurrentThreadHoldsIt()) + return; + + const auto isFinalizing = interpreterIsFinalizing().value_or(false); + if (isFinalizing) + throw InterpreterFinalizingException(); + + _state = ::PyGILState_Ensure(); + QI_ASSERT(gilExistsAndCurrentThreadHoldsIt()); + } + + inline ~GILAcquire() + { + // Even if releasing the GIL while the interpreter is finalizing is allowed, it does + // require the GIL to be currently held. But we have no guarantee that this is the case, + // because the GIL may have been released since we acquired it, and we could not + // reacquire it after that (maybe the interpreter is in fact finalizing). + // Therefore, only release the GIL if it is currently held by this thread. + if (_state && gilExistsAndCurrentThreadHoldsIt()) + ::PyGILState_Release(*_state); } + GILAcquire(const GILAcquire&) = delete; GILAcquire& operator=(const GILAcquire&) = delete; private: - boost::optional _acq; + boost::optional _state; }; -/// RAII utility type that guarantees that the GIL is unlocked for the scope of -/// the lifetime of the object. +/// RAII utility type that (as a best effort) tries to ensure that the GIL is +/// unlocked for the scope of the lifetime of the object. /// /// Objects of this type (or objects composed of them) must not be kept alive /// after the hand is given back to the interpreter. @@ -179,16 +118,44 @@ struct GILAcquire /// This type is re-entrant. /// /// postcondition: `GILRelease rel;` establishes -/// `(interpreterIsFinalizing() && *interpreterIsFinalizing()) || !currentThreadHoldsGil()` +/// `interpreterIsFinalizing().value_or(false) || +/// !gilExistsAndCurrentThreadHoldsIt()` struct GILRelease { inline GILRelease() { - const auto optIsFinalizing = interpreterIsFinalizing(); - const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing; - if (!definitelyFinalizing && currentThreadHoldsGil()) + // Even if releasing the GIL while the interpreter is finalizing is allowed, + // it does require the GIL to be currently held. However, reacquiring the + // GIL is forbidden if finalization started. + // + // It may happen that we try to release the GIL from a function ('F') called + // by the Python interpreter, while it is holding the GIL. In this case, if + // we try to release it, then fail to reacquire it and return the hand to + // the interpreter, the interpreter will most likely terminate the process, + // as it expects to still be holding the GIL. Failure to reacquire the GIL + // can only happen if the interpreter started finalization, either before we + // released it, or while it was released. + // + // Fortunately, in this case, because the interpreter is busy executing the + // function 'F', it is not possible for it to start finalization until 'F' + // returns. This means that the only possible reason of failure of + // reacquisition of the GIL is because the interpreter was finalizing + // *before* calling 'F', therefore before we released to GIL. Therefore, to + // prevent this failure, we check if the interpreter is finalizing before + // releasing the GIL, and if it is, we do nothing, and the GIL stays held. + const auto isFinalizing = interpreterIsFinalizing().value_or(false); + if (!isFinalizing && gilExistsAndCurrentThreadHoldsIt()) _release.emplace(); - QI_ASSERT(definitelyFinalizing || !currentThreadHoldsGil()); + QI_ASSERT(isFinalizing || !gilExistsAndCurrentThreadHoldsIt()); + } + + inline ~GILRelease() + { + // Reacquiring the GIL is forbidden when the interpreter is finalizing, as + // it may terminate the current thread. + const auto isFinalizing = interpreterIsFinalizing().value_or(false); + if (_release && isFinalizing) + detail::pybind11GuardDisarm(*_release); } GILRelease(const GILRelease&) = delete; @@ -198,11 +165,83 @@ struct GILRelease boost::optional _release; }; -/// Wraps a pybind11::object value and locks the GIL on copy and destruction. +/// Wraps a Python object as a shared reference-counted value that does not +/// require the GIL to copy, move or assign to. /// -/// This is useful for instance to put pybind11 objects in lambda functions so -/// that they can be copied around safely. -using GILGuardedObject = detail::Guarded; +/// On destruction, it releases the object with the GIL acquired. However, if +/// the GIL cannot be acquired, the object is leaked. This is most likely +/// mitigated by the fact that if the GIL is not available, it means the object +/// already has been or soon will be garbage collected by interpreter +/// finalization. +template +class SharedObject +{ + static_assert(std::is_base_of_v, + "template parameter T must be a subclass of pybind11::object"); + + struct State + { + std::mutex mutex; + T object; + }; + std::shared_ptr _state; + + struct StateDeleter + { + inline void operator()(State* state) const + { + auto handle = state->object.release(); + delete state; + + // Do not lock the GIL if there is nothing to release. + if (!handle) + return; + + try + { + GILAcquire acquire; + handle.dec_ref(); + } + catch (const qi::py::InterpreterFinalizingException&) + { + // Nothing, the interpreter is finalizing. + } + } + }; + +public: + SharedObject() = default; + + inline explicit SharedObject(T object) + : _state(new State { std::mutex(), std::move(object) }, StateDeleter()) + { + } + + /// Copies the inner Python object value by incrementing its reference count. + /// + /// @pre: If the inner value is not null, the GIL must be acquired. + T inner() const + { + QI_ASSERT_NOT_NULL(_state); + std::scoped_lock lock(_state->mutex); + return _state->object; + } + + /// Takes the inner Python object value and leaves a null value in its place. + /// + /// Any copy of the shared object will now have a null inner value. + /// + /// This operation does not require the GIL as the reference count of the + /// object is preserved. + T takeInner() + { + QI_ASSERT_NOT_NULL(_state); + std::scoped_lock lock(_state->mutex); + /// Hypothesis: Moving a `pybind11::object` steals the object and preserves + /// its reference count and therefore does not require the GIL. + return ka::exchange(_state->object, {}); + } +}; /// Deleter that deletes the pointer outside the GIL. /// @@ -218,23 +257,6 @@ struct DeleteOutsideGIL } }; -/// Deleter that delays the destruction of an object to another thread. -struct DeleteInOtherThread -{ - template - void operator()(T* ptr) const - { - GILRelease unlock; - // `std::async` returns an object, that unless moved from will block when - // destroyed until the task is complete, which is unwanted behavior here. - // Therefore we just take the result of the `std::async` call into a local - // variable and then ignore it. - auto fut = std::async(std::launch::async, [](std::unique_ptr) {}, - std::unique_ptr(ptr)); - QI_IGNORE_UNUSED(fut); - } -}; - } // namespace py } // namespace qi diff --git a/src/pyapplication.cpp b/src/pyapplication.cpp index 315d842..e9a0ed0 100644 --- a/src/pyapplication.cpp +++ b/src/pyapplication.cpp @@ -83,7 +83,7 @@ void exportApplication(::py::module& m) GILAcquire lock; - class_>( + class_( m, "Application") .def(init(withArgcArgv<>([](int& argc, char**& argv) { GILRelease unlock; @@ -93,8 +93,7 @@ void exportApplication(::py::module& m) .def_static("run", &Application::run, call_guard()) .def_static("stop", &Application::stop, call_guard()); - class_>( + class_( m, "ApplicationSession") .def(init(withArgcArgv( diff --git a/src/pyasync.cpp b/src/pyasync.cpp index 01b6310..f39ca2e 100644 --- a/src/pyasync.cpp +++ b/src/pyasync.cpp @@ -38,19 +38,15 @@ ::py::object async(::py::function pyCallback, const MicroSeconds delay(usDelay); - GILGuardedObject guardCb(pyCallback); - GILGuardedObject guardArgs(std::move(args)); - GILGuardedObject guardKwargs(std::move(kwargs)); - + SharedObject sharedCb(pyCallback); + SharedObject sharedArgs(std::move(args)); + SharedObject sharedKwArgs(std::move(kwargs)); auto invokeCallback = [=]() mutable { - GILAcquire lock; - auto res = ::py::object((*guardCb)(**guardArgs, ***guardKwargs)).cast(); - // Release references immediately while we hold the GIL, instead of waiting - // for the lambda destructor to relock the GIL. - *guardKwargs = {}; - *guardArgs = {}; - *guardCb = {}; - return res; + GILAcquire acquire; + return invokeCatchPythonError( + sharedCb.takeInner(), + *sharedArgs.takeInner(), + **sharedKwArgs.takeInner()).cast(); }; // If there is a strand attached to that callable, we use it but we cannot use diff --git a/src/pyfuture.cpp b/src/pyfuture.cpp index 9c473c9..dc4c6fb 100644 --- a/src/pyfuture.cpp +++ b/src/pyfuture.cpp @@ -53,8 +53,8 @@ template std::function(Args...)> toContinuation(const ::py::function& cb) { GILAcquire lock; - GILGuardedObject guardedCb(cb); - auto callGuardedCb = [=](Args... args) mutable { + SharedObject sharedCb(cb); + auto callSharedCb = [=](Args... args) mutable { GILAcquire lock; const auto handleExcept = ka::handle_exception_rethrow( exceptionLogVerbose( @@ -62,17 +62,18 @@ std::function(Args...)> toContinuation(const ::py::function& cb) "An exception occurred while executing a future continuation"), ka::type_t<::py::object>{}); const auto pyRes = - ka::invoke_catch(handleExcept, *guardedCb, std::forward(args)...); - // Release references immediately while we hold the GIL, instead of waiting - // for the lambda destructor to relock the GIL. - *guardedCb = {}; + ka::invoke_catch(handleExcept, [&] { + return invokeCatchPythonError( + sharedCb.takeInner(), + std::forward(args)...); + }); return castIfNotVoid(pyRes); }; auto strand = strandOfFunction(cb); if (strand) - return strand->schedulerFor(std::move(callGuardedCb)); - return futurizeOutput(std::move(callGuardedCb)); + return strand->schedulerFor(std::move(callSharedCb)); + return futurizeOutput(std::move(callSharedCb)); } void addCallback(Future fut, const ::py::function& cb) diff --git a/src/pyobject.cpp b/src/pyobject.cpp index ddd1a4f..9bc1019 100644 --- a/src/pyobject.cpp +++ b/src/pyobject.cpp @@ -149,7 +149,7 @@ using GenericFunction = std::function<::py::object(::py::args)>; // @pre `cargs.size() > 0`, arguments must at least contain a `DynamicObject` // on which the function is to be called. AnyReference callPythonMethod(const AnyReferenceVector& cargs, - const GILGuardedObject& method) + const SharedObject<::py::function>& method) { auto it = cargs.begin(); const auto cargsEnd = cargs.end(); @@ -170,7 +170,7 @@ AnyReference callPythonMethod(const AnyReferenceVector& cargs, // Convert Python future object into a C++ Future, to allow libqi to unwrap // it. - const ::py::object ret = (*method)(*args); + const ::py::object ret = invokeCatchPythonError(method.inner(), *args); if (::py::isinstance(ret)) return AnyValue::from(ret.cast()).release(); return AnyReference::from(ret).content().clone(); @@ -259,7 +259,7 @@ boost::optional registerMethod(DynamicObjectBuilder& gob, return gob.xAdvertiseMethod(mmb, AnyFunction::fromDynamicFunction( boost::bind(callPythonMethod, _1, - GILGuardedObject(method)))); + SharedObject(method)))); } } // namespace @@ -333,7 +333,7 @@ Object toObject(const ::py::object& obj) : ObjectThreadingModel_SingleThread); const auto attrKeys = ::py::reinterpret_steal<::py::list>(PyObject_Dir(obj.ptr())); - for (const ::py::handle& pyAttrKey : attrKeys) + for (const auto& pyAttrKey : attrKeys) { QI_ASSERT_TRUE(pyAttrKey); QI_ASSERT_FALSE(pyAttrKey.is_none()); @@ -342,7 +342,7 @@ Object toObject(const ::py::object& obj) const auto attrKey = pyAttrKey.cast(); auto memberName = attrKey; - const ::py::object attr = obj.attr(pyAttrKey); + const auto attr = obj.attr(pyAttrKey); if (attr.is_none()) { qiLogVerbose() << "The object attribute '" << attrKey @@ -393,11 +393,7 @@ Object toObject(const ::py::object& obj) // This is a useless callback, needed to keep a reference on the python object. // When the GenericObject is destroyed, the reference is released. - GILGuardedObject guardedObj(obj); - Object anyobj = gob.object([guardedObj](GenericObject*) mutable { - GILAcquire lock; - *guardedObj = {}; - }); + Object anyobj = gob.object([sharedObj = SharedObject(obj)](GenericObject*) {}); // If there was no ObjectUid stored in the python object, store a new one. if (!maybeObjectUid) diff --git a/src/pysignal.cpp b/src/pysignal.cpp index aa3a864..b6c3e9d 100644 --- a/src/pysignal.cpp +++ b/src/pysignal.cpp @@ -26,7 +26,7 @@ namespace constexpr static const auto asyncArgName = "_async"; -AnyReference dynamicCallFunction(const GILGuardedObject& func, +AnyReference dynamicCallFunction(const SharedObject<::py::function>& func, const AnyReferenceVector& args) { GILAcquire lock; @@ -34,7 +34,7 @@ AnyReference dynamicCallFunction(const GILGuardedObject& func, ::py::size_t i = 0; for (const auto& arg : args) pyArgs[i++] = castToPyObject(arg); - (*func)(*pyArgs); + invokeCatchPythonError(func.inner(), *pyArgs); return AnyValue::makeVoid().release(); } @@ -49,7 +49,7 @@ ::py::object connect(F&& connect, SignalSubscriber subscriber(AnyFunction::fromDynamicFunction( boost::bind(dynamicCallFunction, - GILGuardedObject(pyCallback), + SharedObject(pyCallback), _1)), strand.get()); subscriber.setCallType(MetaCallType_Auto); diff --git a/src/pystrand.cpp b/src/pystrand.cpp index 7da98c5..0e1b2d8 100644 --- a/src/pystrand.cpp +++ b/src/pystrand.cpp @@ -35,15 +35,15 @@ bool isUnboundMethod(const ::py::function& func, const ::py::object& object) GILAcquire lock; const auto dir = ::py::reinterpret_steal<::py::list>(PyObject_Dir(object.ptr())); - for (const ::py::handle attrName : dir) + for (const auto& attrName : dir) { - const ::py::handle attr = object.attr(attrName); + const auto attr = object.attr(attrName); QI_ASSERT_TRUE(attr); if (!PyMethod_Check(attr.ptr())) continue; - const ::py::handle unboundMethod = PyMethod_GET_FUNCTION(attr.ptr()); + const auto unboundMethod = ::py::reinterpret_borrow<::py::object>(PyMethod_GET_FUNCTION(attr.ptr())); if (func.is(unboundMethod)) return true; } diff --git a/src/pytypes.cpp b/src/pytypes.cpp index 088ba13..fa03102 100644 --- a/src/pytypes.cpp +++ b/src/pytypes.cpp @@ -28,16 +28,6 @@ namespace py namespace { -struct ObjectDecRef -{ - ::py::handle obj; - void operator()() const - { - GILAcquire lock; - obj.dec_ref(); - } -}; - template boost::optional<::py::object> tryToCastObjectTo(ObjectTypeInterface* type, void* ptr) diff --git a/tests/test_guard.cpp b/tests/test_guard.cpp index bc16e48..d9d835c 100644 --- a/tests/test_guard.cpp +++ b/tests/test_guard.cpp @@ -39,70 +39,6 @@ TEST_F(InvokeGuardedTest, ExecutesFunctionWithGuard) EXPECT_EQ(52, r); } -using GuardedTest = GuardTest; - -TEST_F(GuardedTest, GuardsConstruction) -{ - using T = GuardedTest; - - struct Check - { - int i; - Check(int i) : i(i) { EXPECT_TRUE(T::guarded); } - explicit operator bool() const { return true; } - }; - - EXPECT_FALSE(T::guarded); - qi::py::detail::Guarded g(42); - EXPECT_FALSE(T::guarded); - EXPECT_EQ(42, g->i); -} - -TEST_F(GuardedTest, GuardsCopyConstruction) -{ - using T = GuardedTest; - - struct Check - { - int i = 0; - Check(int i) : i(i) {} - Check(const Check& o) - : i(o.i) - { EXPECT_TRUE(T::guarded); } - explicit operator bool() const { return true; } - }; - - EXPECT_FALSE(T::guarded); - qi::py::detail::Guarded a(42); - qi::py::detail::Guarded b(a); - EXPECT_FALSE(T::guarded); - EXPECT_EQ(42, a->i); - EXPECT_EQ(42, b->i); -} - -TEST_F(GuardedTest, GuardsCopyAssignment) -{ - using T = GuardedTest; - - struct Check - { - int i = 0; - Check(int i) : i(i) {} - Check& operator=(const Check& o) - { EXPECT_TRUE(T::guarded); i = o.i; return *this; } - explicit operator bool() const { return true; } - }; - - EXPECT_FALSE(T::guarded); - qi::py::detail::Guarded a(42); - qi::py::detail::Guarded b(37); - EXPECT_FALSE(T::guarded); - b = a; - EXPECT_FALSE(T::guarded); - EXPECT_EQ(42, a->i); - EXPECT_EQ(42, b->i); -} - TEST(GILAcquire, IsReentrant) { qi::py::GILAcquire acq0; QI_IGNORE_UNUSED(acq0); @@ -120,3 +56,57 @@ TEST(GILRelease, IsReentrant) qi::py::GILRelease rel2; QI_IGNORE_UNUSED(rel2); SUCCEED(); } + +struct SharedObject : testing::Test +{ + SharedObject() + { + // GIL is only required for creation of the inner object. + object = [&]{ + qi::py::GILAcquire lock; + return pybind11::capsule( + &this->destroyed, + [](void* destroyed){ + *reinterpret_cast(destroyed) = true; + } + ); + }(); + } + + ~SharedObject() + { + if (object) { + qi::py::GILAcquire lock; + object = {}; + } + } + + bool destroyed = false; + pybind11::capsule object; +}; + +TEST_F(SharedObject, KeepsRefCount) +{ + std::optional sharedObject = qi::py::SharedObject(std::move(object)); + ASSERT_FALSE(object); // object has been released. + EXPECT_FALSE(destroyed); // inner object is still alive. + { + auto sharedObjectCpy = *sharedObject; // copy the shared object locally. + EXPECT_FALSE(destroyed); // inner object is maintained by both copies. + sharedObject.reset(); + EXPECT_FALSE(destroyed); // inner object is maintained by the copy. + } + EXPECT_TRUE(destroyed); // inner object has been destroyed. +} + +TEST_F(SharedObject, TakeInnerStealsInnerRefCount) +{ + std::optional sharedObject = qi::py::SharedObject(std::move(object)); + auto inner = sharedObject->takeInner(); + EXPECT_FALSE(sharedObject->inner()); // inner object is null. + sharedObject.reset(); + EXPECT_FALSE(destroyed); // inner object is still alive. + qi::py::GILAcquire lock; // release local inner object, which requires the GIL. + inner = {}; + EXPECT_TRUE(destroyed); // inner object has been destroyed. +} diff --git a/tests/test_qipython_local_interpreter.cpp b/tests/test_qipython_local_interpreter.cpp index c1113f7..581255a 100644 --- a/tests/test_qipython_local_interpreter.cpp +++ b/tests/test_qipython_local_interpreter.cpp @@ -26,13 +26,80 @@ PYBIND11_EMBEDDED_MODULE(test_local_interpreter, m) { })); } -TEST(InterpreterFinalize, DoesNotSegfaultGarbageObjectDtorOutsideGIL) +TEST(InterpreterFinalize, GarbageObjectDtorOutsideGIL) { pybind11::scoped_interpreter interp; pybind11::globals()["qitli"] = pybind11::module::import("test_local_interpreter"); pybind11::exec("obj = qitli.ObjectDtorOutsideGIL()"); } +// This test checks that concurrent uses of GIL guards during finalization +// of the interpreter does not cause crashes. +// +// It uses 2 threads: a main thread, and a second native thread. +// +// The main thread starts an interpreter, starts the second thread and +// releases the GIL. The second thread acquires the GIL, then releases +// it, and waits for the interpreter to finalize. Once it is finalized, +// it tries to reacquire the GIL, and then release it, according to +// GIL guards destructors. +// +// Horizontal lines are synchronization points between the threads. +// +// main thread (T1) +// ~~~~~~~~~~~~~~~~ +// ▼ +// ╔═══════╪════════╗ +// ║ interpreter ║ +// ----------------------------------------------> start native thread T2 +// ║ ╎ ║ native thread (T2) +// ║ ╎ ║ ~~~~~~~~~~~~~~~~~~ +// ║ ╎ ║ ▼ +// ║ ╔═════╪══════╗ ║ ╎ +// ║ ║ GILRelease ║ ║ ╎ -> T1 releases the GIL +// ----------------------------------------------> GIL shift T1 -> T2 +// ║ ║ ╎ ║ ║ ╔═══════╪════════╗ +// ║ ║ ╎ ║ ║ ║ GILAcquire ║ -> T2 acquires the GIL +// ║ ║ ╎ ║ ║ ║ ╔═════╪══════╗ ║ +// ║ ║ ╎ ║ ║ ║ ║ GILRelease ║ ║ -> T2 releases the GIL +// ----------------------------------------------> GIL shift T2 -> T1 +// ║ ╚═════╪══════╝ ║ ║ ║ ╎ ║ ║ -> T1 acquires the GIL +// ╚═══════╪════════╝ ║ ║ ╎ ║ ║ -> interpreter starts finalizing +// ----------------------------------------------> interpreter finalized +// ╎ ║ ╚═════╪══════╝ ║ -> T2 tries to reacquire GIL but fails, it's a noop. +// ╎ ╚═══════╪════════╝ -> T2 tries to release GIL, but it's a noop. +// ╎ ╎ +TEST(InterpreterFinalize, GILReleasedInOtherThread) +{ + // Synchronization mechanism for the first GIL shift, otherwise T1 will release and reacquire the + // GIL instantly without waiting for T2. + qi::Promise shift1Prom; + auto shift1Fut = shift1Prom.future(); + // Second GIL shift does not require an additional synchronization + // mechanism. Waiting for interpreter finalization ensures that the + // GIL was acquired back by thread T1. + // Synchronization mechanism for interpreter finalization. + qi::Promise finalizedProm; + std::future asyncFut; + { + pybind11::scoped_interpreter interp; + asyncFut = std::async( + std::launch::async, + [finalizedFut = finalizedProm.future(), shift1Prom]() mutable { + qi::py::GILAcquire acquire; + // First GIL shift is done. + shift1Prom.setValue(nullptr); + qi::py::GILRelease release; + finalizedFut.value(); + }); + qi::py::GILRelease release; + shift1Fut.value(); + } + finalizedProm.setValue(nullptr); + // Join the thread, wait for the task to finish and ensure no exception was thrown. + asyncFut.get(); +} + int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv);