From 285ed719b8149c479c394b8b0530f6d5a78100a2 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Tue, 20 Oct 2026 16:23:07 -0500 Subject: [PATCH 1/3] fix(python): no deadlock when a running Task / Timer / TrajectoryPlanner is deleted or left alive at exit Deleting a python-owned espp object whose destructor joins a thread ran that join with the GIL held; when the thread was waiting for the GIL to run its python callback, neither side could proceed and `del task` hung forever. The call-guard pass could not help (tp_dealloc is not a .def), and at interpreter shutdown the same join happened during finalisation, when a thread can no longer re-acquire the GIL at all. - espp::py_detail::StopOnExitHolder (lib/python_bindings/stop_on_exit_holder.h) is now the pybind11 holder for Task, Timer, TrajectoryPlanner, ThreadPool, FtpServer and RtspServer (_STOP_ON_EXIT_CLASSES, applied by a new postprocess pass of autogenerate_bindings.py). It deletes with the GIL released and registers every live object in a StopRegistry (stop_registry.h); the module's atexit hook stops all of them -- GIL released -- before finalisation, so the later destructors have nothing to join. Classes with a python-visible base (UdpSocket) keep the default holder, which pybind11 requires to match the base's. - TrajectoryPlanner::start_task() / stop_task() become public (they are the documented way to pause and resume the planner, and the hook needs stop_task()); both are bound, stop_task with the GIL released. - python/blocking_destructors_test.py runs each case in a child interpreter under a timeout: delete a running Task / Timer / TrajectoryPlanner whose callback blocks in python, exit with all three alive, and an unhandled exception with a live planner. Before the fix the three delete cases hang; after it all five pass. Added to the wheel test command. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- .../include/trajectory_planner.hpp | 25 +-- lib/README.md | 11 ++ lib/autogenerate_bindings.py | 36 +++++ lib/python_bindings/espp/__init__.pyi | 20 +++ lib/python_bindings/module.cpp | 8 + lib/python_bindings/pybind_espp.cpp | 34 +++- lib/python_bindings/stop_on_exit_holder.h | 98 +++++++++++ lib/python_bindings/stop_registry.h | 79 +++++++++ pyproject.toml | 5 +- python/blocking_destructors_test.py | 153 ++++++++++++++++++ 10 files changed, 448 insertions(+), 21 deletions(-) create mode 100644 lib/python_bindings/stop_on_exit_holder.h create mode 100644 lib/python_bindings/stop_registry.h create mode 100644 python/blocking_destructors_test.py diff --git a/components/trajectory_planner/include/trajectory_planner.hpp b/components/trajectory_planner/include/trajectory_planner.hpp index 8f7a183f92..66b168adf6 100644 --- a/components/trajectory_planner/include/trajectory_planner.hpp +++ b/components/trajectory_planner/include/trajectory_planner.hpp @@ -214,16 +214,6 @@ Leave as nullptr to disable. */ */ bool is_running() const; -protected: - /// Internal motion state tracked between update() calls. - struct State { - float v = 0.0f; /**< Current linear velocity (m/s). */ - float w = 0.0f; /**< Current angular velocity (rad/s). */ - float a_v = 0.0f; /**< Current linear acceleration (m/s^2) -- jerk-limited mode only. */ - float a_w = 0.0f; /**< Current angular acceleration (rad/s^2) -- jerk-limited mode only. */ - }; - friend struct fmt::formatter; - /** * @brief Start the periodic planning and callback timers. * @@ -236,12 +226,25 @@ Leave as nullptr to disable. */ bool start_task(); /** - * @brief Stop the periodic update task. + * @brief Stop the periodic planning and callback timers (joins their tasks). + * + * The planner can be started again with start_task(); the destructor stops + * them itself. * * @return true if the task was stopped, false if it was not running. */ bool stop_task(); +protected: + /// Internal motion state tracked between update() calls. + struct State { + float v = 0.0f; /**< Current linear velocity (m/s). */ + float w = 0.0f; /**< Current angular velocity (rad/s). */ + float a_v = 0.0f; /**< Current linear acceleration (m/s^2) -- jerk-limited mode only. */ + float a_w = 0.0f; /**< Current angular acceleration (rad/s^2) -- jerk-limited mode only. */ + }; + friend struct fmt::formatter; + /** * @brief Advance the planner by one time step. * diff --git a/lib/README.md b/lib/README.md index 8212b8ca01..3ea2ee1d18 100644 --- a/lib/README.md +++ b/lib/README.md @@ -212,6 +212,17 @@ by hand: when called from python, because the blocked-on thread needs the GIL to invoke its python callback (see `_GIL_RELEASE_METHODS` in `autogenerate_bindings.py`; the hand-written `rtps` bindings apply the same guard). +- `espp::py_detail::StopOnExitHolder` as the holder of the classes whose + **destructor** joins a thread (`Task`, `Timer`, `TrajectoryPlanner`, + `ThreadPool`, `FtpServer`, `RtspServer`; `_STOP_ON_EXIT_CLASSES`). A call + guard cannot cover `tp_dealloc`, so deleting a running `Task` from python + joined its thread with the GIL held and deadlocked the moment that thread + needed the GIL for its callback. The holder (`stop_on_exit_holder.h`) deletes + with the GIL released, and registers every live object so the module's + `atexit` hook (`stop_registry.h`) stops their threads before the interpreter + finalises, when a thread could no longer re-acquire the GIL at all. + `python/blocking_destructors_test.py` covers delete-while-running and + exit-while-running for each class (it is part of the wheel test command). The litgen dependency is unpinned and recent versions (0.20–0.22) regressed nested-scope/template generation, which is why this automation is needed. diff --git a/lib/autogenerate_bindings.py b/lib/autogenerate_bindings.py index b46077344c..e73f9e47df 100644 --- a/lib/autogenerate_bindings.py +++ b/lib/autogenerate_bindings.py @@ -239,6 +239,7 @@ def _fix_rtsp_qualifications(code: str) -> str: _GIL_RELEASE_METHODS = { "espp::Task": ["start", "stop"], "espp::Timer": ["start", "stop", "cancel"], + "espp::TrajectoryPlanner": ["start_task", "stop_task"], "espp::FtpServer": ["start", "stop"], "espp::TcpSocket": ["connect", "transmit", "receive", "accept"], "espp::UdpSocket": ["send", "receive", "start_receiving", "stop_receiving"], @@ -254,6 +255,40 @@ def _fix_rtsp_qualifications(code: str) -> str: _GIL_GUARD = "py::call_guard()" +# Classes whose destructor joins a thread (a Task's thread, a Timer's, a TrajectoryPlanner's planning / +# callback tasks, ...). Deleting one from python runs that join with the GIL held, which deadlocks +# whenever the thread is waiting for the GIL to invoke its python callback, and at interpreter +# shutdown the thread can no longer get the GIL at all. These classes get the +# espp::py_detail::StopOnExitHolder holder (lib/python_bindings/stop_on_exit_holder.h): it deletes +# with the GIL released and registers the live object so the module's atexit hook stops its threads +# before finalisation. Only classes without a python-visible base can use it (pybind11 requires a +# derived class to share its base's holder kind); each needs a stop_for_exit() overload in the header. +_STOP_ON_EXIT_CLASSES = [ + "Task", + "Timer", + "TrajectoryPlanner", + "ThreadPool", + "FtpServer", + "RtspServer", +] +_STOP_ON_EXIT_INCLUDE = '#include "stop_on_exit_holder.h"' + + +def _use_stop_on_exit_holders(code: str) -> str: + for name in _STOP_ON_EXIT_CLASSES: + bare = f"py::class_" + replacement = f"py::class_>" + if bare in code: + code = code.replace(bare, replacement) + elif replacement not in code: + print(f"WARNING: stop-on-exit holder for {name} did not match {bare}") + if _STOP_ON_EXIT_INCLUDE not in code: + anchor = '#include "espp.hpp"\n' + assert anchor in code, "generated file does not include espp.hpp" + code = code.replace(anchor, anchor + "\n" + _STOP_ON_EXIT_INCLUDE + "\n", 1) + return code + + def _find_call_end(code: str, open_paren: int) -> int: """Return the index of the ')' closing the call whose '(' is at open_paren. @@ -365,6 +400,7 @@ def _postprocess_generated(code: str) -> str: code = _fix_class_holders(code) code = _fix_rtsp_qualifications(code) code = _add_gil_release_guards(code) + code = _use_stop_on_exit_holders(code) return code diff --git a/lib/python_bindings/espp/__init__.pyi b/lib/python_bindings/espp/__init__.pyi index d6a780f919..7339c299eb 100644 --- a/lib/python_bindings/espp/__init__.pyi +++ b/lib/python_bindings/espp/__init__.pyi @@ -4138,6 +4138,26 @@ class TrajectoryPlanner: """ pass + def start_task(self) -> bool: + """* + * @brief Start the periodic planning and callback timers. + * + * The planning timer calls update() at Config::planning_period. + * The callback task fires output_callback whenever update() produces new output. + * Has no effect if the timers are already running. + * + * @return True if both timers were started successfully. + + """ + pass + def stop_task(self) -> bool: + """* + * @brief Stop the periodic planning and callback timers (joins their tasks). + * + * @return True if the task was stopped, False if it was not running. + + """ + pass def __init__(self) -> None: """Auto-generated default constructor""" diff --git a/lib/python_bindings/module.cpp b/lib/python_bindings/module.cpp index cb57ec96d3..7088d0d526 100644 --- a/lib/python_bindings/module.cpp +++ b/lib/python_bindings/module.cpp @@ -1,5 +1,7 @@ #include +#include "stop_registry.h" + #define STRINGIFY(x) #x #define MACRO_STRINGIFY(x) STRINGIFY(x) @@ -43,4 +45,10 @@ PYBIND11_MODULE(_espp, m) { py_init_odrive_native(m); py_init_dispatcher(m); py_init_wdi(m); + + // Stop the threads of every live Task / Timer / TrajectoryPlanner / ... before the interpreter + // finalises (see stop_registry.h): their destructors run during finalisation, when a thread + // waiting for the GIL to run its python callback could never get it, so the join would hang. + py::module_::import("atexit").attr("register")( + py::cpp_function([]() { espp::py_detail::StopRegistry::instance().stop_all(); })); } diff --git a/lib/python_bindings/pybind_espp.cpp b/lib/python_bindings/pybind_espp.cpp index 38c1ab81e4..5fac4d6c73 100644 --- a/lib/python_bindings/pybind_espp.cpp +++ b/lib/python_bindings/pybind_espp.cpp @@ -13,6 +13,8 @@ #include "espp.hpp" +#include "stop_on_exit_holder.h" + namespace py = pybind11; // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! AUTOGENERATED CODE !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! @@ -346,8 +348,8 @@ void py_init_module_espp(py::module &m) { //////////////////// //////////////////// auto pyClassFtpServer = - py::class_(m, "FtpServer", py::dynamic_attr(), - "/ \\brief A class that implements a FTP server.") + py::class_>( + m, "FtpServer", py::dynamic_attr(), "/ \\brief A class that implements a FTP server.") .def(py::init(), py::arg("ip_address"), py::arg("port"), py::arg("root"), "/ \\brief A class that implements a FTP server.\n/ \note The IP Address is not " @@ -2147,7 +2149,7 @@ void py_init_module_espp(py::module &m) { //////////////////// //////////////////// //////////////////// //////////////////// - auto pyClassTask = py::class_( + auto pyClassTask = py::class_>( m, "Task", py::dynamic_attr(), "*\n * @brief Task provides an abstraction over std::thread which optionally\n * includes " "memory / priority configuration on ESP systems. It allows users to\n * easily stop the " @@ -2289,7 +2291,7 @@ void py_init_module_espp(py::module &m) { //////////////////// //////////////////// //////////////////// //////////////////// - auto pyClassTimer = py::class_( + auto pyClassTimer = py::class_>( m, "Timer", py::dynamic_attr(), "/ @brief A timer that can be used to schedule tasks to run at a later time.\n/ @details A " "timer can be used to schedule a task to run at a later time.\n/ The timer will run " @@ -2457,7 +2459,8 @@ void py_init_module_espp(py::module &m) { //////////////////// //////////////////// //////////////////// //////////////////// - auto pyClassTrajectoryPlanner = py::class_( + auto pyClassTrajectoryPlanner = py::class_< + espp::TrajectoryPlanner, espp::py_detail::StopOnExitHolder>( m, "TrajectoryPlanner", py::dynamic_attr(), "*\n * @brief Converts normalized joystick velocity commands into smooth,\n * " "dynamically feasible chassis motion commands (v, w).\n *\n * The planner is drive-system " @@ -2674,11 +2677,25 @@ void py_init_module_espp(py::module &m) { "* or before re-initialising with a new configuration.\n") .def("is_running", &espp::TrajectoryPlanner::is_running, "*\n * @brief Check whether the periodic update task is currently running.\n * " - "@return True if the task is running.\n"); + "@return True if the task is running.\n") + .def("start_task", &espp::TrajectoryPlanner::start_task, + "*\n * @brief Start the periodic planning and callback timers.\n *\n * The " + "planning " + "timer calls update() at Config::planning_period.\n * The callback task fires " + "output_callback whenever update() produces new output.\n * Has no effect if the " + "timers " + "are already running.\n *\n * @return True if both timers were started " + "successfully.\n", + py::call_guard()) + .def("stop_task", &espp::TrajectoryPlanner::stop_task, + "*\n * @brief Stop the periodic planning and callback timers (joins their tasks).\n " + "*\n * @return True if the task was stopped, False if it was not running.\n", + py::call_guard()); //////////////////// //////////////////// //////////////////// //////////////////// - auto pyClassThreadPool = py::class_( + auto pyClassThreadPool = py::class_>( m, "ThreadPool", py::dynamic_attr(), "*\n * @brief A thread pool that dispatches submitted jobs to a fixed set of worker " "threads.\n *\n * Workers are implemented as espp::Task instances. Jobs are queued and\n * " @@ -3836,7 +3853,8 @@ void py_init_module_espp(py::module &m) { //////////////////// //////////////////// //////////////////// //////////////////// - auto pyClassRtspServer = py::class_( + auto pyClassRtspServer = py::class_>( m, "RtspServer", py::dynamic_attr(), "/ Class for streaming MJPEG data from a camera using RTSP + RTP\n/ Starts a TCP socket to " "listen for RTSP connections, and then spawns off a\n/ new RTSP session for each " diff --git a/lib/python_bindings/stop_on_exit_holder.h b/lib/python_bindings/stop_on_exit_holder.h new file mode 100644 index 0000000000..3467d3ca41 --- /dev/null +++ b/lib/python_bindings/stop_on_exit_holder.h @@ -0,0 +1,98 @@ +#pragma once + +// StopOnExitHolder: the pybind11 holder type for espp classes whose +// destructor joins a thread (see stop_registry.h for the two deadlocks it +// prevents). It owns the object like std::unique_ptr, registers it with the +// StopRegistry while alive, and deletes it with the GIL released. +// +// The classes using it are listed in _STOP_ON_EXIT_CLASSES in +// lib/autogenerate_bindings.py, which rewrites their `py::class_` to +// `py::class_>`; every such +// class needs a stop_for_exit() overload below naming the call that stops its +// threads (idempotent, safe on an object that never started). + +#include + +#include "ftp_server.hpp" +#include "rtsp_server.hpp" +#include "task.hpp" +#include "thread_pool.hpp" +#include "timer.hpp" +#include "trajectory_planner.hpp" + +#include "stop_registry.h" + +namespace espp { +namespace py_detail { + +inline void stop_for_exit(espp::Task &t) { t.stop(); } +inline void stop_for_exit(espp::Timer &t) { t.cancel(); } +inline void stop_for_exit(espp::TrajectoryPlanner &p) { p.stop_task(); } +inline void stop_for_exit(espp::ThreadPool &p) { p.stop(); } +inline void stop_for_exit(espp::FtpServer &s) { s.stop(); } +inline void stop_for_exit(espp::RtspServer &s) { s.stop(); } + +template class StopOnExitHolder { +public: + StopOnExitHolder() = default; + explicit StopOnExitHolder(T *p) + : ptr_(p) { + if (ptr_) + StopRegistry::instance().add(ptr_, [ptr = ptr_]() { stop_for_exit(*ptr); }); + } + StopOnExitHolder(const StopOnExitHolder &) = delete; + StopOnExitHolder &operator=(const StopOnExitHolder &) = delete; + StopOnExitHolder(StopOnExitHolder &&other) noexcept + : ptr_(other.ptr_) { + other.ptr_ = nullptr; + } + StopOnExitHolder &operator=(StopOnExitHolder &&other) noexcept { + if (this != &other) { + reset(); + ptr_ = other.ptr_; + other.ptr_ = nullptr; + } + return *this; + } + ~StopOnExitHolder() { reset(); } + + T *get() const { return ptr_; } + T *operator->() const { return ptr_; } + T &operator*() const { return *ptr_; } + explicit operator bool() const { return ptr_ != nullptr; } + + /// Give up ownership without deleting (pybind11 uses this for + /// return_value_policy::take_ownership hand-overs). + T *release() { + T *p = ptr_; + if (p) + StopRegistry::instance().remove(p); + ptr_ = nullptr; + return p; + } + + /// Delete the object. The destructor joins the object's threads; a thread + /// that is waiting for the GIL to run its python callback can only finish if + /// the GIL is released here (pybind11 deletes holders with it held). + void reset() { + T *p = ptr_; + if (!p) + return; + ptr_ = nullptr; + StopRegistry::instance().remove(p); + if (PyGILState_Check()) { + pybind11::gil_scoped_release release; + delete p; + } else { + delete p; + } + } + +private: + T *ptr_ = nullptr; +}; + +} // namespace py_detail +} // namespace espp + +PYBIND11_DECLARE_HOLDER_TYPE(T, espp::py_detail::StopOnExitHolder) diff --git a/lib/python_bindings/stop_registry.h b/lib/python_bindings/stop_registry.h new file mode 100644 index 0000000000..8ef0e58f7e --- /dev/null +++ b/lib/python_bindings/stop_registry.h @@ -0,0 +1,79 @@ +#pragma once + +// Registry of live python-owned espp objects whose teardown joins a thread +// (Task, Timer, TrajectoryPlanner, ThreadPool, ...). Two problems it solves: +// +// 1. Deleting such an object from python runs its destructor with the GIL +// held. The destructor joins the object's task thread, which may at that +// moment be waiting for the GIL to invoke its python callback: deadlock. +// StopOnExitHolder (stop_on_exit_holder.h) is the pybind11 holder for these +// classes; it releases the GIL around `delete`. +// 2. At interpreter shutdown the same join happens while the interpreter is +// finalising, when a C++ thread can no longer re-acquire the GIL to run its +// callback (it blocks for good, or the process crashes). The holder +// registers every live object here, and the module's atexit hook calls +// stop_all() -- which stops each object's threads with the GIL released -- +// while python threads can still run, so the later destructors find nothing +// left to join. +// +// Standard library + pybind11 only (no espp headers), so module.cpp can +// register the atexit hook without pulling in the whole library. + +#include +#include +#include +#include +#include + +#include + +namespace espp { +namespace py_detail { + +class StopRegistry { +public: + static StopRegistry &instance() { + static StopRegistry registry; + return registry; + } + + /// Register a live object with the function that stops its threads. + void add(const void *key, std::function stop) { + std::lock_guard lock(mutex_); + entries_[key] = std::move(stop); + } + + /// Forget an object (its holder is deleting it). + void remove(const void *key) { + std::lock_guard lock(mutex_); + entries_.erase(key); + } + + /// Stop the threads of every registered object. Called from python's atexit + /// (GIL held): the GIL is released around the stops so a callback thread + /// waiting for it can finish its callback and exit. + void stop_all() { + std::vector> stops; + { + std::lock_guard lock(mutex_); + stops.reserve(entries_.size()); + for (auto &[key, stop] : entries_) + stops.push_back(stop); + } + pybind11::gil_scoped_release release; + for (auto &stop : stops) + stop(); + } + + size_t size() { + std::lock_guard lock(mutex_); + return entries_.size(); + } + +private: + std::mutex mutex_; + std::unordered_map> entries_; +}; + +} // namespace py_detail +} // namespace espp diff --git a/pyproject.toml b/pyproject.toml index 827898f02e..df43820186 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -127,8 +127,9 @@ skip = ["*-musllinux_*", "*_i686", "*-win32"] # Smoke-test the wheel: import + assert the RTPS RMI/AMI binding surface is # exposed (a functional in-process round-trip needs multicast, so it lives in the # dockerised interop harness / python/rtps_rpc_demo.py, not here), and run the -# self-contained stream_frame / Dispatcher binding test. -test-command = "python \"{project}/python/rtps_bindings_smoke.py\" && python \"{project}/python/dispatcher_test.py\"" +# self-contained stream_frame / Dispatcher binding test, and the blocking-destructor test +# (deleting / exiting with a running Task / Timer / TrajectoryPlanner must not deadlock). +test-command = "python \"{project}/python/rtps_bindings_smoke.py\" && python \"{project}/python/dispatcher_test.py\" && python \"{project}/python/blocking_destructors_test.py\"" [tool.cibuildwheel.linux] # The repo is mounted at /project inside the manylinux container; git must diff --git a/python/blocking_destructors_test.py b/python/blocking_destructors_test.py new file mode 100644 index 0000000000..7024404da2 --- /dev/null +++ b/python/blocking_destructors_test.py @@ -0,0 +1,153 @@ +"""Blocking-destructor test for the espp python bindings. + +Objects whose C++ destructor joins a thread (Task, Timer, TrajectoryPlanner, +ThreadPool) used to deadlock when deleted from python while their thread was +waiting for the GIL to run its python callback, and to hang or crash the +interpreter when still alive at exit. Their pybind11 holder now releases the +GIL around the delete and the module's atexit hook stops them before +finalisation (lib/python_bindings/stop_on_exit_holder.h). + +Each case runs in a child interpreter under a timeout, so a regression shows +up as a failed case rather than a hung test. + +Exit code 0 on full pass, 1 on any failure. +""" + +import os +import subprocess +import sys +import time + +TIMEOUT_S = 15.0 + +CASES = { + # delete a running Task whose callback is python: must return promptly + "task_del": """ +import time, espp +calls = [0] +def cb(): + calls[0] += 1 + time.sleep(0.005) + return False # keep running +t = espp.Task(cb, espp.Task.BaseConfig("py_task")) +t.start() +time.sleep(0.2) +t0 = time.time(); del t; dt = time.time() - t0 +assert calls[0] > 5, calls +assert dt < 2.0, dt +print("task_del ok", calls[0], round(dt, 3)) +""", + # the same through a Timer (its own task + a python callback) + "timer_del": """ +import time, espp +calls = [0] +def cb(): + calls[0] += 1 + time.sleep(0.003) + return False +t = espp.Timer(espp.Timer.Config("py_timer", 0.005, 0.0, cb, True)) +time.sleep(0.2) +t0 = time.time(); del t; dt = time.time() - t0 +assert calls[0] > 5, calls +assert dt < 2.0, dt +print("timer_del ok", calls[0], round(dt, 3)) +""", + # a running TrajectoryPlanner with a python output callback + "planner_del": """ +import time, espp +outputs = [0] +def on_output(cmd): + outputs[0] += 1 + time.sleep(0.003) +cfg = espp.TrajectoryPlanner.Config() +cfg.driving_profile.max_linear_acceleration = 1.0 +cfg.driving_profile.max_angular_acceleration = 1.0 +cfg.stopping_profile.max_linear_acceleration = 1.0 +cfg.stopping_profile.max_angular_acceleration = 1.0 +cfg.output_callback = on_output +cfg.planning_period = 0.005 +cfg.planning_task_config = espp.Task.BaseConfig("tp_plan") +cfg.callback_task_config = espp.Task.BaseConfig("tp_cb") +p = espp.TrajectoryPlanner(cfg) +p.set_target(1.0, 0.0) +time.sleep(0.2) +t0 = time.time(); del p; dt = time.time() - t0 +assert outputs[0] > 5, outputs +assert dt < 2.0, dt +print("planner_del ok", outputs[0], round(dt, 3)) +""", + # leave running objects alive at interpreter exit: the process must end cleanly + "exit_with_live_objects": """ +import time, espp +def cb(): + time.sleep(0.002) + return False +task = espp.Task(cb, espp.Task.BaseConfig("py_task")); task.start() +timer = espp.Timer(espp.Timer.Config("py_timer", 0.005, 0.0, cb, True)) +outputs = [0] +def on_output(cmd): + outputs[0] += 1 + time.sleep(0.003) +cfg = espp.TrajectoryPlanner.Config() +cfg.driving_profile.max_linear_acceleration = 1.0 +cfg.driving_profile.max_angular_acceleration = 1.0 +cfg.stopping_profile.max_linear_acceleration = 1.0 +cfg.stopping_profile.max_angular_acceleration = 1.0 +cfg.output_callback = on_output +cfg.planning_period = 0.005 +cfg.planning_task_config = espp.Task.BaseConfig("tp_plan") +cfg.callback_task_config = espp.Task.BaseConfig("tp_cb") +planner = espp.TrajectoryPlanner(cfg); planner.set_target(1.0, 0.0) +time.sleep(0.2) +print("exit_with_live_objects: leaving", outputs[0] > 0) +# no stop, no del: the interpreter exits with all three running +""", + # an exception with a live planner must not turn into a hang or a crash either + "exception_with_live_planner": """ +import time, espp +def on_output(cmd): + time.sleep(0.003) +cfg = espp.TrajectoryPlanner.Config() +cfg.driving_profile.max_linear_acceleration = 1.0 +cfg.driving_profile.max_angular_acceleration = 1.0 +cfg.stopping_profile.max_linear_acceleration = 1.0 +cfg.stopping_profile.max_angular_acceleration = 1.0 +cfg.output_callback = on_output +cfg.planning_period = 0.005 +cfg.planning_task_config = espp.Task.BaseConfig("tp_plan") +cfg.callback_task_config = espp.Task.BaseConfig("tp_cb") +planner = espp.TrajectoryPlanner(cfg); planner.set_target(1.0, 0.0) +time.sleep(0.1) +raise RuntimeError("expected: unhandled exception with a live planner") +""", +} +EXPECTED_EXIT = {"exception_with_live_planner": 1} + + +def run_case(name: str, code: str) -> bool: + t0 = time.time() + try: + proc = subprocess.run([sys.executable, "-u", "-c", code], capture_output=True, text=True, + timeout=TIMEOUT_S, env=os.environ.copy()) + except subprocess.TimeoutExpired as e: + print(f"FAIL {name}: timed out after {TIMEOUT_S:.0f} s (hang)\n{(e.stdout or b'')}") + return False + dt = time.time() - t0 + expected = EXPECTED_EXIT.get(name, 0) + if proc.returncode != expected: + sig = f" (signal {-proc.returncode})" if proc.returncode < 0 else "" + print(f"FAIL {name}: exit {proc.returncode}{sig}, expected {expected}\n{proc.stdout}{proc.stderr}") + return False + print(f"PASS {name} ({dt:.2f} s): {proc.stdout.strip().splitlines()[-1] if proc.stdout.strip() else ''}") + return True + + +def main() -> int: + results = [run_case(name, code) for name, code in CASES.items()] + passed = sum(results) + print(f"{passed}/{len(results)} cases passed") + return 0 if all(results) else 1 + + +if __name__ == "__main__": + sys.exit(main()) From e5ca83f764a81c8d30aa4b2bf069ae8084e8f5dd Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Tue, 20 Oct 2026 16:50:08 -0500 Subject: [PATCH 2/3] fix(trajectory_planner): never copy or destroy the output callback while holding the planner mutex The blocking-destructor test hung on CI (Linux cp311, macOS x86_64) and in roughly one run in four locally, and a stack sample showed why: the main thread held the GIL and waited for the planner's recursive_mutex inside set_target(), while the callback task held that mutex and waited for the GIL -- it copied the output_callback std::function under the lock, and copying (or destroying) a std::function that wraps a python callable takes the GIL in pybind11. A GIL / mutex lock-order inversion that predates this branch and hits any python user of the planner, not only the destructor paths. The callback is now held in a shared_ptr: the callback task snapshots the pointer under the mutex (no GIL) and calls it with the mutex released, and set_config() copies the new configuration before taking the mutex and lets the previous one die after releasing it. Observable behaviour is unchanged. The test's callback-count assertions only require that the callbacks ran at all (an emulated x86_64 CI runner manages a handful in the sampling window). Result: 0 hangs in 50 runs across Python 3.12 and 3.15, versus 7 of 23 before. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- .../include/trajectory_planner.hpp | 7 ++++- .../src/trajectory_planner.cpp | 26 +++++++++++++------ python/blocking_destructors_test.py | 6 ++--- 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/components/trajectory_planner/include/trajectory_planner.hpp b/components/trajectory_planner/include/trajectory_planner.hpp index 66b168adf6..bd3a8b72ea 100644 --- a/components/trajectory_planner/include/trajectory_planner.hpp +++ b/components/trajectory_planner/include/trajectory_planner.hpp @@ -259,7 +259,12 @@ Leave as nullptr to disable. */ State state_ = {}; float target_v_ = 0.0f; float target_w_ = 0.0f; - espp::TrajectoryPlanner::output_callback_t output_callback_ = nullptr; + /// The output callback, shared so the callback task can take a snapshot under + /// mutex_ without copying the std::function itself. Copying or destroying a + /// std::function that wraps a python callable takes the GIL (pybind11), and a + /// thread that holds the GIL may be waiting for mutex_ in set_target() / + /// output(): no GIL work may ever happen while mutex_ is held. + std::shared_ptr output_callback_; std::unique_ptr timer_ = nullptr; std::unique_ptr callback_task_ = nullptr; // woken by CV from update(), falls back to timeout diff --git a/components/trajectory_planner/src/trajectory_planner.cpp b/components/trajectory_planner/src/trajectory_planner.cpp index 824f5d8b1f..47d1359aa1 100644 --- a/components/trajectory_planner/src/trajectory_planner.cpp +++ b/components/trajectory_planner/src/trajectory_planner.cpp @@ -134,11 +134,19 @@ bool TrajectoryPlanner::set_config(const Config &config, bool reset_state) { } logger_.info("Updated config: {}", config); - std::lock_guard lk(mutex_); - config_ = config; - output_callback_ = config.output_callback; - if (reset_state) { - reset(); + // Copy the configuration (and its callback) before taking mutex_, and let the + // previous ones die after releasing it: copying or destroying a std::function + // that wraps a python callable takes the GIL, and a thread holding the GIL may + // be waiting for mutex_ (see output_callback_ in the header). + Config new_config = config; + auto new_callback = std::make_shared(config.output_callback); + { + std::lock_guard lk(mutex_); + std::swap(config_, new_config); + output_callback_.swap(new_callback); + if (reset_state) { + reset(); + } } return true; } @@ -295,15 +303,17 @@ bool TrajectoryPlanner::start_task() { return true; // stop the task callback_wake_flag_ = false; } - output_callback_t cb; + // snapshot the callback by shared_ptr (no std::function copy under mutex_, + // see output_callback_) and call it with the mutex released + std::shared_ptr cb; MotionCommand cmd; { std::lock_guard lk(mutex_); cb = output_callback_; cmd = MotionCommand{.linear_velocity = state_.v, .angular_velocity = state_.w}; } - if (cb) - cb(cmd); + if (cb && *cb) + (*cb)(cmd); return false; }, .task_config = config_.callback_task_config, diff --git a/python/blocking_destructors_test.py b/python/blocking_destructors_test.py index 7024404da2..af3791e184 100644 --- a/python/blocking_destructors_test.py +++ b/python/blocking_destructors_test.py @@ -33,7 +33,7 @@ def cb(): t.start() time.sleep(0.2) t0 = time.time(); del t; dt = time.time() - t0 -assert calls[0] > 5, calls +assert calls[0] > 0, calls # ran at all (slow emulated CI runners manage only a few) assert dt < 2.0, dt print("task_del ok", calls[0], round(dt, 3)) """, @@ -48,7 +48,7 @@ def cb(): t = espp.Timer(espp.Timer.Config("py_timer", 0.005, 0.0, cb, True)) time.sleep(0.2) t0 = time.time(); del t; dt = time.time() - t0 -assert calls[0] > 5, calls +assert calls[0] > 0, calls # ran at all (slow emulated CI runners manage only a few) assert dt < 2.0, dt print("timer_del ok", calls[0], round(dt, 3)) """, @@ -72,7 +72,7 @@ def on_output(cmd): p.set_target(1.0, 0.0) time.sleep(0.2) t0 = time.time(); del p; dt = time.time() - t0 -assert outputs[0] > 5, outputs +assert outputs[0] > 0, outputs assert dt < 2.0, dt print("planner_del ok", outputs[0], round(dt, 3)) """, From d236ba1af94486e1fe9c300c83735fff6d272c0a Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Tue, 20 Oct 2026 17:06:28 -0500 Subject: [PATCH 3/3] fix(python): serialise stop_all() against a concurrent delete; document start_task / stop_task Self-review of the holder: stop_all() copied raw stop functions and ran them with the GIL released, so a daemon python thread deleting one of those objects at the same moment would have been a use-after-free. Each registry entry now carries its own mutex and an alive flag: the holder takes it (after releasing the GIL, so no GIL / mutex inversion) around the delete, stop_all() takes it around each stop and skips dead entries. A stop function that throws is ignored at exit. The planner README lists the now-public start_task() / stop_task(). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU --- components/trajectory_planner/README.md | 4 ++ lib/python_bindings/stop_on_exit_holder.h | 33 ++++++++++---- lib/python_bindings/stop_registry.h | 55 +++++++++++++++++------ 3 files changed, 71 insertions(+), 21 deletions(-) diff --git a/components/trajectory_planner/README.md b/components/trajectory_planner/README.md index 5b667cce2d..64d6fc4539 100644 --- a/components/trajectory_planner/README.md +++ b/components/trajectory_planner/README.md @@ -40,6 +40,10 @@ constraints. Safe to call `set_target()` and `output()` from within it. - **Thread-safe** - `set_target()`, `get_target()`, `output()`, `stop()`, and `reset()` are safe to call from any thread concurrently. +- **Pause / resume** - `stop_task()` stops the planning and callback tasks (the + destructor does the same) and `start_task()` starts them again; `is_running()` + reports the state. From Python, `stop_task()` is the clean way to shut a + planner down before dropping it. ## Usage diff --git a/lib/python_bindings/stop_on_exit_holder.h b/lib/python_bindings/stop_on_exit_holder.h index 3467d3ca41..19341e4094 100644 --- a/lib/python_bindings/stop_on_exit_holder.h +++ b/lib/python_bindings/stop_on_exit_holder.h @@ -11,6 +11,8 @@ // class needs a stop_for_exit() overload below naming the call that stops its // threads (idempotent, safe on an object that never started). +#include + #include #include "ftp_server.hpp" @@ -61,30 +63,45 @@ template class StopOnExitHolder { T &operator*() const { return *ptr_; } explicit operator bool() const { return ptr_ != nullptr; } - /// Give up ownership without deleting (pybind11 uses this for - /// return_value_policy::take_ownership hand-overs). + /// Give up ownership without deleting. T *release() { T *p = ptr_; - if (p) - StopRegistry::instance().remove(p); + if (p) { + if (auto entry = StopRegistry::instance().take(p)) { + std::lock_guard lock(entry->mutex); + entry->alive = false; + } + } ptr_ = nullptr; return p; } /// Delete the object. The destructor joins the object's threads; a thread /// that is waiting for the GIL to run its python callback can only finish if - /// the GIL is released here (pybind11 deletes holders with it held). + /// the GIL is released here (pybind11 deletes holders with it held). The + /// delete is serialised against a concurrent StopRegistry::stop_all() through + /// the entry's mutex, taken only after the GIL is released (stop_all() holds + /// that mutex while it joins threads that may need the GIL). void reset() { T *p = ptr_; if (!p) return; ptr_ = nullptr; - StopRegistry::instance().remove(p); + auto entry = StopRegistry::instance().take(p); + auto destroy = [&]() { + if (entry) { + std::lock_guard lock(entry->mutex); + delete p; + entry->alive = false; + } else { + delete p; + } + }; if (PyGILState_Check()) { pybind11::gil_scoped_release release; - delete p; + destroy(); } else { - delete p; + destroy(); } } diff --git a/lib/python_bindings/stop_registry.h b/lib/python_bindings/stop_registry.h index 8ef0e58f7e..ac7c5e759c 100644 --- a/lib/python_bindings/stop_registry.h +++ b/lib/python_bindings/stop_registry.h @@ -20,6 +20,7 @@ // register the atexit hook without pulling in the whole library. #include +#include #include #include #include @@ -30,6 +31,17 @@ namespace espp { namespace py_detail { +/// One registered object. Its mutex serialises "stop it" against "delete it", so +/// stop_all() can never run a stop function on an object another python thread +/// is deleting at the same moment: the holder takes the entry's mutex (with the +/// GIL released, see StopOnExitHolder::reset) around the delete and clears +/// `alive`; stop_all() takes it around each stop and skips dead entries. +struct StopEntry { + std::mutex mutex; + std::function stop; + bool alive = true; +}; + class StopRegistry { public: static StopRegistry &instance() { @@ -38,31 +50,48 @@ class StopRegistry { } /// Register a live object with the function that stops its threads. - void add(const void *key, std::function stop) { + std::shared_ptr add(const void *key, std::function stop) { + auto entry = std::make_shared(); + entry->stop = std::move(stop); std::lock_guard lock(mutex_); - entries_[key] = std::move(stop); + entries_[key] = entry; + return entry; } - /// Forget an object (its holder is deleting it). - void remove(const void *key) { + /// Forget an object (its holder is about to delete it); returns its entry so + /// the holder can serialise the delete against a concurrent stop_all(). + std::shared_ptr take(const void *key) { std::lock_guard lock(mutex_); - entries_.erase(key); + auto it = entries_.find(key); + if (it == entries_.end()) + return nullptr; + auto entry = std::move(it->second); + entries_.erase(it); + return entry; } /// Stop the threads of every registered object. Called from python's atexit /// (GIL held): the GIL is released around the stops so a callback thread - /// waiting for it can finish its callback and exit. + /// waiting for it can finish its callback and exit. A stop function that + /// throws is ignored (nothing sensible can be done with it at exit). void stop_all() { - std::vector> stops; + std::vector> entries; { std::lock_guard lock(mutex_); - stops.reserve(entries_.size()); - for (auto &[key, stop] : entries_) - stops.push_back(stop); + entries.reserve(entries_.size()); + for (auto &[key, entry] : entries_) + entries.push_back(entry); } pybind11::gil_scoped_release release; - for (auto &stop : stops) - stop(); + for (auto &entry : entries) { + std::lock_guard lock(entry->mutex); + if (!entry->alive) + continue; + try { + entry->stop(); + } catch (...) { + } + } } size_t size() { @@ -72,7 +101,7 @@ class StopRegistry { private: std::mutex mutex_; - std::unordered_map> entries_; + std::unordered_map> entries_; }; } // namespace py_detail