[#1433] Keep Msg_C reader subscriptions alive (module-scoped keep-alive) by robotrocketscience · Pull Request #1442 · AVSLab/basilisk

robotrocketscience · 2026-06-26T00:01:47Z

Tickets addressed: C-message reader keep-alive (follow-up to #676) #1433
Review: By commit
Merge strategy: Squash and merge

Description

Follow-up to #676 / #1432. That work fixed the garbage-collection use-after-free for C++ readers (ReadFunctor<T>): the subscription keeps the Python source alive via a keep-alive bridge driven by ReadFunctor's C++ destructor. It did not cover the other direction — a C-module input message (Msg_C reader) subscribing to a stand-alone source. A Msg_C is a plain C struct with no destructor, so there is nothing to hang a Py_DECREF on, and #1432 deferred it.

This PR closes that gap from the Python side. Msg_C.subscribeTo(...) now retains the source in a small keep-alive registry (_msgKeepAlive.py) for as long as the subscription is live, and releases it on unsubscribe(), on re-subscribe, and when the subscriber dies. Release is driven differently depending on how the subscriber is owned, and that distinction was settled by probing the live build rather than assuming:

A stand-alone Msg_C subscriber owns its struct (thisown=True) and its proxy is persistent, so the source is pinned as an attribute on that proxy and dies with it.
A module-embedded Msg_C subscriber (e.g. mod.dataInMsg) is reached through a transient proxy — SWIG hands out a fresh, non-owning proxy on every attribute access — so it has no usable per-message destruction hook. (Notably this means the Msg_C proxy __del__/tp_dealloc suggested in the issue would misfire: it fires on the throwaway temporary, not at module death.) Instead, the source is pinned in the registry keyed by the subscriber struct's stable address, and the owning C module's CWrapper — which does have a real C++ destructor — arms a weakref finalizer over its config address range to release every embedded subscriber's source when the module is garbage-collected. This mirrors how [#676] Keep subscribed stand-alone messages alive (Python refcount bridge) #1432 relies on the C++ reader's destructor.

Two new CWrapper getters (getConfigAddress()/getConfigSize()) expose the config extent used by the range release.

Commits are organized as impl / test / docs.

Verification

New regression test test_cMsgReaderKeepAlive.py (7 cases) asserts the lifetime contract deterministically with weakref (no reliance on memory being clobbered), plus one end-to-end check that a module reads correct live data after a GC pass:

C++ Message source survives GC while subscribed (and reads correctly end-to-end)
stand-alone Msg_C source survives GC while subscribed
unsubscribe() releases the keep-alive
re-subscribe releases the previous source and retains the new one
module garbage-collection releases its subscribers' sources (no leak — exercises the config address-range finalizer)
stand-alone subscriber keeps its source alive and releases it on its own death
raw integer-address subscription is unaffected

Full messaging suite (111 tests) and a sample of C-module suites (cModuleTemplate, mrpFeedback, mrpPD — 54 tests) pass on a clean build. Built and tested locally against this branch.

Documentation

Added release-notes snippet 1433-cmsg-reader-keepalive.rst. The bskKnownIssues.rst workaround note for this case is added in #1432; once both land that note can be trimmed to reflect that the workaround is no longer required for C-module readers.

Future work

None anticipated for this case. The two new CWrapper getters are general-purpose and could support future address-range bookkeeping if needed.

schaubh · 2026-06-26T19:26:04Z

Thanks for taking a stab at this issue. I think it is an interesting direction. I did come across some issues. For example, for module internal C-msgs they are retained even when the parent BSK module are deleted.

I pushed a commit to this test branch to show an alternate approach that builds on this. Additional tests are added to catch the issues before.

Further, the CI builds were failing on Linux and macOS. It was related to the order in how a messaging init.py file was created. This commit addressed that as well. Let's see if all CI tests pass now.

Finally, I won't close this PR until we have more time to evaluate this. For example, the BSK-SDK plugin code will need to be updated in a synchronous manner to keep BSK and BSK-SDK in sync. That will take more time to fully test on both sides.

schaubh · 2026-06-27T21:48:13Z

I found why the documentation threw a warning and fixed it. All tests should pass now. Interesting branch. Let me think on this more. We are looking at a major change to the messaging system that might make this not needed. I'll let this PR sit for now.

schaubh · 2026-07-11T19:49:18Z

Howdy @robotrocketscience , now that I'm on the 2.12 beta cycle I would like to wrap up your two pending PRs to keep msgs alive. I rebased this PR branch on latest develop, and then let Code 5.6 Sol (Extra High) loose on it. It found some more edge issues that I fixed. I also update the BSK learning documentation on this issue.

Thanks for reviewing my changes, let me know if you are ok with them.

robotrocketscience · 2026-07-15T16:19:15Z

Thanks @schaubh — reviewed your owner-aware rework (3a1e9abd9 and follow-ups) and built + ran the messaging tests on Linux. I'm good with it. The transferable-lease + token-guarded weakref.finalize design cleanly fixes the "module-internal C-msg retained after the parent module is deleted" problem you flagged in my original. Verified empirically: _owner_by_address grows on module construction and returns to baseline after the module is collected — no registry leak, and the config→wrapper lease transfer holds up.

Two things I turned up in deep review, both for your judgment:

1. Docs / known-issues. This branch doesn't touch bskKnownIssues.rst, so once it merges the BSK-676 bullet added in #1432 still reads "C-module input messages (Msg_C readers ...) are not yet covered by this automatic keep-alive ... this case is tracked in pull request #1442" — which this PR makes false. Worth amending that bullet here (and adding the usual BSK-1433: ... fixed in the current version line) so develop doesn't ship self-contradicting docs. Order-sensitive if #1432 lands first.

2. Msg_C.recorder() gap (pre-existing, likely out of scope). The subscribeTo direction is now covered, but a stand-alone C-message's .recorder() still isn't — cMsgCInterfacePy.i.in builds the Recorder from a raw void* with no keep-alive. Repro on Linux:

import gc, weakref
from Basilisk.architecture import messaging

p = messaging.CModuleTemplateMsgPayload()
src = messaging.CModuleTemplateMsg_C(); src.write(p)
wr = weakref.ref(src)
rec = src.recorder(); del src; gc.collect()
assert wr() is None   # source freed while `rec` still holds a void* into it

Same use-after-free class as #676/#1433, fixed by neither PR (the C++ Message.recorder() path is covered by #1432). Not a regression — probably a separate follow-up issue rather than something to fold in here.

Everything else checks out. Nice cleanup.

… their source A Msg_C reader (e.g. a C module's dataInMsg) that subscribes to a stand-alone source stores raw pointers into that source's memory. If the only Python reference to the source then went out of scope, Python could garbage-collect it and the reader was left reading freed memory. Issue AVSLab#676 fixed this for the C++ ReadFunctor direction via that reader's C++ destructor; a Msg_C has no destructor to hang the release on, so this completes the other direction from Python. subscribeTo now retains the source via a process-wide keep-alive registry keyed by the subscriber struct's stable address. A stand-alone subscriber pins the source on its own (persistent) proxy; a module-embedded subscriber's proxy is transient, so the owning C module's wrapper arms a weakref finalizer over its config address range to release every embedded subscriber's source when the module is collected. The reference is also dropped on unsubscribe() and on re-subscribe. New CWrapper getConfigAddress()/getConfigSize() expose the config extent for the range release.

Msg_C subscriptions store raw pointers into the source message object. If the source is only referenced from Python and is garbage-collected while a C-message reader is still subscribed, the reader can be left pointing into freed memory. The earlier keep-alive approach handled the simple standalone-source case, but it did not model ownership correctly for C modules. C module messages are embedded fields inside the module config struct. SWIG returns transient, non-owning Python proxies for those embedded Msg_C fields on each attribute access, so the proxy itself is not the object that owns the underlying message storage. Retaining that proxy is therefore the wrong lifetime anchor. The owning Python object is the C module wrapper, whose config contains the embedded messages. Register each C module wrapper with the Python keep-alive helper when the SWIG CWrapper is constructed. Registration discovers the wrapper's embedded Msg_C properties and records their stable C addresses in an owner map. Later, when a Msg_C subscribes to a source, the keep-alive layer resolves the true retention target: - standalone message sources retain the source proxy itself; - module-embedded Msg_C sources retain the owning module wrapper; - standalone Msg_C subscribers store the keep-alive on their own proxy; - module-embedded Msg_C subscribers store keep-alives in a hidden dictionary on the owning module wrapper. This avoids adding config address/size APIs to the CWrapper and avoids releasing subscriptions by address range. Ownership is tracked directly by embedded message address, with weak finalizers cleaning the owner map when a module wrapper is collected. Also release any previous Python-owned source when a Msg_C is re-subscribed by raw integer address, since raw-address subscriptions are explicitly caller-owned and should not keep an earlier Python source alive. Copy _msgKeepAlive.py into the generated messaging package at build time rather than configure time. This prevents the helper copy from creating the generated messaging package directory early, before generatePackageInit.py writes the real messaging __init__.py. Add the C-message SWIG template as a dependency so changes to cMsgCInterfacePy.i.in regenerate the payload interfaces. Add regression coverage for: - embedded C-module readers retaining Python sources; - embedded Msg_C sources retaining their owning producer module; - unsubscribe and re-subscribe releasing keep-alives; - standalone Msg_C subscribers releasing sources on collection; - raw-address subscriptions preserving caller-owned semantics.

The docs crawler auto-generates reStructuredText pages for Python files under src. Adding architecture/messaging/_msgKeepAlive.py caused it to generate a page with the target label: .. __msgKeepAlive: Because the module name starts with an underscore, docutils interprets that as a malformed hyperlink target. Sphinx treats warnings as errors in CI, so both the docs job and the canary build failed during documentation generation. Skip private Python helper modules whose filenames begin with "_" when collecting files for generated API documentation. This keeps internal runtime helpers such as _msgKeepAlive.py out of the public docs tree and avoids malformed generated RST labels.

…ings The Msg_C keep-alive change inadvertently reintroduced an import of the generated C++ message class from Basilisk.architecture.messaging: from Basilisk.architecture.messaging import {type}, _msgKeepAlive That assumption is valid for built-in Basilisk messages, but not for custom messages generated by BSK-SDK extensions. Extension message classes live in the extension's own messaging package, so attempting to import their type from Basilisk.architecture.messaging raises ImportError before subscribeTo() can connect the messages. This also reversed the develop-branch fix that made generated message subscriptions package-independent. Import only the shared _msgKeepAlive helper from Basilisk. The generated {type} class is already present in the generated module's global namespace, so subscribeTo() continues to resolve built-in message types while also working for BSK-SDK and other externally generated message packages. Strengthen the generator regression test to inspect the rendered CustomMsg name instead of the unexpanded {type} template placeholder. Also verify that the generated interface still imports _msgKeepAlive. This ensures a future template change or rebase cannot silently restore the hard-coded built-in message import. Tests: - pytest -q src/architecture/messaging/_UnitTest/test_generateSWIGModules.py

Msg_C readers retain the Python object that owns their subscribed source storage. For module-embedded C messages, that object is the owning module wrapper rather than the transient, non-owning Msg_C proxy returned by SWIG. When replacing a subscription, retainSource() previously released the old keep-alive before resolving the owner of the incoming source. This ordering could destroy the source module during the re-subscription operation. For example, a consumer could subscribe to an embedded producer message and then release its ordinary reference to the producer. The active subscription would correctly keep the producer alive. If the consumer subsequently re-subscribed through a previously obtained non-owning message proxy, the keep-alive helper would: 1. release the existing reference to the producer; 2. destroy the producer and its embedded message storage; 3. attempt to resolve the incoming proxy's owner after that owner was gone; 4. retain only the now-dangling non-owning proxy. The consumer would then hold raw pointers into released message storage. This could occur when wiring code is run more than once or when switching between embedded outputs owned by the same producer. Resolve the incoming source's retention target before releasing the previous keep-alive. The local target reference now keeps the source owner alive while the subscription is replaced, after which the target is stored as the new keep-alive. This preserves atomic re-subscription behavior without changing explicit unsubscribe semantics. Once unsubscribe() is called, the source owner remains eligible for garbage collection, and callers are still responsible for retaining it if they intend to reconnect later. Add a regression test that: - subscribes through an embedded Msg_C source; - removes the ordinary reference to the producer; - re-subscribes using the remaining non-owning source proxy; - verifies that the producer remains alive and its data remains readable; - verifies that an explicit unsubscribe still releases the producer. Tests: - pytest -q src/architecture/messaging/_UnitTest/test_cMsgReaderKeepAlive.py - pytest -q src/architecture/messaging/_UnitTest

Directly constructed C module config objects own the storage for their embedded Msg_C fields before they are converted into CWrapper modules. SWIG returns a fresh, non-owning Python proxy whenever one of these message fields is accessed. The keep-alive registry previously registered only CWrapper instances. Embedded messages accessed through a raw *Config object therefore had no known storage owner and fell back to retaining their transient proxy. That did not protect the underlying config storage. This left two unsafe paths: - A config reader could subscribe to a Python message before createWrapper(). Once the transient reader proxy was collected, its source keep-alive was also lost, leaving the config reader connected to released storage. - A module reader subscribing to a config-owned output retained the transient output proxy rather than the config that owned its storage. The config could then be collected while the reader still held raw pointers into it. Register raw config objects as message-storage owners from their generated SWIG constructors. Embedded config messages can now resolve their owner by stable C address in the same way as messages embedded in CWrapper modules. Creating a wrapper introduces an additional ownership transition: createWrapper() hands the config pointer to CWrapper and marks the Python config proxy as non-owning. Existing subscriptions must follow that transition. Retaining the config object directly would no longer be sufficient because the wrapper now owns and eventually releases the underlying storage. Introduce transferable owner handles and leases: - each config or wrapper has an owner handle registered by embedded message address; - subscriptions to embedded sources retain a lease on the current storage owner; - the handle tracks its active leases without retaining the owner itself; - transferModuleOwner() moves config-owned subscriber pins to the wrapper and retargets every existing source lease to the wrapper; - token-protected weak finalizers continue to prevent an old config finalizer from removing address registrations that have moved to its wrapper. This preserves existing standalone-message behavior and avoids introducing owner-reference cycles for ordinary subscriptions. Explicit unsubscribe and owner destruction still release their retained sources normally. Add regression coverage confirming that: - a raw config reader retains its source before createWrapper(); - the active keep-alive transfers to the wrapper; - a wrapper reader retains a config that owns its embedded source; - transient embedded proxies are not retained in place of their owners; - an existing source lease follows config storage into a new wrapper; - unsubscribe and wrapper destruction release the transferred owners and sources. Tests: - pytest -q src/architecture/messaging/_UnitTest/test_cMsgReaderKeepAlive.py - pytest -q src/architecture/messaging/_UnitTest - pytest -q src/moduleTemplates/cModuleTemplate/_UnitTest

… memory

It talked about having to explicitly retain a stand-alone msg in memory which is no longer true.

A recorder created from a C message stores raw pointers into the Msg_C source storage. If the only Python reference to that source went out of scope, Python could garbage-collect the message while the recorder still used its header and payload pointers. Subsequent recording could therefore read freed or corrupted memory. Extend the owner-aware Msg_C keep-alive mechanism to recorders. Wrap the native Msg_C recorder constructor and attach the resolved retention target to the persistent recorder proxy. Stand-alone sources retain their message proxy, while embedded sources retain a transferable lease on the config or C-module wrapper that owns their storage. Config-owned leases continue to follow the storage when createWrapper() transfers ownership to a wrapper. Also preserve the retention target when a Recorder is copied from Python. The C++ Recorder copy constructor duplicates its ReadFunctor and raw source pointers, but Python attributes are not copied automatically. Propagating the hidden retention target prevents a copied recorder from outliving the source after the original recorder is collected. Apply the same behavior to direct Recorder(Msg_C) construction so callers receive the lifetime guarantee whether they use msg.recorder() or construct the generated recorder class explicitly. Default and C++ Message recorder construction remain unchanged. Add regression coverage for: - stand-alone Msg_C sources retained by recorders; - direct Recorder(Msg_C) construction; - copied recorders inheriting source retention; - module-embedded sources retaining their owning module; - config-owned source leases transferring to C-module wrappers; - source owners being released when the final recorder is collected; and - generated bindings containing the recorder keep-alive hooks. Update the learning documentation and release note to state that message recorders retain the objects that own their source storage. Tests: - full generated-message CMake build - pytest -q src/architecture/messaging/_UnitTest - pytest -q src/moduleTemplates/cModuleTemplate/_UnitTest - pytest -q src/tests/test_scenarioFlexiblePanel.py - pre-commit on all modified files

schaubh · 2026-07-16T15:39:17Z

Howdy @robotrocketscience , thanks for the feedback.

I do plan on merging the other PR first, then the documentation on this branch will make sense.
corrected the Msg_C.recorder() gap, added new tests to validated expected behavior.

I think this branch is good to go. I'll just wait for your final approval of #1432 and will then merge this branch. Thanks for the support!

robotrocketscience requested a review from a team as a code owner June 26, 2026 00:01

schaubh self-requested a review June 26, 2026 18:25

schaubh self-assigned this Jun 26, 2026

schaubh added the enhancement New feature or request label Jun 26, 2026

schaubh added this to Basilisk Jun 26, 2026

schaubh moved this to 🏗 In progress in Basilisk Jun 29, 2026

This was referenced Jul 2, 2026

[#676] Keep subscribed stand-alone messages alive (Python refcount bridge) #1432

Open

Message objects created within python function contain garbage data #295

Open

schaubh force-pushed the feature/bsk-1433--cmsg-reader-keepalive branch from f0384de to 5463990 Compare July 11, 2026 19:47

robotrocketscience and others added 11 commits July 16, 2026 09:00

[AVSLab#1433] Add regression test for Msg_C reader keep-alive

9195af9

[AVSLab#1433] Add release note for the Msg_C reader keep-alive fix

b81198c

[AVSLab#1433] update docs on how a stand-alone msg is retained now in…

f9f73a3

… memory

[AVSLab#1433] update example scenario

5babf6b

It talked about having to explicitly retain a stand-alone msg in memory which is no longer true.

schaubh force-pushed the feature/bsk-1433--cmsg-reader-keepalive branch from 5463990 to 0b5a512 Compare July 16, 2026 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[#1433] Keep Msg_C reader subscriptions alive (module-scoped keep-alive)#1442

[#1433] Keep Msg_C reader subscriptions alive (module-scoped keep-alive)#1442
robotrocketscience wants to merge 11 commits into
AVSLab:developfrom
robotrocketscience:feature/bsk-1433--cmsg-reader-keepalive

robotrocketscience commented Jun 26, 2026

Uh oh!

schaubh commented Jun 26, 2026

Uh oh!

schaubh commented Jun 27, 2026

Uh oh!

schaubh commented Jul 11, 2026

Uh oh!

robotrocketscience commented Jul 15, 2026

Uh oh!

schaubh commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

robotrocketscience commented Jun 26, 2026

Description

Verification

Documentation

Future work

Uh oh!

schaubh commented Jun 26, 2026

Uh oh!

schaubh commented Jun 27, 2026

Uh oh!

schaubh commented Jul 11, 2026

Uh oh!

robotrocketscience commented Jul 15, 2026

Uh oh!

schaubh commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants