Skip to content

Tools: ros2: tear down rclpy cleanly in the AP_DDS tests - #33894

Open
peterbarker wants to merge 1 commit into
ArduPilot:masterfrom
peterbarker:pr-claude2/dds-test-teardown
Open

Tools: ros2: tear down rclpy cleanly in the AP_DDS tests#33894
peterbarker wants to merge 1 commit into
ArduPilot:masterfrom
peterbarker:pr-claude2/dds-test-teardown

Conversation

@peterbarker

Copy link
Copy Markdown
Contributor

Summary

Fixes segfault recently starting in colcon tests

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

Description

Every test in ardupilot_dds_tests tore down with nothing but rclpy.shutdown(), which shuts the context down while that context's node, subscriptions and DDS participant are all still alive, and left its rclpy.spin() thread unjoined. colcon runs the whole package in one interpreter, so those zombie endpoints accumulated from test to test.

They race with Fast-DDS intra-process delivery, and the process intermittently died with SIGSEGV on an internal DDS thread:

History::get_change_nts
StatefulReader::NotifyChanges
StatefulReader::change_received
StatefulReader::processDataMsg
StatefulWriter::deliver_sample_to_intraprocesses
FlowControllerImpl<...>::run

Both CI occurrences were in the 22nd of the 23 tests, i.e. at maximum accumulation, and killed the whole pytest run with exit code -11.

rclpy.spin() cannot be torn down cleanly: it uses a process-wide global executor bound to whichever context was current when the first test span, and the only way to make it return is to shut the context down. So spin on a private SingleThreadedExecutor via a ros_node() context manager, which shuts the executor down, joins the spin thread and destroys the node while the context is still valid.

Reproduced by repeating the time tests in one pytest process (pytest --count=40). Over 6 shards x 4 loops, with identical harness and only this change differing: 4 of 24 loops segfaulted before, 0 of 24 after, with all 80 tests passing in every loop.

Every test in ardupilot_dds_tests tore down with nothing but
rclpy.shutdown(), which shuts the context down while that context's node,
subscriptions and DDS participant are all still alive, and left its
rclpy.spin() thread unjoined.  colcon runs the whole package in one
interpreter, so those zombie endpoints accumulated from test to test.

They race with Fast-DDS intra-process delivery, and the process
intermittently died with SIGSEGV on an internal DDS thread:

  History::get_change_nts
  StatefulReader::NotifyChanges
  StatefulReader::change_received
  StatefulReader::processDataMsg
  StatefulWriter::deliver_sample_to_intraprocesses
  FlowControllerImpl<...>::run

Both CI occurrences were in the 22nd of the 23 tests, i.e. at maximum
accumulation, and killed the whole pytest run with exit code -11.

rclpy.spin() cannot be torn down cleanly: it uses a process-wide global
executor bound to whichever context was current when the first test span,
and the only way to make it return is to shut the context down.  So spin
on a private SingleThreadedExecutor via a ros_node() context manager,
which shuts the executor down, joins the spin thread and destroys the
node while the context is still valid.

Reproduced by repeating the time tests in one pytest process
(pytest --count=40).  Over 6 shards x 4 loops, with identical harness and
only this change differing: 4 of 24 loops segfaulted before, 0 of 24
after, with all 80 tests passing in every loop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@peterbarker
peterbarker force-pushed the pr-claude2/dds-test-teardown branch from 80ac7ee to 8152eb1 Compare July 31, 2026 04:05
@khancyr khancyr mentioned this pull request Jul 31, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant