[#676] Keep subscribed stand-alone messages alive (Python refcount bridge)#1432
Open
robotrocketscience wants to merge 13 commits into
Open
Conversation
robotrocketscience
force-pushed
the
feature/bsk-676--message-keepalive-refcount
branch
from
1ebae69 to
01eb9d5
Compare
Contributor
|
@robotrocketscience , is this draft PR still needed given the status of #1433 ? |
Contributor
Author
|
This isn't a duplicate of #1433. The two PRs fix opposite subscriber directions:
They edit different But I think holding this PR is the right call, for reasons beyond the overlap:
#676 is still a real bug on develop. The hand-retain workarounds in #1431, #918, and #1107 keep coming back, so it does need a fix at some point. My thinking is: keep #1432 as a draft so the reproduction test stays around, and decide based on the #282 timeline. If smart pointers aren't near-term, I'll combine #1432 and #1442 into one keep-alive PR built on whatever #1457 settles for the bindings, instead of carrying two. What are your thoughts? |
Contributor
|
Thanks for the feedback @robotrocketscience. Let's keep both PRs for now and see how the general message wrapping work develops. |
Contributor
|
Ok, I'll pull this PR as well and do a review of it. |
A subscribed ReadFunctor stores only raw pointers into the source message's payload/header. If the Python source goes out of scope it is garbage collected and the reader dereferences freed memory (issue AVSLab#676). Add an opaque keep-alive bridge to ReadFunctor: a void* owner token plus acquire/release C function pointers, set only from the SWIG layer. messaging.h stays free of <Python.h> so the C/C++ core still builds without Python headers; a no-Python build sees null callbacks and pays nothing. Because a ReadFunctor is normally a C++ module member (where SWIG %extend destructors never run), the lifetime bookkeeping is done in a hand-written rule-of-five: the destructor releases, copy acquires, move transfers, and copy-assign releases-then-acquires with self- and pointer-equality guards. Moves are noexcept so std::vector<ReadFunctor> reallocation moves rather than copies. unsubscribe() now also drops the keep-alive reference.
After a C++ reader subscribes (to either a C++ Message or a C Msg_C source), the Python subscribeTo wrapper now installs the keep-alive on the reader, incrementing the source object's refcount so it survives until the reader is destroyed. The acquire/release callbacks are GIL-safe: they guard on Py_IsInitialized() (avoiding the hang-until-exit at interpreter finalization) and bracket the refcount op with PyGILState_Ensure/Release. All Python refcounting lives here in the SWIG layer, keeping messaging.h Python-free.
Subscribes a C++ reader to a stand-alone message created inside a helper function scope, forces gc.collect(), then runs the sim and asserts the data survived. Without the keep-alive this read freed/garbage memory. Also tests that unsubscribe() releases the keep-alive without crashing.
setSource() unconditionally early-returned when the incoming source matched the one already held, but _install_keepalive had already taken a Py_INCREF that the early return then dropped on the floor. Re-subscribing a reader to the same message therefore leaked one reference per call, which unsubscribe/destruction (a single release) never balanced. Release the redundant reference in the no-op branch so the per-call refcount stays balanced.
Update the synthetic CustomMsgReader used by the SWIG interface generator tests to provide the _install_keepalive() hook expected by generated subscribeTo() methods. Track the installed source in the fixture and assert that subscribeTo() passes the subscribed message to the keepalive hook. This keeps the test double aligned with the real SWIG %extend implementation and fixes test_generated_read_functor_without_c_interface_uses_cpp_message. Also remove a stray merge-conflict marker from bskKnownIssues.rst. Tests: - test_generateSWIGModules.py::test_generated_read_functor_without_c_interface_uses_cpp_message - test_standaloneMsgKeepAlive.py Result: 3 passed
Route the Python-facing Message.recorder() helper through a SWIG bridge that installs the source message handle on the recorder-owned ReadFunctor. This prevents a stand-alone C++ message from being garbage collected while its recorder still points into the message payload and header. Delegate source ownership to the existing ReadFunctor keep-alive machinery so Recorder copies acquire independent references and destruction releases them correctly. Hide the internal Recorder::setSource() callback bridge from the generated Python API. Add a weak-reference regression test that verifies the source message: - survives garbage collection while its recorder exists - remains readable through Recorder.UpdateState() - is released after the recorder is destroyed Update the BSK-676 known-issue entry and release-note snippet to state that the automatic keep-alive covers both C++ ReadFunctor subscriptions and Message.recorder(). C-module input readers remain outside the current scope. Tests: - 115 architecture/messaging unit tests passed - targeted CModuleTemplateMsgPayload SWIG rebuild passed
…ation Add a shared runtime-safety check before the ReadFunctor keep-alive callbacks acquire the GIL. The check verifies that Python is initialized and, when the active C API exposes it, that interpreter finalization has not begun. Gate Py_IsFinalizing() on both Python 3.13+ headers and an compatible limited-API level. This preserves Basilisk's Python 3.9 stable-ABI support, where Py_IsFinalizing() is intentionally unavailable, while protecting non-limited and ABI-3.13+ builds from unsafe PyGILState_Ensure() calls during shutdown. Route the initial source increment through the GIL-safe acquire callback as well. SWIG may release the GIL around _install_keepalive(), so a bare Py_INCREF there was not guaranteed to be safe. Add generator-level regression checks for the finalization guard, stable ABI condition, and guarded source acquisition. Tests: - 116 architecture/messaging unit tests passed - CModuleTemplateMsgPayload SWIG binding rebuilt successfully
Python Message.addSubscriber() returned a C++ ReadFunctor without installing the SWIG keep-alive callbacks. When the source message was created in a local scope, it could be garbage collected while the returned reader still held pointers to its payload and header. Route the Python method through a SWIG helper that acquires the source PyObject and transfers that ownership token to the reader. Add a weak-reference regression test that verifies the source survives while the reader exists and is released with the reader.
The existing Py_IsFinalizing() check is compiled out when Basilisk targets its default Python 3.9 Stable ABI, even on Python 3.13 and newer. A late ReadFunctor release could therefore call PyGILState_Ensure() after interpreter finalization had begun. Register a Python atexit callback in every generated message module to set an atomic shutdown flag before finalization. Keep the direct Py_IsFinalizing() guard when the active C API exposes it, and extend the generator regression test to cover both shutdown checks.
ReadFunctor released its Python owner while the ownership token and callback were still installed. Py_DECREF can run a weak-reference callback that re-enters unsubscribe(), allowing the same token to be released twice. Snapshot and clear the ownership state before invoking the release callback so reentrant cleanup observes an already-released reader. Also remove the incorrect noexcept declarations from move operations because moving BSKLogger or the payload is not guaranteed not to throw; retaining noexcept could terminate the process instead of propagating an exception. Add a regression test whose source weak-reference callback re-enters unsubscribe() and verifies one callback, one release, and a cleanly unlinked reader.
Note that C++ reader subscriptions now keep their source alive automatically, and that C-module input messages (Msg_C readers) are not yet covered and still require retaining the source in a persistent scope (tracked as a follow-up).
Explicitly ignore the ReadFunctor move constructor and copy/move assignment operators in generated Python interfaces. Python has no meaningful call shape for these C++ ownership operations. SWIG consequently discarded them while emitting warning 509 for the move constructor and warning 362 for the assignment operators once per message payload, flooding clean-build output. Keeping these operations available to C++ while excluding them at the SWIG boundary removes the warnings without globally disabling diagnostic classes or changing ReadFunctor ownership behavior. Add a generator regression test verifying that all three ignore directives are included in generated message interfaces.
Exclude ReadFunctor::setSource() from generated Python bindings. This method accepts a raw owner token and reference-management callbacks and is intended solely for the internal SWIG keep-alive bridge. Exposing it as a public Python method allowed callers to clear the source owner while leaving the reader linked to its payload and header. The source could then be garbage collected, recreating the use-after-free fixed by issue AVSLab#676. Keep setSource() available to the SWIG extension implementation while hiding it from the generated Python API. Extend the generator regression test to verify that the ignore directive is emitted.
schaubh
force-pushed
the
feature/bsk-676--message-keepalive-refcount
branch
from
01eb9d5 to
864e5f6
Compare
schaubh
added
enhancement
schaubh
reviewed
Jul 12, 2026
schaubh
left a comment
schaubh
left a comment
Contributor
There was a problem hiding this comment.
I pulled this branch, rebased on develop, addressed a range of review feedback that came up. You can see my commits. Thanks for your thoughts on this fix. All tests pass now.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 864e5f6a6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[#676] Keep subscribed stand-alone messages alive (Python refcount bridge)
Draft — requesting design feedback before marking ready. @juan-g-bonilla @sassy-asjp — this implements the callback-bridge approach discussed in #676; I'd value your review of the C++ rule-of-five and the GIL handling before this goes further.
Problem
Fixes #676. When Python subscribes a reader to a stand-alone message
(
reader.subscribeTo(msg)), the C++ side stores only raw pointers into the message'spayload/header. If the Python
msggoes out of scope, it is garbage-collected and thesubscriber reads freed memory — silent undefined behavior. This is the root cause behind the
recurring
.disown/keep-a-reference workarounds (e.g. #1107).Approach
An opaque C-callback bridge, matching @juan-g-bonilla's suggestion in the thread:
ReadFunctor<T>gainsvoid* sourceHandleplusacquire/releasefunction pointers(plain C types).
messaging.hstays free of<Python.h>— a no-Python C/C++ build seesnull callbacks and pays nothing.
Py_INCREF/Py_DECREFlogic lives only in the SWIG layer (newMessaging.ih), in twoGIL-safe callbacks installed on the functor by the Python
subscribeTowrapper after theC++ subscribe completes.
ReadFunctorgets a hand-written rule-of-five (destructor releases; copy acquires;move transfers; copy-assign releases-then-acquires with self- and pointer-equality guards).
This is required because a
ReadFunctoris usually a C++ module member, so SWIG%extenddestructors never run for it — only a real C++ destructor does. Moves arenoexceptsostd::vector<ReadFunctor>reallocation moves rather than copies.Py_IsInitialized()(avoids the hang-until-exit atinterpreter finalization) and brackets the refcount op with
PyGILState_Ensure/Release.Why not the SWIG built-ins
%feature("ref"/"unref"),thisown,%newobjectall control who deletes the C++ object —the inverse of what's needed here — and don't fire for C++-instantiated members.
Scope
ReadFunctorreaders subscribing to both C++Message<T>and CMsg_Csources (covers the issue's reproduction and the common module case).
Msg_C-as-subscriber case (a C module'sdataInMsg).Msg_Cis a plainC struct with no destructor and no reader/writer distinction, so there is no safe hook to
drop the reference. This needs its own design (side-table + a SWIG
__del__hook) and isleft as a follow-up. The existing Removal of .disown in scenarioRoboticArm.py led to angles not being updated #1107 Python-side workarounds remain in place for that case.
Testing
Built and tested on Linux / gcc 16 (CachyOS). New regression test
src/architecture/messaging/_UnitTest/test_standaloneMsgKeepAlive.pysubscribes a C++ readerto a stand-alone message created in a helper scope, forces
gc.collect(), runs the sim, andasserts the data survived (this read freed/garbage memory pre-fix); plus a test that
unsubscribe()releases the keep-alive cleanly.Verified green (≈214 tests, 0 failures):
test_standaloneMsgKeepAlive(new)messaging/_UnitTest(recorders, unsubscribe, C/C++ paths)std::vector<ReadFunctor>reallocation)SWIG macro note (for reviewers)
The keep-alive comments inside the
INSTANTIATE_TEMPLATESmacro deliberately avoid literal#and%characters — inside a SWIG%definemacro these are the stringize/directiveoperators and corrupt the generated wrappers.
Files
src/architecture/messaging/messaging.h—ReadFunctormembers + rule-of-five (Python-free).src/architecture/messaging/newMessaging.ih— GIL-safe callbacks +_install_keepalive+subscribeTohook (both source paths).src/architecture/messaging/_UnitTest/test_standaloneMsgKeepAlive.py— new test.docs/source/Support/bskKnownIssues.rst+ release-note snippet.