diff --git a/docs/source/Learn/bskPrinciples/bskPrinciples-5.rst b/docs/source/Learn/bskPrinciples/bskPrinciples-5.rst index 5b4bc21db51..b71d2479153 100644 --- a/docs/source/Learn/bskPrinciples/bskPrinciples-5.rst +++ b/docs/source/Learn/bskPrinciples/bskPrinciples-5.rst @@ -63,121 +63,54 @@ Next, the simulation stop time is extended for an additional 10s to 20s total an Retaining Message Objects in Memory ----------------------------------- -When creating stand-alone message objects in Python, it's crucial to understand that these objects must be retained in memory to function properly. If a message object is created locally within a function or method and not stored in a persistent variable, Python's garbage collector may remove it from memory once the function exits, even if C++ components still need to access it. +Before Basilisk 2.12, a stand-alone message created inside a Python helper had to +be stored in a persistent variable. Otherwise, Python could garbage-collect the +message after the helper returned while a module still held pointers to its data. -This is particularly important when: +Starting with Basilisk 2.12, object-based message subscriptions automatically +retain their source for the lifetime of the subscription. A stand-alone message +may therefore leave Python scope safely while an input message remains subscribed +to it. This applies whether the input reader is embedded in a C or C++ module. If +the source is embedded in another module or C-module config, Basilisk retains that +owning object rather than the temporary Python message proxy. -1. Creating message objects in subroutines or helper functions -2. Dynamically generating messages based on runtime conditions -3. Setting up message interfaces that will be used throughout simulation +Message recorders follow the same lifetime rule. A recorder created from a +stand-alone or module-embedded message retains the object that owns the message +storage until the recorder is released. -**Example: Using a Class-Level Registry to Retain Objects** - -A common pattern in Basilisk to ensure message objects remain in memory is to use a -class-level registry or list that persists for the lifetime of your simulation. -Here's how this is implemented in Basilisk's own code: - -.. code-block:: python - - class BskSimulation: - def __init__(self): - # Create class-level registry if it doesn't exist - if not hasattr(self, '_message_registry'): - self._message_registry = [] - - def setup_sensors(self): - """Set up sensor messages and devices.""" - - # Create a message - sensor_msg = messaging.SensorMsg() - - # Store message in class-level registry to prevent garbage collection - self._message_registry.append(sensor_msg) - - # Create the sensor module that will use this message - self.sensor_module = sensorModule.SensorModule() - self.sensor_module.sensorOutMsg.subscribeTo(sensor_msg) - - return - - -This pattern is used in Basilisk's own implementation for components like Coarse -Sun Sensors (CSS): +For example, this helper does not need to return or otherwise retain ``inputMsg``: .. code-block:: python - def SetCSSConstellation(self): - """Set the CSS sensors""" - self.CSSConstellationObject.ModelTag = "cssConstellation" - - # Create class-level registry if it doesn't exist - if not hasattr(self, '_css_registry'): - self._css_registry = [] + def connectInput(module): + """Create and connect a stand-alone input message.""" + payload = messaging.CModuleTemplateMsgPayload(dataVector=[1.0, 2.0, 3.0]) + inputMsg = messaging.CModuleTemplateMsg().write(payload) + module.dataInMsg.subscribeTo(inputMsg) - def setupCSS(cssDevice): - cssDevice = coarseSunSensor.CoarseSunSensor() - cssDevice.fov = 80. * mc.D2R - cssDevice.scaleFactor = 2.0 - cssDevice.sunInMsg.subscribeTo(self.gravFactory.spiceObject.planetStateOutMsgs[self.sun]) - cssDevice.stateInMsg.subscribeTo(self.scObject.scStateOutMsg) - # Store CSS in class-level registry to prevent garbage collection - self._css_registry.append(cssDevice) - # Create CSS devices and add them to the registry... - -**Alternative Approach: Using a Dictionary Registry** - -For more complex simulations where you need to retrieve specific messages later, -you can use a dictionary-based registry: +Keeping an explicit Python reference remains valid and can make ownership clearer. +It is also useful when the caller needs to write new data, inspect the message, +unsubscribe and reconnect it, or otherwise access it later: .. code-block:: python class BskSimulation: - """A registry to keep message objects alive in Python memory.""" - def __init__(self): - self.messages = {} - - def make_message(self, name): - """Make a message object with a unique name.""" - msg_obj = messaging.SensorMsg() - self.messages[name] = msg_obj - return msg_obj - - def get_message(self, name): - """Retrieve a message object by name.""" - return self.messages.get(name) - -**Have Parent Method Retain the Msg Object in memory** - -If the message setup method returns in instance of the message, then the parent method -could be responsible for retaining this message object in memory - -.. code-block:: python - - class BskSimulation: - """A registry to keep message objects alive in Python memory.""" - - def make_message(self): - """Make a message object and return it.""" - msg = messaging.SensorMsg() - msg.fov = 2.0 - return msg - - def parent_method(self, name): - """Retrieve a message object by name.""" - self.sensorMsg = self.make_message() - return - + self.inputMsg = messaging.CModuleTemplateMsg() -**Common Pitfalls** + def connectInput(self, module, payload): + """Write and connect the retained input message.""" + self.inputMsg.write(payload) + module.dataInMsg.subscribeTo(self.inputMsg) -Without proper retention, you might encounter issues like: -- Messages that appear to be properly connected but don't transmit data -- Simulation components that can't communicate as expected -- Mysterious segmentation faults or access violations in C++ code +The automatic lifetime guarantee has the following boundaries: -By using a registry pattern or ensuring message objects are stored in long-lived -variables, you can avoid these issues and create more modular, maintainable -simulation code. +* Calling ``unsubscribe()`` releases the subscription's reference. Retain the + message explicitly if it must be available for a later reconnection. +* Subscribing by a raw integer address is caller-owned and does not retain a + Python source object. +* This guarantee applies to message sources connected through ``subscribeTo()``. + Modules, sensors, effectors, and other wrapped simulation objects can have + separate ownership requirements and may still need persistent Python references. diff --git a/docs/source/Support/bskReleaseNotesSnippets/1433-cmsg-reader-keepalive.rst b/docs/source/Support/bskReleaseNotesSnippets/1433-cmsg-reader-keepalive.rst new file mode 100644 index 00000000000..39f859d82c9 --- /dev/null +++ b/docs/source/Support/bskReleaseNotesSnippets/1433-cmsg-reader-keepalive.rst @@ -0,0 +1 @@ +- Fixed use-after-free paths involving Python-owned C messages. A C module input message (a ``Msg_C`` reader) now keeps its subscribed source alive for as long as the subscription is live, and a recorder created from a ``Msg_C`` keeps the stand-alone message or embedded-message owner alive for the recorder's lifetime. Subscription references are released on ``unsubscribe()``, on re-subscribe, or when the subscriber owner is garbage-collected; recorder references are released when the recorder is garbage-collected. This completes the message keep-alive started for the C++ reader and recorder direction in issue #676. diff --git a/docs/source/conf.py b/docs/source/conf.py index ca151456265..f3f3a364c2e 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -415,6 +415,7 @@ def grabRelevantFiles(self,dir_path): removeList = [] for i in range(len(files_in_dir)): if "__init__" in files_in_dir[i] or \ + (files_in_dir[i].endswith(".py") and os.path.basename(files_in_dir[i]).startswith("_")) or \ "conftest.py" in files_in_dir[i] or \ "*.xml" in files_in_dir[i] or \ "vizMessage.pb.cc" in files_in_dir[i] or \ diff --git a/examples/scenarioFlexiblePanel.py b/examples/scenarioFlexiblePanel.py index 310d82ace9a..b6c343ad041 100644 --- a/examples/scenarioFlexiblePanel.py +++ b/examples/scenarioFlexiblePanel.py @@ -131,12 +131,6 @@ def createSimBaseClass(): fswTimeStep = macros.sec2nano(0.5) scSim.fswProcess.addTask(scSim.CreateNewTask(scSim.fswTaskName, fswTimeStep)) - # Keep standalone reference messages alive for the run. They are created in - # setup helpers that return before the simulation runs; without a Python - # reference their C++ backing is garbage collected and the subscriber reads a - # dead message (a fallout of the .disown() removal in #918). - scSim.refMessages = [] - return scSim @@ -430,7 +424,6 @@ def setUpControl(scSim, extFTObject, attError, scGeometry): mrpControl.guidInMsg.subscribeTo(attError.attGuidOutMsg) mrpControl.vehConfigInMsg.subscribeTo(configDataMsg) - scSim.refMessages.append(configDataMsg) extFTObject.cmdTorqueInMsg.subscribeTo(mrpControl.cmdTorqueOutMsg) diff --git a/src/architecture/_GeneralModuleFiles/swig_c_wrap.i b/src/architecture/_GeneralModuleFiles/swig_c_wrap.i index 78a22aa2e90..36a52305685 100644 --- a/src/architecture/_GeneralModuleFiles/swig_c_wrap.i +++ b/src/architecture/_GeneralModuleFiles/swig_c_wrap.i @@ -101,11 +101,22 @@ class CWrapper : public SysModel { template inline void Reset_ ## functionSuffix(T, uint64_t, int64_t) {} %} - // The constructor CWrapper(TConfig* config) takes ownership of the given pointer - // We don't want the Python object for this config to also think it owns the memory + // Config and CWrapper constructors register embedded Msg_C storage owners. + // When a wrapper takes a config pointer, transferModuleOwner moves existing + // keep-alives to the wrapper. Keep these pythonappend bodies free of apostrophes + // and hash comment lines because SWIG macro expansion mis-parses them. + %pythonappend configName::configName() %{ + from Basilisk.architecture.messaging import _msgKeepAlive + _msgKeepAlive.registerModule(self) + %} + %pythonappend CWrapper::CWrapper %{ + from Basilisk.architecture.messaging import _msgKeepAlive if (len(args)) > 0: args[0].thisown = False + _msgKeepAlive.transferModuleOwner(args[0], self) + else: + _msgKeepAlive.registerModule(self) %} %include "moduleName.h" diff --git a/src/architecture/messaging/CMakeLists.txt b/src/architecture/messaging/CMakeLists.txt index 2d04c9551ad..12c1d26a580 100644 --- a/src/architecture/messaging/CMakeLists.txt +++ b/src/architecture/messaging/CMakeLists.txt @@ -110,6 +110,20 @@ endfunction(generate_messages) # TODO: Deprecate this! configure_file(${CMAKE_CURRENT_SOURCE_DIR}/cMsgCInterfacePy/__init__.py ${CMAKE_BINARY_DIR}/Basilisk/architecture/cMsgCInterfacePy/__init__.py COPYONLY) +# issue #1433: Python-side keep-alive helpers for Msg_C subscribers. Copy at +# build time so this helper does not make the messaging package directory exist +# during configure, before generatePackageInit.py writes messaging/__init__.py. +set(MSG_KEEP_ALIVE_PY "${CMAKE_BINARY_DIR}/Basilisk/architecture/messaging/_msgKeepAlive.py") +add_custom_command( + OUTPUT ${MSG_KEEP_ALIVE_PY} + COMMAND ${CMAKE_COMMAND} -E make_directory "${CMAKE_BINARY_DIR}/Basilisk/architecture/messaging" + COMMAND ${CMAKE_COMMAND} -E copy_if_different + "${CMAKE_CURRENT_SOURCE_DIR}/_msgKeepAlive.py" + "${MSG_KEEP_ALIVE_PY}" + DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/_msgKeepAlive.py + VERBATIM) +add_custom_target(msgKeepAlivePy DEPENDS ${MSG_KEEP_ALIVE_PY}) + # Track payload headers so the generated Python message package is rebuilt when message definitions change. file(GLOB_RECURSE package_init_payload_files CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/../msgPayloadDefC/*Payload.h" @@ -142,6 +156,7 @@ endif(NOT "${EXTERNAL_MODULES_PATH}" STREQUAL "") # Custom target for establishing dependency add_custom_target(swigtrick DEPENDS ${CMAKE_BINARY_DIR}/Basilisk/architecture/messaging/__init__.py) +add_dependencies(swigtrick msgKeepAlivePy) # Generate per-payload equality headers. Each "_equality.h" is included # only by that payload's own SWIG module. diff --git a/src/architecture/messaging/_UnitTest/test_CMsgRecorderReadsLiveMessage.py b/src/architecture/messaging/_UnitTest/test_CMsgRecorderReadsLiveMessage.py index 080328c0fb7..95d3d257d71 100644 --- a/src/architecture/messaging/_UnitTest/test_CMsgRecorderReadsLiveMessage.py +++ b/src/architecture/messaging/_UnitTest/test_CMsgRecorderReadsLiveMessage.py @@ -24,7 +24,7 @@ # """ -Regression test for issue #338. +Regression tests for issues #338 and #1433. A recorder attached to a C module's C output message must read the module's live message data, not a stale snapshot. The C-message read path reaches the @@ -32,10 +32,17 @@ test verifies end-to-end that the recorded payload tracks the values the module actually writes over time (and is not constant, which would indicate the recorder is bound to the wrong / a copied address). + +The recorder also stores raw pointers into its ``Msg_C`` source. It must retain +the object that owns that storage until the recorder is collected, including +when config-owned storage transfers into a C-module wrapper. """ +import gc +import weakref + import numpy as np -from Basilisk.architecture import bskLogging +from Basilisk.architecture import bskLogging, messaging from Basilisk.moduleTemplates import cModuleTemplate from Basilisk.utilities import SimulationBaseClass, macros @@ -75,5 +82,132 @@ def test_cMsgRecorderReadsLiveMessage(): "recorded C message is constant; recorder is not reading the live message" +def test_standaloneCMsgRecorderKeepsSourceAlive(): + """A recorder retains its stand-alone ``Msg_C`` source until collection.""" + inputVector = [1.0, 2.0, 3.0] # [-] + payload = messaging.CModuleTemplateMsgPayload(dataVector=inputVector) + source = messaging.CModuleTemplateMsg_C().write(payload) + sourceReference = weakref.ref(source) + recorder = source.recorder() + + del source + gc.collect() + + assert sourceReference() is not None + recorder.UpdateState(0) + assert list(recorder.dataVector[0]) == inputVector + + del recorder + gc.collect() + + assert sourceReference() is None + + +def test_copiedCMsgRecorderKeepsSourceAlive(): + """A copied recorder inherits the original recorder's source retention.""" + emptyRecorder = messaging.CModuleTemplateMsgRecorder() + assert emptyRecorder.size() == 0 + + inputVector = [10.0, 11.0, 12.0] # [-] + source = messaging.CModuleTemplateMsg_C().write( + messaging.CModuleTemplateMsgPayload(dataVector=inputVector) + ) + sourceReference = weakref.ref(source) + originalRecorder = source.recorder() + copiedRecorder = type(originalRecorder)(originalRecorder) + + del source, originalRecorder + gc.collect() + + assert sourceReference() is not None + copiedRecorder.UpdateState(0) + assert list(copiedRecorder.dataVector[0]) == inputVector + + del copiedRecorder + gc.collect() + + assert sourceReference() is None + + +def test_directCMsgRecorderConstructorKeepsSourceAlive(): + """The public recorder constructor retains a direct ``Msg_C`` argument.""" + inputVector = [13.0, 14.0, 15.0] # [-] + source = messaging.CModuleTemplateMsg_C().write( + messaging.CModuleTemplateMsgPayload(dataVector=inputVector) + ) + sourceReference = weakref.ref(source) + recorder = messaging.CModuleTemplateMsgRecorder(source, 0) + + del source + gc.collect() + + assert sourceReference() is not None + recorder.UpdateState(0) + assert list(recorder.dataVector[0]) == inputVector + + del recorder + gc.collect() + + assert sourceReference() is None + + +def test_embeddedCMsgRecorderKeepsModuleOwnerAlive(): + """A recorder retains the module that owns an embedded ``Msg_C`` source.""" + inputVector = [4.0, 5.0, 6.0] # [-] + module = cModuleTemplate.cModuleTemplate() + module.dataOutMsg.write( + messaging.CModuleTemplateMsgPayload(dataVector=inputVector) + ) + + source = module.dataOutMsg + moduleReference = weakref.ref(module) + recorder = source.recorder() + + del module, source + gc.collect() + + assert moduleReference() is not None + recorder.UpdateState(0) + assert list(recorder.dataVector[0]) == inputVector + + del recorder + gc.collect() + + assert moduleReference() is None + + +def test_configCMsgRecorderLeaseTransfersToWrapper(): + """A recorder lease follows config-owned storage into its C-module wrapper.""" + inputVector = [7.0, 8.0, 9.0] # [-] + config = cModuleTemplate.cModuleTemplateConfig() + config.dataOutMsg.write( + messaging.CModuleTemplateMsgPayload(dataVector=inputVector) + ) + + source = config.dataOutMsg + configReference = weakref.ref(config) + recorder = source.recorder() + module = config.createWrapper() + moduleReference = weakref.ref(module) + + del config, module, source + gc.collect() + + assert configReference() is None + assert moduleReference() is not None + recorder.UpdateState(0) + assert list(recorder.dataVector[0]) == inputVector + + del recorder + gc.collect() + + assert moduleReference() is None + + if __name__ == "__main__": test_cMsgRecorderReadsLiveMessage() + test_standaloneCMsgRecorderKeepsSourceAlive() + test_copiedCMsgRecorderKeepsSourceAlive() + test_directCMsgRecorderConstructorKeepsSourceAlive() + test_embeddedCMsgRecorderKeepsModuleOwnerAlive() + test_configCMsgRecorderLeaseTransfersToWrapper() diff --git a/src/architecture/messaging/_UnitTest/test_cMsgReaderKeepAlive.py b/src/architecture/messaging/_UnitTest/test_cMsgReaderKeepAlive.py new file mode 100644 index 00000000000..ea355871a03 --- /dev/null +++ b/src/architecture/messaging/_UnitTest/test_cMsgReaderKeepAlive.py @@ -0,0 +1,336 @@ +# +# ISC License +# +# Copyright (c) 2026, Autonomous Vehicle Systems Lab, University of Colorado at Boulder +# +# Permission to use, copy, modify, and/or distribute this software for any +# purpose with or without fee is hereby granted, provided that the above +# copyright notice and this permission notice appear in all copies. +# +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN +# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF +# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +# + +# +# Unit Test Script +# Module Name: messaging (Msg_C reader keep-alive) +# Author: robotrocketscience (https://github.com/robotrocketscience) +# Creation Date: June 25, 2026 +# + +""" +Regression test for issue #1433 (follow-up to #676 / #1432). + +A ``Msg_C`` reader (e.g. a C module's ``dataInMsg``) that subscribes to a Python +source stores raw pointers into that source's memory. If the only Python reference +to the source then goes out of scope, the source must *not* be garbage-collected +while the subscription is live, otherwise the reader points into freed memory. The +keep-alive added for #1433 retains the source owner and releases it again on +``unsubscribe()``, on re-subscribe, when a stand-alone subscriber proxy is +collected, and when the C module that embeds the subscriber is collected. + +These tests assert the lifetime contract directly with ``weakref`` (deterministic, +no reliance on memory being clobbered) and additionally check that a module reads +the correct, live data end-to-end after a garbage-collection pass. +""" + +import gc +import weakref + +import numpy as np +from Basilisk.architecture import bskLogging, messaging +from Basilisk.moduleTemplates import cModuleTemplate +from Basilisk.utilities import SimulationBaseClass, macros + + +def _cppSourceInScope(reader, vec): + """Subscribe `reader` (a Msg_C) to a C++ Message source created in this scope, + then let the only Python reference to the source fall out of scope on return. + Returns a weakref to the source so the caller can observe its lifetime.""" + payload = messaging.CModuleTemplateMsgPayload() + payload.dataVector = vec + src = messaging.CModuleTemplateMsg().write(payload) + reader.subscribeTo(src) + return weakref.ref(src) + + +def _cMsgSourceInScope(reader, vec): + """Same as `_cppSourceInScope` but the source is a stand-alone Msg_C.""" + payload = messaging.CModuleTemplateMsgPayload() + payload.dataVector = vec + src = messaging.CModuleTemplateMsg_C() + src.write(payload) + reader.subscribeTo(src) + return weakref.ref(src) + + +def test_moduleEmbeddedCppSourceSurvivesGC(): + """A C module's Msg_C reader keeps a C++ Message source alive across GC, and + reads its live data end-to-end.""" + bskLogging.setDefaultLogLevel(bskLogging.BSK_WARNING) + + scSim = SimulationBaseClass.SimBaseClass() + proc = scSim.CreateNewProcess("p") + proc.addTask(scSim.CreateNewTask("t", macros.sec2nano(1.0))) + + mod = cModuleTemplate.cModuleTemplate() + mod.ModelTag = "cMod" + scSim.AddModelToTask("t", mod) + + vec = [10.0, 20.0, 30.0] + wr = _cppSourceInScope(mod.dataInMsg, vec) + gc.collect() + assert wr() is not None, "source was garbage-collected despite an active subscription (#1433)" + + rec = mod.dataInMsg.recorder() + scSim.AddModelToTask("t", rec) + scSim.InitializeSimulation() + scSim.ConfigureStopTime(macros.sec2nano(3.0)) + scSim.ExecuteSimulation() + + recorded = np.array(rec.dataVector) + np.testing.assert_allclose( + recorded, [vec] * len(recorded), atol=1e-9, + err_msg="C module read garbage from a GC'd subscribed source (#1433)", + ) + + +def test_moduleEmbeddedCMsgSourceSurvivesGC(): + """A C module's Msg_C reader keeps a stand-alone Msg_C source alive across GC.""" + mod = cModuleTemplate.cModuleTemplate() + wr = _cMsgSourceInScope(mod.dataInMsg, [40.0, 50.0, 60.0]) + gc.collect() + assert wr() is not None, "Msg_C source was garbage-collected despite a subscription (#1433)" + + +def test_moduleEmbeddedCMsgSourceOwnerSurvivesGC(): + """A C module's Msg_C reader keeps the owning module alive when subscribing + to another module's embedded Msg_C source.""" + consumer = cModuleTemplate.cModuleTemplate() + producer = cModuleTemplate.cModuleTemplate() + + payload = messaging.CModuleTemplateMsgPayload() + payload.dataVector = [14.0, 15.0, 16.0] + producer.dataOutMsg.write(payload) + + source = producer.dataOutMsg + producer_ref = weakref.ref(producer) + source_ref = weakref.ref(source) + + consumer.dataInMsg.subscribeTo(source) + del producer, source + gc.collect() + + assert producer_ref() is not None, "embedded Msg_C source owner was not retained (#1433)" + assert source_ref() is None, "embedded Msg_C proxy was retained instead of its owner (#1433)" + assert list(consumer.dataInMsg.read().dataVector) == [14.0, 15.0, 16.0] + + consumer.dataInMsg.unsubscribe() + gc.collect() + assert producer_ref() is None, "unsubscribe() did not release embedded Msg_C source owner (#1433)" + + +def test_unsubscribeReleasesKeepAlive(): + """unsubscribe() drops the keep-alive so the source can be collected.""" + mod = cModuleTemplate.cModuleTemplate() + wr = _cppSourceInScope(mod.dataInMsg, [4.0, 5.0, 6.0]) + gc.collect() + assert wr() is not None + + mod.dataInMsg.unsubscribe() + gc.collect() + assert wr() is None, "unsubscribe() did not release the keep-alive (#1433)" + + +def test_resubscribeReplacesKeepAlive(): + """Re-subscribing releases the previous source and retains the new one.""" + mod = cModuleTemplate.cModuleTemplate() + wrA = _cppSourceInScope(mod.dataInMsg, [1.0, 1.0, 1.0]) + gc.collect() + assert wrA() is not None + + wrB = _cppSourceInScope(mod.dataInMsg, [2.0, 2.0, 2.0]) + gc.collect() + assert wrA() is None, "re-subscribe did not release the previous source (#1433)" + assert wrB() is not None, "re-subscribe did not retain the new source (#1433)" + + +def test_resubscribeToEmbeddedSourcePreservesOwner(): + """Re-subscribing through a non-owning ``Msg_C`` proxy preserves its owner.""" + consumer = cModuleTemplate.cModuleTemplate() + producer = cModuleTemplate.cModuleTemplate() + + payload = messaging.CModuleTemplateMsgPayload() + payload.dataVector = [21.0, 22.0, 23.0] + producer.dataOutMsg.write(payload) + + source = producer.dataOutMsg + producer_ref = weakref.ref(producer) + consumer.dataInMsg.subscribeTo(source) + + del producer + gc.collect() + assert producer_ref() is not None + + consumer.dataInMsg.subscribeTo(source) + gc.collect() + + assert producer_ref() is not None, "re-subscribe released the embedded source owner (#1433)" + assert list(consumer.dataInMsg.read().dataVector) == [21.0, 22.0, 23.0] + + consumer.dataInMsg.unsubscribe() + gc.collect() + assert producer_ref() is None + + +def test_configReaderKeepAliveTransfersToWrapper(): + """A config reader transfers its active source keep-alive to its wrapper.""" + config = cModuleTemplate.cModuleTemplateConfig() + source_ref = _cppSourceInScope(config.dataInMsg, [31.0, 32.0, 33.0]) + gc.collect() + assert source_ref() is not None + + config_ref = weakref.ref(config) + module = config.createWrapper() + del config + gc.collect() + + assert config_ref() is None + assert source_ref() is not None, "config-to-wrapper transfer released the source (#1433)" + assert list(module.dataInMsg.read().dataVector) == [31.0, 32.0, 33.0] + + del module + gc.collect() + assert source_ref() is None + + +def test_configEmbeddedSourceOwnerSurvivesGC(): + """A wrapper reader retains the config that owns an embedded source.""" + consumer = cModuleTemplate.cModuleTemplate() + producer_config = cModuleTemplate.cModuleTemplateConfig() + + payload = messaging.CModuleTemplateMsgPayload() + payload.dataVector = [41.0, 42.0, 43.0] + producer_config.dataOutMsg.write(payload) + + source = producer_config.dataOutMsg + config_ref = weakref.ref(producer_config) + source_ref = weakref.ref(source) + consumer.dataInMsg.subscribeTo(source) + + del producer_config, source + gc.collect() + + assert config_ref() is not None, "embedded source config owner was not retained (#1433)" + assert source_ref() is None, "embedded config message proxy was retained instead of its owner (#1433)" + assert list(consumer.dataInMsg.read().dataVector) == [41.0, 42.0, 43.0] + + consumer.dataInMsg.unsubscribe() + gc.collect() + assert config_ref() is None + + +def test_configSourceOwnerLeaseTransfersToWrapper(): + """An existing source lease follows config storage into its new wrapper.""" + consumer = cModuleTemplate.cModuleTemplate() + producer_config = cModuleTemplate.cModuleTemplateConfig() + + payload = messaging.CModuleTemplateMsgPayload() + payload.dataVector = [51.0, 52.0, 53.0] + producer_config.dataOutMsg.write(payload) + + source = producer_config.dataOutMsg + config_ref = weakref.ref(producer_config) + consumer.dataInMsg.subscribeTo(source) + + producer = producer_config.createWrapper() + producer_ref = weakref.ref(producer) + del producer_config, producer, source + gc.collect() + + assert config_ref() is None + assert producer_ref() is not None, "source lease did not follow config ownership transfer (#1433)" + assert list(consumer.dataInMsg.read().dataVector) == [51.0, 52.0, 53.0] + + consumer.dataInMsg.unsubscribe() + gc.collect() + assert producer_ref() is None + + +def test_moduleDeathReleasesSources(): + """When the owning C module is collected, its subscribed sources are released + (no leak); this exercises the module-owned keep-alive dictionary.""" + mod = cModuleTemplate.cModuleTemplate() + wr = _cppSourceInScope(mod.dataInMsg, [1.0, 2.0, 3.0]) + gc.collect() + assert wr() is not None, "source not retained while the module is alive (#1433)" + + del mod + gc.collect() + assert wr() is None, "module garbage-collection did not release the subscribed source (#1433)" + + +def test_standaloneSubscriberSurvivesGCAndReleasesOnDeath(): + """A stand-alone Msg_C subscriber keeps its source alive, reads it correctly, + and releases it when the subscriber itself is collected.""" + sub = messaging.CModuleTemplateMsg_C() + wr = _cppSourceInScope(sub, [7.0, 8.0, 9.0]) + gc.collect() + assert wr() is not None, "source GC'd despite a stand-alone subscription (#1433)" + assert list(sub.read().dataVector) == [7.0, 8.0, 9.0] + + del sub + gc.collect() + assert wr() is None, "stand-alone subscriber death did not release its source (#1433)" + + +def test_rawAddressSubscriptionStillWorks(): + """Subscribing by raw integer address must keep working: that path has no Python + source object to retain (the caller owns the source's lifetime), so the keep-alive + layer must simply stay out of its way.""" + payload = messaging.CModuleTemplateMsgPayload() + payload.dataVector = [3.0, 6.0, 9.0] + persistent = messaging.CModuleTemplateMsg().write(payload) # kept alive by the caller + + mod = cModuleTemplate.cModuleTemplate() + mod.dataInMsg.subscribeTo(int(persistent.this)) # raw-address branch + assert list(mod.dataInMsg.read().dataVector) == [3.0, 6.0, 9.0] + + +def test_rawAddressResubscribeReleasesPreviousKeepAlive(): + """Re-subscribing by raw integer address releases any previous Python source.""" + payload = messaging.CModuleTemplateMsgPayload() + payload.dataVector = [9.0, 8.0, 7.0] + persistent = messaging.CModuleTemplateMsg().write(payload) + + mod = cModuleTemplate.cModuleTemplate() + wr = _cppSourceInScope(mod.dataInMsg, [1.0, 2.0, 3.0]) + gc.collect() + assert wr() is not None + + mod.dataInMsg.subscribeTo(int(persistent.this)) + gc.collect() + assert wr() is None, "raw-address re-subscribe did not release the previous source (#1433)" + assert list(mod.dataInMsg.read().dataVector) == [9.0, 8.0, 7.0] + + +if __name__ == "__main__": + test_moduleEmbeddedCppSourceSurvivesGC() + test_moduleEmbeddedCMsgSourceSurvivesGC() + test_moduleEmbeddedCMsgSourceOwnerSurvivesGC() + test_unsubscribeReleasesKeepAlive() + test_resubscribeReplacesKeepAlive() + test_resubscribeToEmbeddedSourcePreservesOwner() + test_configReaderKeepAliveTransfersToWrapper() + test_configEmbeddedSourceOwnerSurvivesGC() + test_configSourceOwnerLeaseTransfersToWrapper() + test_moduleDeathReleasesSources() + test_standaloneSubscriberSurvivesGCAndReleasesOnDeath() + test_rawAddressSubscriptionStillWorks() + test_rawAddressResubscribeReleasesPreviousKeepAlive() + print("All #1433 keep-alive tests passed.") diff --git a/src/architecture/messaging/_UnitTest/test_generateSWIGModules.py b/src/architecture/messaging/_UnitTest/test_generateSWIGModules.py index af6927d475f..9d78b87847f 100644 --- a/src/architecture/messaging/_UnitTest/test_generateSWIGModules.py +++ b/src/architecture/messaging/_UnitTest/test_generateSWIGModules.py @@ -125,8 +125,13 @@ def test_generated_message_bindings_use_module_local_classes(tmp_path): assert "from Basilisk.architecture.messaging.messageType" not in new_messaging_template assert "if type(source) == messageType ## _C:" in new_messaging_template - assert "from Basilisk.architecture.messaging import {type}" not in generated + assert "from Basilisk.architecture.messaging import CustomMsg" not in generated + assert "from Basilisk.architecture.messaging import _msgKeepAlive" in generated assert "elif type(source) == CustomMsg:" in generated + assert "recorder = self._recorder(timeDiff)" in generated + assert "_msgKeepAlive.retainRecorderSource(recorder, self)" in generated + assert "if args:" in generated + assert "_msgKeepAlive.retainRecorderConstructorSource(self, args[0])" in generated def test_generated_read_functor_without_c_interface_uses_cpp_message(tmp_path): diff --git a/src/architecture/messaging/_msgKeepAlive.py b/src/architecture/messaging/_msgKeepAlive.py new file mode 100644 index 00000000000..a36890f8f63 --- /dev/null +++ b/src/architecture/messaging/_msgKeepAlive.py @@ -0,0 +1,276 @@ +# +# ISC License +# +# Copyright (c) 2026, Autonomous Vehicle Systems Lab, University of Colorado at Boulder +# +# Permission to use, copy, modify, and/or distribute this software for any +# purpose with or without fee is hereby granted, provided that the above +# copyright notice and this permission notice appear in all copies. +# +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN +# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF +# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +# + +"""Keep-alive helpers for C-message (``Msg_C``) readers -- issue #1433. + +When a ``Msg_C`` reader (for example a C module's ``dataInMsg``) subscribes to a +stand-alone source, it stores *raw pointers* into that source's memory. If the +only Python reference to the source then goes out of scope, Python may +garbage-collect it and the subscriber is left reading freed memory. + +Likewise, a recorder created from a ``Msg_C`` stores raw pointers into that +message's storage. The recorder therefore retains the stand-alone message or +embedded-message owner for as long as the recorder exists. + +Issue #676 fixed the C++ ``ReadFunctor`` direction by hanging the keep-alive on +that reader's C++ destructor. A ``Msg_C`` is a plain C struct with no destructor, +so this module keeps the source alive from the Python side instead. + +The retention target follows the Python object that owns the source storage: + +* a stand-alone message source retains the source proxy itself; +* an embedded ``Msg_C`` source retains its owning config or module wrapper through + a transferable lease, because the embedded proxy is transient and does not own + the source storage. + +Where the reference is stored depends on how the subscriber is owned: + +* a stand-alone ``Msg_C`` subscriber pins the source on its own persistent proxy; +* an embedded ``Msg_C`` subscriber pins the source in a hidden dictionary on its + owning config or C-module wrapper. + +In both cases the reference is also released on an explicit ``unsubscribe()`` or +when the subscriber re-subscribes to a different source. + +Recorders store their retention target directly on the persistent recorder proxy. +The reference is released automatically with the recorder. + +All access happens on the Python side under the GIL (module construction, +recorder construction, subscribe/unsubscribe calls, and ``weakref`` finalizers +all run in the interpreter thread), so the registries need no additional locking. +""" + +import weakref + +#: Embedded ``Msg_C`` C-address (``int``) -> (weak owner handle, owner token). +_owner_by_address = {} + +#: Attribute name used to pin a retention target on a persistent SWIG proxy. +_PIN_ATTR = "_bskKeepAliveSource" + +#: Attribute name used for the module-owned keep-alive dictionary. +_MODULE_PIN_ATTR = "_bskMsgKeepAlive" + +#: Attribute names used to make module registration idempotent and finalizable. +_MODULE_ADDRS_ATTR = "_bskMsgKeepAliveAddrs" +_MODULE_TOKEN_ATTR = "_bskMsgKeepAliveToken" +_MODULE_OWNER_HANDLE_ATTR = "_bskMsgKeepAliveOwner" + + +class _OwnerLease: + """Strong reference to the current owner of embedded message storage.""" + + def __init__(self, owner): + self.owner = owner + + +class _OwnerHandle: + """Track an embedded message owner and every active lease on that owner.""" + + def __init__(self, owner): + self._owner_ref = weakref.ref(owner) + self._leases = weakref.WeakSet() + + @property + def owner(self): + """Return the current storage owner, or ``None`` after its collection.""" + return self._owner_ref() + + def lease(self): + """Return a strong, transferable reference to the current owner.""" + owner = self.owner + if owner is None: + return None + lease = _OwnerLease(owner) + self._leases.add(lease) + return lease + + def transfer(self, owner): + """Move this handle and all active leases to a new storage owner.""" + self._owner_ref = weakref.ref(owner) + for lease in self._leases: + lease.owner = owner + + +def registerModule(module): + """Register the embedded ``Msg_C`` fields owned by ``module`` or a config. + + The C-module and config SWIG wrappers call this from their Python constructors. + SWIG returns fresh non-owning proxies for embedded C messages on each attribute + access, so this registry lets later ``subscribeTo()`` calls recover the storage + owner from an embedded message's stable C address. + """ + if hasattr(module, _MODULE_ADDRS_ATTR): + return + + _register_owner(module, _OwnerHandle(module), {}) + + +def transferModuleOwner(config, module): + """Transfer config-owned message storage and keep-alives to ``module``. + + Existing leases are retargeted so subscriptions established before + ``createWrapper()`` retain the module after its C wrapper takes ownership of the + config storage. + """ + if not hasattr(config, _MODULE_OWNER_HANDLE_ATTR): + registerModule(config) + + handle = getattr(config, _MODULE_OWNER_HANDLE_ATTR) + pins = getattr(config, _MODULE_PIN_ATTR) + object.__setattr__(config, _MODULE_PIN_ATTR, {}) + + handle.transfer(module) + _register_owner(module, handle, pins) + + +def retainSource(subscriber, source): + """Retain ``source`` for as long as ``subscriber`` (a ``Msg_C``) reads it. + + Replaces any previously retained source for the same subscriber, dropping the + old reference. + """ + target = _retention_target(source) + releaseSource(subscriber) + + owner = _owner_of(subscriber) + if owner is not None: + getattr(owner, _MODULE_PIN_ATTR)[int(subscriber.this)] = target + elif getattr(subscriber, "thisown", False): + # Stand-alone subscriber: persistent, owning proxy -- pin on the proxy. + object.__setattr__(subscriber, _PIN_ATTR, target) + else: + # Unknown non-owning subscriber. This is unusual, but retaining on the + # proxy is still the least surprising fallback for Python-created objects. + object.__setattr__(subscriber, _PIN_ATTR, target) + + +def releaseSource(subscriber): + """Drop the retained source for ``subscriber`` (no-op if none is held).""" + owner = _owner_of(subscriber) + if owner is not None: + pins = getattr(owner, _MODULE_PIN_ATTR, None) + if pins is not None: + pins.pop(int(subscriber.this), None) + else: + try: + delattr(subscriber, _PIN_ATTR) + except AttributeError: + pass + + +def retainRecorderSource(recorder, source): + """Retain the storage owner of ``source`` for the lifetime of ``recorder``.""" + object.__setattr__(recorder, _PIN_ATTR, _retention_target(source)) + + +def copyRecorderSource(recorder, sourceRecorder): + """Copy the source retention target when a Python recorder is copied.""" + try: + target = vars(sourceRecorder).get(_PIN_ATTR) + except TypeError: + return + if target is not None: + object.__setattr__(recorder, _PIN_ATTR, target) + + +def retainRecorderConstructorSource(recorder, source): + """Retain a direct ``Msg_C`` constructor source or copy an existing pin.""" + if _looks_like_c_msg(source): + retainRecorderSource(recorder, source) + else: + copyRecorderSource(recorder, source) + + +def _retention_target(source): + """Return the Python owner that must stay alive for ``source`` to be valid.""" + handle = _owner_handle_of(source) + if handle is None: + return source + lease = handle.lease() + return lease if lease is not None else source + + +def _owner_of(msg): + """Return the current storage owner for embedded ``Msg_C`` ``msg``.""" + handle = _owner_handle_of(msg) + return handle.owner if handle is not None else None + + +def _owner_handle_of(msg): + """Return the transferable owner handle for embedded ``Msg_C`` ``msg``.""" + if not _looks_like_c_msg(msg) or getattr(msg, "thisown", False): + return None + + entry = _owner_by_address.get(int(msg.this)) + if entry is None: + return None + + handle_ref, _ = entry + return handle_ref() + + +def _register_owner(owner, handle, pins): + """Register ``owner`` and its embedded message addresses with ``handle``.""" + token = object() + addresses = tuple(int(msg.this) for msg in _embedded_c_msgs(owner)) + + object.__setattr__(owner, _MODULE_OWNER_HANDLE_ATTR, handle) + object.__setattr__(owner, _MODULE_TOKEN_ATTR, token) + object.__setattr__(owner, _MODULE_PIN_ATTR, pins) + object.__setattr__(owner, _MODULE_ADDRS_ATTR, addresses) + + handle_ref = weakref.ref(handle) + for address in addresses: + _owner_by_address[address] = (handle_ref, token) + + weakref.finalize(owner, _release_module_owner, addresses, token) + + +def _embedded_c_msgs(module): + """Yield embedded ``Msg_C`` proxies exposed as SWIG properties on ``module``.""" + seen = set() + for cls in type(module).mro(): + for name, attr in cls.__dict__.items(): + if name in seen or not isinstance(attr, property): + continue + seen.add(name) + try: + value = getattr(module, name) + except Exception: + continue + if _looks_like_c_msg(value) and not getattr(value, "thisown", False): + yield value + + +def _looks_like_c_msg(value): + """Return ``True`` for generated ``*Msg_C`` SWIG proxy objects.""" + return ( + type(value).__name__.endswith("Msg_C") + and hasattr(value, "this") + and hasattr(value, "header") + and hasattr(value, "payload") + ) + + +def _release_module_owner(addresses, token): + """Forget ownership entries that still belong to the finalized owner.""" + for addr in addresses: + entry = _owner_by_address.get(addr) + if entry is not None and entry[1] is token: + _owner_by_address.pop(addr, None) diff --git a/src/architecture/messaging/msgAutoSource/cMsgCInterfacePy.i.in b/src/architecture/messaging/msgAutoSource/cMsgCInterfacePy.i.in index 5093682f36e..ab8f8a91af9 100644 --- a/src/architecture/messaging/msgAutoSource/cMsgCInterfacePy.i.in +++ b/src/architecture/messaging/msgAutoSource/cMsgCInterfacePy.i.in @@ -6,26 +6,41 @@ %include "architecture/messaging/msgHeader.h" typedef struct {type}; %extend {type}_C {{ - Recorder<{type}Payload> recorder(uint64_t timeDiff = 0) {{ + Recorder<{type}Payload> _recorder(uint64_t timeDiff = 0) {{ self->header.isLinked = 1; return Recorder<{type}Payload>{{static_cast(self), timeDiff}}; }} %pythoncode %{{ + def recorder(self, timeDiff=0): + """Create a recorder that keeps this C message's storage owner alive.""" + from Basilisk.architecture.messaging import _msgKeepAlive + recorder = self._recorder(timeDiff) + _msgKeepAlive.retainRecorderSource(recorder, self) + return recorder + def subscribeTo(self, source): """subscribe to another message source""" + from Basilisk.architecture.messaging import _msgKeepAlive if type(source) == type(self): {type}_C_subscribe(self, source) elif type(source) == {type}: {type}_cpp_subscribe(self, source) elif type(source) == int: #Note that is assumes it is a uint64_t address in memory {type}_addr_subscribe(self, source) + _msgKeepAlive.releaseSource(self) + return # a raw address: the caller owns the source, nothing to keep alive else: raise Exception('tried to subscribe {type} to another message type') + # issue #1433: this Msg_C reader now holds raw pointers into `source`; keep the + # (otherwise garbage-collectable) source object alive for as long as we read it + _msgKeepAlive.retainSource(self, source) def unsubscribe(self): """Unsubscribe to the connected message, noop if no message was connected""" + from Basilisk.architecture.messaging import _msgKeepAlive + _msgKeepAlive.releaseSource(self) # issue #1433: drop the keep-alive, if any {type}_unsubscribe(self) def isSubscribedTo(self, source): diff --git a/src/architecture/messaging/msgAutoSource/msgInterfacePy.i.in b/src/architecture/messaging/msgAutoSource/msgInterfacePy.i.in index c9e0c8e2ec3..68e5932a162 100644 --- a/src/architecture/messaging/msgAutoSource/msgInterfacePy.i.in +++ b/src/architecture/messaging/msgAutoSource/msgInterfacePy.i.in @@ -39,6 +39,11 @@ STRUCTASLIST(CSSArraySensorMsgPayload) %ignore SysModel::SysModel(const SysModel &); %ignore SysModel::operator=(const SysModel &); +%pythonappend Recorder::Recorder %{{ + from Basilisk.architecture.messaging import _msgKeepAlive + if args: + _msgKeepAlive.retainRecorderConstructorSource(self, args[0]) +%}} %include "messaging/messaging.h" %include "_GeneralModuleFiles/sys_model.h"