diff --git a/.github/workflows/publish-container.yml b/.github/workflows/publish-container.yml index 4ddfe88a0e4..eeaba693836 100644 --- a/.github/workflows/publish-container.yml +++ b/.github/workflows/publish-container.yml @@ -5,6 +5,16 @@ on: tags: - "v*" workflow_dispatch: + inputs: + release_tag: + description: "Existing release tag to build without moving it, such as v2.12.2" + required: true + type: string + publish_latest: + description: "Also update latest when recovering this release" + required: false + default: false + type: boolean permissions: contents: read @@ -14,12 +24,55 @@ env: IMAGE_GHCR: ghcr.io/${{ github.repository }} concurrency: - group: publish-container-${{ github.ref }} - cancel-in-progress: true + group: publish-container-${{ inputs.release_tag || github.ref_name }} + cancel-in-progress: false jobs: + release: + name: Resolve Release Tag + runs-on: ubuntu-24.04 + permissions: + contents: read + outputs: + tag: ${{ steps.tag.outputs.tag }} + sha: ${{ steps.source.outputs.sha }} + steps: + - name: Validate release tag + id: tag + shell: bash + env: + RELEASE_TAG: ${{ inputs.release_tag || github.ref_name }} + run: | + set -euo pipefail + if [[ ! "$RELEASE_TAG" =~ ^v[0-9][A-Za-z0-9._-]*$ ]]; then + echo "::error::An existing version tag is required, such as v2.12.2" + exit 1 + fi + git check-ref-format "refs/tags/$RELEASE_TAG" + printf 'tag=%s\n' "$RELEASE_TAG" >> "$GITHUB_OUTPUT" + + - name: Checkout tagged source + uses: actions/checkout@v5 + with: + ref: refs/tags/${{ steps.tag.outputs.tag }} + + - name: Verify source version and resolve commit + id: source + shell: bash + env: + RELEASE_TAG: ${{ steps.tag.outputs.tag }} + run: | + set -euo pipefail + version=$(tr -d '\r\n' < docs/source/bskVersion.txt) + if [[ "$RELEASE_TAG" != "v$version" ]]; then + echo "::error::Release tag does not match the tagged source version" + exit 1 + fi + printf 'sha=%s\n' "$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT" + build: name: Build and Push (${{ matrix.arch }}) + needs: release runs-on: ${{ matrix.runner }} timeout-minutes: 180 strategy: @@ -36,6 +89,22 @@ jobs: steps: - name: Checkout uses: actions/checkout@v5 + with: + ref: ${{ needs.release.outputs.sha }} + + - name: Free runner disk for container build + shell: bash + run: | + set -euo pipefail + echo "Disk space before cleanup:" + df -h + sudo rm -rf /usr/local/lib/android + sudo rm -rf /usr/share/dotnet + sudo rm -rf /opt/ghc + sudo rm -rf /opt/hostedtoolcache/CodeQL + docker system prune -af + echo "Disk space after cleanup:" + df -h - name: Set up Docker Buildx uses: docker/setup-buildx-action@v3 @@ -58,10 +127,14 @@ jobs: latest=false suffix=-${{ matrix.arch }} tags: | - type=ref,event=tag - type=raw,value=latest,enable=${{ startsWith(github.ref, 'refs/tags/v') }} + type=raw,value=${{ needs.release.outputs.tag }} + type=raw,value=latest,enable=${{ github.event_name == 'push' || inputs.publish_latest }} + labels: | + org.opencontainers.image.revision=${{ needs.release.outputs.sha }} + org.opencontainers.image.version=${{ needs.release.outputs.tag }} - name: Build and push arch image + id: image uses: docker/build-push-action@v6 with: context: . @@ -76,11 +149,31 @@ jobs: # Cache export is best-effort and must not invalidate a published image. cache-to: type=gha,mode=max,scope=container-${{ matrix.arch }},ignore-error=true + - name: Verify published image + shell: bash + env: + IMAGE_TAGS: ${{ steps.meta.outputs.tags }} + IMAGE_DIGEST: ${{ steps.image.outputs.digest }} + BSK_RELEASE_TAG: ${{ needs.release.outputs.tag }} + run: | + set -euo pipefail + image_tag=${IMAGE_TAGS%%$'\n'*} + docker run --rm -e BSK_RELEASE_TAG "$image_tag@$IMAGE_DIGEST" python -c ' + import os + import Basilisk + from Basilisk.architecture import messaging + from Basilisk.simulation import camera, spacecraft, vizInterface + expected = os.environ["BSK_RELEASE_TAG"][1:] + assert Basilisk.__version__ == expected, (Basilisk.__version__, expected) + assert all(Basilisk.hasBuildFeature(name) for name in ("opNav", "mujoco", "vizInterface")) + print("Container import and version checks passed:", Basilisk.__version__) + ' + publish: name: Publish Multi-Arch Manifests runs-on: ubuntu-24.04 timeout-minutes: 30 - needs: build + needs: [release, build] steps: - name: Set up Docker Buildx @@ -100,19 +193,32 @@ jobs: with: images: | ${{ env.IMAGE_GHCR }} + flavor: latest=false tags: | - type=ref,event=tag - type=raw,value=latest,enable=${{ startsWith(github.ref, 'refs/tags/v') }} + type=raw,value=${{ needs.release.outputs.tag }} + type=raw,value=latest,enable=${{ github.event_name == 'push' || inputs.publish_latest }} - name: Create and push manifest lists shell: bash + env: + IMAGE_TAGS: ${{ steps.meta.outputs.tags }} + RELEASE_TAG: ${{ needs.release.outputs.tag }} run: | set -euo pipefail + release_image="${IMAGE_GHCR,,}:$RELEASE_TAG" while IFS= read -r tag; do [[ -z "$tag" ]] && continue docker buildx imagetools create \ -t "$tag" \ - "$tag-amd64" \ - "$tag-arm64" + "$release_image-amd64" \ + "$release_image-arm64" docker buildx imagetools inspect "$tag" - done <<< "${{ steps.meta.outputs.tags }}" + docker buildx imagetools inspect "$tag" --raw | python3 -c ' + import json, sys + manifest = json.load(sys.stdin) + architectures = {item["platform"]["architecture"] for item in manifest["manifests"] + if item.get("platform", {}).get("os") == "linux"} + assert architectures == {"amd64", "arm64"}, architectures + print("Verified Linux architectures:", ", ".join(sorted(architectures))) + ' + done <<< "$IMAGE_TAGS" diff --git a/docs/source/Support/Developer/CodingGuidlines.rst b/docs/source/Support/Developer/CodingGuidlines.rst index 970008769c4..9264e024e06 100644 --- a/docs/source/Support/Developer/CodingGuidlines.rst +++ b/docs/source/Support/Developer/CodingGuidlines.rst @@ -18,7 +18,7 @@ the coding guidelines developed by Stroustrup and Sutter. **Python foundational guidelines**: the `PEP 8 `__ guidelines are to be -followed for python code development. +followed for Python code development, with the exceptions described below. The both of these foundational sets of guidelines are extensive and where the Basilisk document is silent these guidelines will prevail. @@ -278,9 +278,12 @@ C/C++ Exceptions Python Exceptions ----------------- -- General-purpose Python implementation and tooling names are to follow - PEP 8. This includes using ``under_scored`` names for functions, methods, - local variables, and parameters. +- Python functions, methods, local variables, and parameters may use either + ``snake_case`` or ``lowerCamelCase``. This applies to general-purpose + implementation and tooling as well as BSK architecture code. Follow the + established convention in the surrounding file or API, and avoid mixing + naming styles within the same interface. Do not rename existing interfaces + solely to change naming style. - Public BSK module attributes, message variables, mathematical quantities, and names shared across C/C++ and Python interfaces are to retain the established Basilisk naming conventions. This includes lower ``camelCase`` diff --git a/docs/source/Support/bskKnownIssuesSnippets/1226-cpp-message-redirection.rst b/docs/source/Support/bskKnownIssuesSnippets/1226-cpp-message-redirection.rst index 376fc5bec54..f601807a331 100644 --- a/docs/source/Support/bskKnownIssuesSnippets/1226-cpp-message-redirection.rst +++ b/docs/source/Support/bskKnownIssuesSnippets/1226-cpp-message-redirection.rst @@ -1,8 +1,16 @@ -- Since 2.12.0, assigning a C++ module output to a stand-alone message copied - its initial contents instead of redirecting subsequent writes. This left - shared circles messages in the OpNav scenarios at zero despite successful - image processing. Python output assignment now redirects reads and writes - again and retains the gateway for producers, readers, and recorders, including - inputs connected through module registration methods. Native C++ copies and - moves of redirected messages take independent snapshots without consuming the - gateway payload. +- GitHub issue 1226: Fixed C++ output redirection and message lifetimes, + restoring shared OpNav outputs affected since 2.12.0. Message-vector access + now preserves the correct source owner across later module imports, preventing + premature deletion and false deleted-module errors. +- Configure redirection before connecting readers or recorders, and keep modules + alive during simulation. Use stand-alone gateways for redirected feedback; + native ownership cycles remain unsupported and are not automatically detected. + Redirecting through raw pointer-vector copies or slices raises + ``ReferenceError``; use the original module output or a Python list selection. + Keep the module's output-vector entries intact. +- Camera and Vizard image buffers are valid only until the producer replaces + them. Message recorders do not archive image bytes; save images or copy their + bytes while current. +- Fixed Python iterator crashes and numeric-vector access errors that depended + on module import order, including storage-message reads and :ref:`linkBudget` + lookup tables. diff --git a/docs/source/Support/bskKnownIssuesSnippets/container-release-recovery.rst b/docs/source/Support/bskKnownIssuesSnippets/container-release-recovery.rst new file mode 100644 index 00000000000..c459e1b3f04 --- /dev/null +++ b/docs/source/Support/bskKnownIssuesSnippets/container-release-recovery.rst @@ -0,0 +1,2 @@ +- Container release builds now free runner disk space and support recovery from + an existing release tag without moving it. diff --git a/docs/source/Support/bskReleaseNotesSnippets/container-release-recovery.rst b/docs/source/Support/bskReleaseNotesSnippets/container-release-recovery.rst new file mode 100644 index 00000000000..216d449a529 --- /dev/null +++ b/docs/source/Support/bskReleaseNotesSnippets/container-release-recovery.rst @@ -0,0 +1 @@ +- Container release builds now free runner disk space before Docker setup and support rebuilding an existing release tag without moving it, with image-version and multi-architecture manifest checks. diff --git a/docs/source/Support/bskReleaseNotesSnippets/cpp-message-module-owner.rst b/docs/source/Support/bskReleaseNotesSnippets/cpp-message-module-owner.rst new file mode 100644 index 00000000000..c685fbebf0d --- /dev/null +++ b/docs/source/Support/bskReleaseNotesSnippets/cpp-message-module-owner.rst @@ -0,0 +1 @@ +- Fixed a lifetime issue when redirecting a C++ output to another module's embedded output: the source module now remains alive while its storage is used by the redirected output, readers, or recorders. This includes outputs accessed through temporary MuJoCo body and site proxies. Redirecting to an output whose module has already been deleted raises ``ReferenceError``. diff --git a/docs/source/Support/bskReleaseNotesSnippets/cpp-message-vector-owner.rst b/docs/source/Support/bskReleaseNotesSnippets/cpp-message-vector-owner.rst new file mode 100644 index 00000000000..e0ece3cd88b --- /dev/null +++ b/docs/source/Support/bskReleaseNotesSnippets/cpp-message-vector-owner.rst @@ -0,0 +1,2 @@ +- Fixed C++ output redirection through message vectors so indexing, front/back access, and iteration preserve the actual module owner, including iterator copies and movement after later module imports. +- Redirecting to a borrowed message with an unknown storage owner, including elements of raw pointer-vector copies and slices, now raises ``ReferenceError``. Use the original module-owned output, a stand-alone message, or a selection from ``list(module.outputMsgs)``. diff --git a/docs/source/Support/bskReleaseNotesSnippets/message-lifetime-contract.rst b/docs/source/Support/bskReleaseNotesSnippets/message-lifetime-contract.rst new file mode 100644 index 00000000000..933669922de --- /dev/null +++ b/docs/source/Support/bskReleaseNotesSnippets/message-lifetime-contract.rst @@ -0,0 +1,2 @@ +- Documented message-lifetime limits for output redirection: configure wiring before simulation, retain participating modules, and use stand-alone gateways for redirected feedback connections. Native ownership cycles remain unsupported and are not automatically detected. +- Clarified that camera and Vizard image buffers belong to their producers and must be consumed before replacement; message recorders do not archive the image bytes. diff --git a/docs/source/Support/bskReleaseNotesSnippets/numeric-vector-bindings.rst b/docs/source/Support/bskReleaseNotesSnippets/numeric-vector-bindings.rst new file mode 100644 index 00000000000..268c09aa7ac --- /dev/null +++ b/docs/source/Support/bskReleaseNotesSnippets/numeric-vector-bindings.rst @@ -0,0 +1 @@ +- Fixed import-order-dependent numeric-vector binding errors affecting storage-message reads after importing :ref:`simpleInstrument`, ``zeroWindModel.VectorDouble``, and :ref:`linkBudget` lookup tables. Indexing, slicing, and mutation now work with the shared messaging vector without changing the native ABI. diff --git a/docs/source/Support/bskReleaseNotesSnippets/swig-iterator-gil.rst b/docs/source/Support/bskReleaseNotesSnippets/swig-iterator-gil.rst new file mode 100644 index 00000000000..8686079945e --- /dev/null +++ b/docs/source/Support/bskReleaseNotesSnippets/swig-iterator-gil.rst @@ -0,0 +1 @@ +- Fixed Python interpreter aborts in shared SWIG iterator element access and boundary-error handling that depended on module import order. Native class layouts and the messaging ABI are unchanged. diff --git a/src/architecture/_GeneralModuleFiles/swig_conly_data.i b/src/architecture/_GeneralModuleFiles/swig_conly_data.i index 0dc0c65118d..236710498ac 100644 --- a/src/architecture/_GeneralModuleFiles/swig_conly_data.i +++ b/src/architecture/_GeneralModuleFiles/swig_conly_data.i @@ -26,6 +26,99 @@ %include "stdint.i" %include "architecture/messaging/pythonSourceOwner.ih" +// Capture the owning proxy only for instance methods, including member getters. +// The shared local also exists for free/static Message-returning functions, where +// it stays null. This avoids treating their first argument as an owning instance. +%typemap(check) SWIGTYPE *self (PyObject *_global_bskMessageParent = nullptr) { + _global_bskMessageParent = $self; + (void)_global_bskMessageParent; +} + +%fragment("bsk_message_parent", "header") %{ +// Borrowed MuJoCo and configuration proxies retain their native storage owner +// through this chain. Return an owned reference to its terminal Python owner. +static PyObject* _bsk_message_storage_owner(PyObject* parent) { + PyObject* owner = PyObject_GetAttrString(parent, "_swig_bsk_owner"); + if (!owner) { + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) return nullptr; + PyErr_Clear(); + Py_INCREF(parent); + return parent; + } + if (Py_EnterRecursiveCall(" while resolving a message storage owner")) { + Py_DECREF(owner); + return nullptr; + } + PyObject* result = _bsk_message_storage_owner(owner); + Py_LeaveRecursiveCall(); + Py_DECREF(owner); + return result; +} + +// Flatten borrowed container views to their actual owner. A stand-alone pointer +// vector does not own its elements and must not masquerade as their owner. +static int _bsk_remember_message_parent(PyObject* proxy, PyObject* parent) { + PyObject* owner = _bsk_message_storage_owner(parent); + if (!owner) return -1; + PyObject* reference = PyObject_GetAttrString(owner, "_bskMessageOwner"); + if (!reference) { + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { + Py_DECREF(owner); + return -1; + } + PyErr_Clear(); + PyObject* pointerVector = PyObject_GetAttrString(owner, "_bskPointerVector"); + if (pointerVector) { + int isPointerVector = PyObject_IsTrue(pointerVector); + Py_DECREF(pointerVector); + if (isPointerVector != 0) { + Py_DECREF(owner); + return isPointerVector < 0 ? -1 : 0; + } + } else { + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { + Py_DECREF(owner); + return -1; + } + PyErr_Clear(); + } + reference = PyWeakref_NewRef(owner, nullptr); + } + Py_DECREF(owner); + if (!reference) return -1; + PyObject* name = PyUnicode_FromString("_bskMessageOwner"); + int status = name ? PyObject_GenericSetAttr(proxy, name, reference) : -1; + Py_XDECREF(name); + Py_DECREF(reference); + return status; +} +%} + +%typemap(out, fragment="bsk_message_parent") Message * (PyObject *_global_bskMessageParent = nullptr) { + $result = SWIG_NewPointerObj(SWIG_as_voidptr($1), $descriptor, $owner | %newpointer_flags); + if ($result && $1 && !$owner && _global_bskMessageParent) { + if (_bsk_remember_message_parent($result, _global_bskMessageParent) < 0) { + Py_CLEAR($result); + SWIG_fail; + } + } +} + +%typemap(out) Message & = Message *; +%typemap(out) const Message & = Message *; + +// Only borrowed member vectors have an enclosing storage owner. Copies and +// slices own a new array but not its pointees, so do not inherit that owner. +%typemap(out, fragment="bsk_message_parent") std::vector * (PyObject *_global_bskMessageParent = nullptr) { + $result = SWIG_NewPointerObj(SWIG_as_voidptr($1), $descriptor, $owner | %newpointer_flags); + if ($result && $1 && !$owner && _global_bskMessageParent) { + if (_bsk_remember_message_parent($result, _global_bskMessageParent) < 0) { + Py_CLEAR($result); + SWIG_fail; + } + } +} + // Preserve the original Python argument for the Message member setter below. %typemap(in) Message * (void *pointer = nullptr, PyObject *messageOwner_$1 = nullptr) { int result = SWIG_ConvertPtr($input, &pointer, $descriptor, $disown | %convertptr_flags); @@ -34,15 +127,29 @@ } $1 = reinterpret_cast<$1_ltype>(pointer); messageOwner_$1 = $input; + (void)messageOwner_$1; // Only member setters consume the saved Python owner. } // Python output assignment connects and retains a gateway; native assignment copies a value. -%typemap(memberin, fragment="bsk_python_source_owner_init") Message { +%typemap(memberin, fragment="bsk_python_source_owner_init,bsk_message_parent") Message { if (!$input) { SWIG_exception_fail(SWIG_ValueError, "A message gateway cannot be None"); } - $1.redirectTo(*$input, messageOwner_$input, - &_bsk_readfunctor_acquire, &_bsk_readfunctor_release); + PyObject *storageOwner = PyObject_CallMethod(messageOwner_$input, "_redirection_owner", nullptr); + if (!storageOwner) { + SWIG_fail; + } + PyObject *enclosingOwner = _global_bskMessageParent + ? _bsk_message_storage_owner(_global_bskMessageParent) : nullptr; + if (_global_bskMessageParent && !enclosingOwner) { + Py_DECREF(storageOwner); + SWIG_fail; + } + $1.redirectTo(*$input, storageOwner, + &_bsk_readfunctor_acquire, &_bsk_readfunctor_release, + enclosingOwner); + Py_XDECREF(enclosingOwner); + Py_DECREF(storageOwner); } %define ARRAYASLIST(type, fromfunc, asfunc) diff --git a/src/architecture/_GeneralModuleFiles/swig_python_iterators.ih b/src/architecture/_GeneralModuleFiles/swig_python_iterators.ih new file mode 100644 index 00000000000..afe690e55eb --- /dev/null +++ b/src/architecture/_GeneralModuleFiles/swig_python_iterators.ih @@ -0,0 +1,17 @@ +/* + * Copyright (c) 2026, Autonomous Vehicle Systems Lab, University of Colorado at Boulder + * Distributed under the ISC license; see LICENSE. + */ + +// value() creates Python objects; movement methods can set StopIteration via +// the Python C API. Keep the GIL in every generated entry point, including the +// arithmetic operators that call those movement methods. Apply this policy +// before std_vector.i or another container declares SwigPyIterator. +%nothread swig::SwigPyIterator::value; +%nothread swig::SwigPyIterator::incr; +%nothread swig::SwigPyIterator::decr; +%nothread swig::SwigPyIterator::advance; +%nothread swig::SwigPyIterator::operator +=; +%nothread swig::SwigPyIterator::operator -=; +%nothread swig::SwigPyIterator::operator +; +%nothread swig::SwigPyIterator::operator -; diff --git a/src/architecture/messaging/_UnitTest/test_cppMessageRedirection.py b/src/architecture/messaging/_UnitTest/test_cppMessageRedirection.py index abf7718cb1f..ce55562e0ec 100644 --- a/src/architecture/messaging/_UnitTest/test_cppMessageRedirection.py +++ b/src/architecture/messaging/_UnitTest/test_cppMessageRedirection.py @@ -9,8 +9,10 @@ import numpy as np import pytest +from Basilisk import hasBuildFeature from Basilisk.architecture import messaging from Basilisk.moduleTemplates import cppModuleTemplate +from Basilisk.simulation import dualHingedRigidBodyStateEffector, spacecraft, twoHingeDamper @pytest.mark.parametrize("subscribe_to_output", [False, True]) @@ -92,11 +94,17 @@ def test_cpp_output_retains_gateway(): @pytest.mark.parametrize("consumer_kind", ["subscriber", "cpp_reader", "c_reader", "recorder", "direct_recorder", "payload"]) @pytest.mark.parametrize("replace_gateway", [False, True]) -def test_cpp_output_consumers_retain_original_gateway(consumer_kind, replace_gateway): +@pytest.mark.parametrize("embedded_gateway", [False, True]) +def test_cpp_output_consumers_retain_original_gateway(consumer_kind, replace_gateway, embedded_gateway): """Consumers retain their original storage after producer deletion or redirection.""" producer = cppModuleTemplate.CppModuleTemplate() - gateway = messaging.CModuleTemplateMsg() - reference = weakref.ref(gateway) + if embedded_gateway: + source = cppModuleTemplate.CppModuleTemplate() + gateway = source.dataOutMsg + reference = weakref.ref(source) + else: + source = gateway = messaging.CModuleTemplateMsg() + reference = weakref.ref(gateway) producer.dataOutMsg = gateway timestamp = 10 # [ns] producer.UpdateState(timestamp) @@ -114,7 +122,7 @@ def test_cpp_output_consumers_retain_original_gateway(consumer_kind, replace_gat consumer = messaging.CModuleTemplateMsgRecorder(producer.dataOutMsg) else: consumer = producer.dataOutMsg.read() - del gateway + del gateway, source if replace_gateway: producer.dataOutMsg = messaging.CModuleTemplateMsg() producer.UpdateState(timestamp) @@ -141,6 +149,170 @@ def test_cpp_output_consumers_retain_original_gateway(consumer_kind, replace_gat assert reference() is None +@pytest.mark.parametrize("replace_gateway", [False, True]) +def test_cpp_output_redirection_retains_source_module(replace_gateway): + """An embedded gateway keeps its module alive until the destination releases it.""" + source = cppModuleTemplate.CppModuleTemplate() + destination = cppModuleTemplate.CppModuleTemplate() + reference = weakref.ref(source) + destination.dataOutMsg = source.dataOutMsg + del source + gc.collect() + # Check lifetime before writing, so a regression cannot access freed storage. + assert reference() is not None + + unrelated = cppModuleTemplate.CppModuleTemplate() + unrelated.setUpdateCounter(100.0) # [-] + timestamp = 10 # [ns] + unrelated.UpdateState(timestamp) + destination.UpdateState(timestamp) + np.testing.assert_array_equal(reference().dataOutMsg.read().dataVector, [1.0, 0.0, 0.0]) + np.testing.assert_array_equal(unrelated.dataOutMsg.read().dataVector, [101.0, 0.0, 0.0]) + + if replace_gateway: + destination.dataOutMsg = messaging.CModuleTemplateMsg() + else: + del destination + gc.collect() + assert reference() is None + + +def test_cpp_output_redirection_keeps_original_embedded_storage(): + """Rerouting the source output does not change the destination's storage owner.""" + source = cppModuleTemplate.CppModuleTemplate() + destination = cppModuleTemplate.CppModuleTemplate() + reference = weakref.ref(source) + destination.dataOutMsg = source.dataOutMsg + gateway = messaging.CModuleTemplateMsg() + source.dataOutMsg = gateway + reader = destination.dataOutMsg.addSubscriber() + del source + gc.collect() + assert reference() is not None + + timestamp = 10 # [ns] + destination.UpdateState(timestamp) + np.testing.assert_array_equal(reader().dataVector, [1.0, 0.0, 0.0]) + np.testing.assert_array_equal(gateway.read().dataVector, [0.0, 0.0, 0.0]) + del destination + gc.collect() + assert reference() is not None + del reader + gc.collect() + assert reference() is None + + +def test_cpp_output_redirection_rejects_deleted_source_module(): + """A saved non-owning output proxy cannot redirect into a deleted module.""" + source = cppModuleTemplate.CppModuleTemplate() + output = source.dataOutMsg + reference = weakref.ref(source) + del source + gc.collect() + assert reference() is None + destination = cppModuleTemplate.CppModuleTemplate() + with pytest.raises(ReferenceError, match="module owning this message has been deleted"): + destination.dataOutMsg = output + + +@pytest.fixture(params=["origin", "center_of_mass", "body_site", "scene_site", "scene_round_trip"]) +def mujoco_output_scene(request): + """Create scenes with output accessors that leave no intermediate proxies alive.""" + if not hasBuildFeature("mujoco"): + pytest.skip("Requires Basilisk built with --mujoco True") + from Basilisk.simulation import mujoco + + accessors = { + "origin": lambda owner: owner.getBody("hub").getOrigin().stateOutMsg, + "center_of_mass": lambda owner: owner.getBody("hub").getCenterOfMass().stateOutMsg, + "body_site": lambda owner: owner.getBody("hub").getSite("site").stateOutMsg, + "scene_site": lambda owner: owner.getSite("site").stateOutMsg, + "scene_round_trip": lambda owner: owner.getBody("hub").getScene().getBody("hub").getOrigin().stateOutMsg, + } + + def create_scene(): + scene = mujoco.MJScene( + '' + ) + return scene, accessors[request.param] + + return create_scene + + +@pytest.mark.parametrize("replace_gateway", [False, True]) +def test_cpp_output_redirection_retains_mujoco_scene(mujoco_output_scene, replace_gateway): + """Temporary MuJoCo accessor chains retain the scene until redirection ends.""" + scene, get_output = mujoco_output_scene() + reference = weakref.ref(scene) + destination = spacecraft.Spacecraft() + destination.scStateOutMsg = get_output(scene) + del scene + gc.collect() + # Check lifetime before accessing native storage if retention regresses. + assert reference() is not None + + payload = messaging.SCStatesMsgPayload() + payload.r_BN_N = [1.0, 2.0, 3.0] # [m] + destination.scStateOutMsg.write(payload) + np.testing.assert_array_equal(get_output(reference()).read().r_BN_N, payload.r_BN_N) + + if replace_gateway: + destination.scStateOutMsg = messaging.SCStatesMsg() + else: + del destination + gc.collect() + assert reference() is None + + +def test_cpp_output_redirection_rejects_deleted_mujoco_scene(mujoco_output_scene): + """A saved MuJoCo output does not keep its scene alive before redirection.""" + scene, get_output = mujoco_output_scene() + output = get_output(scene) + reference = weakref.ref(scene) + del scene + gc.collect() + assert reference() is None + + destination = spacecraft.Spacecraft() + with pytest.raises(ReferenceError, match="module owning this message has been deleted"): + destination.scStateOutMsg = output + + +def test_cpp_output_redirection_does_not_retain_its_own_mujoco_scene(mujoco_output_scene): + """Redirecting two outputs within a scene must not make the scene retain itself.""" + scene, get_output = mujoco_output_scene() + reference = weakref.ref(scene) + scene.getBody("hub").getOrigin().stateOutMsg = get_output(scene) + payload = messaging.SCStatesMsgPayload() + payload.r_BN_N = [1.0, 2.0, 3.0] # [m] + scene.getBody("hub").getOrigin().stateOutMsg.write(payload) + np.testing.assert_array_equal(get_output(scene).read().r_BN_N, payload.r_BN_N) + del scene + gc.collect() + assert reference() is None + + +@pytest.mark.parametrize("through_another_module", [False, True]) +def test_cpp_output_redirection_does_not_retain_its_own_module(through_another_module): + """Redirecting co-owned outputs must not make their module retain itself.""" + owner = twoHingeDamper.TwoHingeDamper() + reference = weakref.ref(owner) + if through_another_module: + intermediate = twoHingeDamper.TwoHingeDamper() + intermediate.phiTorqueOutMsg = owner.thetaTorqueOutMsg + owner.phiTorqueOutMsg = intermediate.phiTorqueOutMsg + del intermediate + else: + owner.phiTorqueOutMsg = owner.thetaTorqueOutMsg + payload = messaging.SingleActuatorMsgPayload() + payload.input = 3.0 # [N*m] + owner.phiTorqueOutMsg.write(payload) + assert owner.thetaTorqueOutMsg.read().input == payload.input + del owner + gc.collect() + assert reference() is None + + def test_cpp_output_redirection_chain_retains_gateway(): """Assigning a redirected output retains the terminal gateway independently.""" first = cppModuleTemplate.CppModuleTemplate() @@ -158,3 +330,146 @@ def test_cpp_output_redirection_chain_retains_gateway(): del second gc.collect() assert reference() is None + + +@pytest.mark.parametrize("accessor", ["index", "front", "back", "list_slice", "iter", "list", + "iterator_value", "iterator_copy", "iterator_advance", + "reverse_iterator", "iterator_previous", "slice_iter"]) +def test_cpp_output_vector_redirection(accessor): + """Temporary vector views resolve to the module that owns their messages.""" + source = dualHingedRigidBodyStateEffector.DualHingedRigidBodyStateEffector() + destination = spacecraft.Spacecraft() + reference = weakref.ref(source) + accessors = { + "index": lambda vector: vector[0], + "front": lambda vector: vector.front(), + "back": lambda vector: vector.back(), + "list_slice": lambda vector: list(vector)[:][0], + "iter": lambda vector: next(iter(vector)), + "list": lambda vector: list(vector)[0], + "iterator_value": lambda vector: vector.iterator().value(), + "iterator_copy": lambda vector: next(vector.iterator().copy()), + "iterator_advance": lambda vector: (vector.begin() + 1).value(), + "reverse_iterator": lambda vector: vector.rbegin().value(), + "iterator_previous": lambda vector: vector.end().previous(), + "slice_iter": lambda vector: next(iter(list(vector)[:])), + } + destination.scStateOutMsg = accessors[accessor](source.dualHingedRigidBodyConfigLogOutMsgs) + del source + gc.collect() + assert reference() is not None + payload = messaging.SCStatesMsgPayload() + payload.r_BN_N = [1.0, 2.0, 3.0] # [m] + destination.scStateOutMsg.write(payload) + index = 1 if accessor in ("back", "iterator_advance", "reverse_iterator", "iterator_previous") else 0 + np.testing.assert_array_equal( + reference().dualHingedRigidBodyConfigLogOutMsgs[index].read().r_BN_N, + payload.r_BN_N, + ) + del destination + gc.collect() + assert reference() is None + + +@pytest.mark.parametrize("iterate", [False, True]) +def test_cpp_output_vector_rejects_deleted_module(iterate): + """Keeping a vector view does not hide deletion of the owning module.""" + source = dualHingedRigidBodyStateEffector.DualHingedRigidBodyStateEffector() + vector = source.dualHingedRigidBodyConfigLogOutMsgs + output = next(iter(vector)) if iterate else vector[0] + assert getattr(output, "_bskMessageOwner", lambda: None)() is source + reference = weakref.ref(source) + del source + gc.collect() + assert reference() is None + destination = spacecraft.Spacecraft() + with pytest.raises(ReferenceError, match="module owning this message has been deleted"): + destination.scStateOutMsg = output + + +def test_cpp_output_iterator_rejects_deleted_module(): + """An iterator checks its weak module owner before accessing native storage.""" + source = dualHingedRigidBodyStateEffector.DualHingedRigidBodyStateEffector() + iterator = iter(source.dualHingedRigidBodyConfigLogOutMsgs) + # Check tagging before deleting storage so regressions cannot dereference it. + assert getattr(iterator, "_bskMessageOwner", lambda: None)() is source + reference = weakref.ref(source) + del source + gc.collect() + assert reference() is None + with pytest.raises(ReferenceError, match="module owning this message has been deleted"): + next(iterator) + + +def test_cpp_pointer_iterator_preserves_other_vector_semantics(): + """The shared iterator still returns numeric values and owning reader copies.""" + source = dualHingedRigidBodyStateEffector.DualHingedRigidBodyStateEffector() + next(iter(source.dualHingedRigidBodyConfigLogOutMsgs)) + numbers = messaging.DoubleVector([1.0, 2.0]) # [-] + assert list(numbers) == [1.0, 2.0] + assert (numbers.begin() + 1).value() == 2.0 + message = messaging.NavTransMsg() + readers = messaging.NavTransMsgInMsgsVector([message.addSubscriber()]) + reader = next(iter(readers)) + assert reader.thisown + assert getattr(reader, "_bskMessageOwner", None) is None + reader.unsubscribe() + assert readers[0].isSubscribedTo(message) + + +def test_cpp_feedback_using_standalone_gateway_is_supported(): + """A standalone gateway permits feedback without retaining the source module.""" + producer = cppModuleTemplate.CppModuleTemplate() + reference = weakref.ref(producer) + gateway = messaging.CModuleTemplateMsg() + producer.dataOutMsg = gateway + producer.dataInMsg.subscribeTo(gateway) + timestamp = 10 # [ns] + producer.UpdateState(timestamp) + np.testing.assert_array_equal(producer.dataInMsg().dataVector, [1.0, 0.0, 0.0]) + del producer + gc.collect() + assert reference() is None + + +@pytest.mark.parametrize("iterate", [False, True]) +@pytest.mark.parametrize("delete_source", [False, True]) +@pytest.mark.parametrize("copy_kind", ["constructor", "slice"]) +def test_cpp_unowned_pointer_vector_redirection_is_rejected(iterate, delete_source, copy_kind): + """A raw pointer-vector copy cannot establish ownership of its pointees.""" + source = dualHingedRigidBodyStateEffector.DualHingedRigidBodyStateEffector() + vector = source.dualHingedRigidBodyConfigLogOutMsgs + copied = messaging.SCStatesMsgOutMsgsPtrVector(vector) if copy_kind == "constructor" else vector[:] + output = next(iter(copied)) if iterate else copied[0] + destination = spacecraft.Spacecraft() + original_address = destination.scStateOutMsg.getPayloadAddress() + with pytest.raises(ReferenceError, match="storage owner is unknown"): + destination.scStateOutMsg = output + if delete_source: + del source + gc.collect() + with pytest.raises(ReferenceError, match="storage owner is unknown"): + destination.scStateOutMsg = output + assert destination.scStateOutMsg.getPayloadAddress() == original_address + + +@pytest.mark.parametrize("iterate", [False, True]) +def test_cpp_mutated_pointer_vector_slice_has_no_owner(iterate): + """Replacing a raw slice element cannot attribute its storage to the original module.""" + source = dualHingedRigidBodyStateEffector.DualHingedRigidBodyStateEffector() + copied = source.dualHingedRigidBodyConfigLogOutMsgs[:] + foreign = messaging.SCStatesMsg() + reference = weakref.ref(foreign) + copied[0] = foreign + output = next(iter(copied)) if iterate else copied[0] + destination = spacecraft.Spacecraft() + original_address = destination.scStateOutMsg.getPayloadAddress() + # Check rejection while storage is live so older builds fail safely. + with pytest.raises(ReferenceError, match="storage owner is unknown"): + destination.scStateOutMsg = output + del foreign + gc.collect() + assert reference() is None + with pytest.raises(ReferenceError, match="storage owner is unknown"): + destination.scStateOutMsg = output + assert destination.scStateOutMsg.getPayloadAddress() == original_address diff --git a/src/architecture/messaging/_UnitTest/test_messageVectorRegistrations.py b/src/architecture/messaging/_UnitTest/test_messageVectorRegistrations.py new file mode 100644 index 00000000000..7f50ffde2cc --- /dev/null +++ b/src/architecture/messaging/_UnitTest/test_messageVectorRegistrations.py @@ -0,0 +1,176 @@ +# Copyright (c) 2026, Autonomous Vehicle Systems Lab, University of Colorado at Boulder +# Distributed under the ISC license; see LICENSE. + +"""Verify ordinary message wiring and pointer-vector ownership across imports.""" + +import os +from pathlib import Path +import subprocess +import sys + +import Basilisk +import pytest + + +@pytest.mark.skipif(not Basilisk.hasBuildFeature("vizInterface"), reason="Requires vizInterface") +@pytest.mark.parametrize("vizard_first", [False, True]) +@pytest.mark.parametrize("message_type, field", [("THROutputMsg", "thrInMsgs"), + ("RWConfigLogMsg", "rwInMsgs")]) +def test_shared_message_vectors_preserve_wiring(vizard_first, message_type, field): + """Temporary nested views accept wiring and raw pointer copies remain unowned.""" + script = r''' +import gc +import sys +import weakref + +if sys.argv[1] == "True": + from Basilisk.simulation import dataFileToViz, vizInterface +from Basilisk.architecture import messaging +if sys.argv[1] == "False": + from Basilisk.simulation import vizInterface, dataFileToViz + +message_type, field = sys.argv[2:] +message_class = getattr(messaging, message_type) + +def check_nested_wiring(): + consumer = vizInterface.VizInterface() + source = message_class() + state = messaging.SCStatesMsg() + entry = vizInterface.VizSpacecraftData() + consumer.scData.push_back(entry) + # Both intermediate spacecraft-data proxies disappear before the calls. + # The live enclosing module still owns the readers' native storage. + consumer.scData[0].scStateInMsg.subscribeTo(state) + getattr(consumer.scData[0], field).append(source.addSubscriber()) + assert consumer.scData[0].scStateInMsg.isSubscribedTo(state) + assert getattr(consumer.scData[0], field)[0].isSubscribedTo(source) + # Copying a configured spacecraft entry also retains its input sources. + entry.scStateInMsg.subscribeTo(state) + getattr(entry, field).append(source.addSubscriber()) + consumer.scData = vizInterface.VizSCVector([entry]) + assert consumer.scData[0].scStateInMsg.isSubscribedTo(state) + assert getattr(consumer.scData[0], field)[0].isSubscribedTo(source) + return [weakref.ref(consumer), weakref.ref(source), weakref.ref(state)] + +references = check_nested_wiring() +gc.collect() +assert all(reference() is None for reference in references) + +# Exercise both classes, including whichever is no longer the active SWIG +# registration after the final import. +for module in (messaging, dataFileToViz): + source = message_class() + reader_vector = getattr(module, message_type + "InMsgsVector")() + reader_vector.append(source.addSubscriber()) + assert reader_vector[0].isSubscribedTo(source) + reader_vector.clear() + + pointers = getattr(module, message_type + "OutMsgsPtrVector")([source]) + for output in (pointers[0], next(iter(pointers))): + try: + output._redirection_owner() + except ReferenceError as error: + assert "storage owner is unknown" in str(error) + else: + raise AssertionError("A raw pointer-vector copy claimed to own its pointee") +print("Shared-vector checks passed") +''' + environment = os.environ.copy() + package_parent = str(Path(Basilisk.__file__).resolve().parent.parent) + environment["PYTHONPATH"] = os.pathsep.join(filter(None, [package_parent, environment.get("PYTHONPATH")])) + environment["PYTHONMALLOC"] = "debug" + result = subprocess.run( + [sys.executable, "-X", "faulthandler", "-c", script, str(vizard_first), message_type, field], + env=environment, capture_output=True, text=True, timeout=120, # [s] + ) + assert result.returncode == 0, result.stdout + result.stderr + assert "Shared-vector checks passed" in result.stdout + + +@pytest.mark.parametrize("late_module", [ + "simpleInstrument", + pytest.param("dataFileToViz", marks=pytest.mark.skipif( + not Basilisk.hasBuildFeature("vizInterface"), reason="Requires vizInterface")), +]) +@pytest.mark.parametrize("operation", ["copy", "incr", "decr", "advance", + "add", "subtract", "iadd", "isub"]) +def test_message_iterator_owner_survives_late_import(late_module, operation): + """Late imports preserve iterator ownership, redirection, and deletion checks.""" + script = r''' +import gc +import importlib +import sys +import weakref + +import pytest +from Basilisk.architecture import messaging +from Basilisk.simulation import dualHingedRigidBodyStateEffector, spacecraft + +source = dualHingedRigidBodyStateEffector.DualHingedRigidBodyStateEffector() +reference = weakref.ref(source) +vector = source.dualHingedRigidBodyConfigLogOutMsgs +operation = sys.argv[2] +iterator = vector.end() if operation in ("decr", "isub") else vector.begin() + +# Import after constructing the iterator so SWIG replaces its shared wrapper +# registration while a live instance of the previous class still exists. +importlib.import_module("Basilisk.simulation." + sys.argv[1]) +operations = { + "copy": lambda: iterator.copy(), + "incr": lambda: iterator.incr(), + "decr": lambda: iterator.decr(), + "advance": lambda: iterator.advance(1), + "add": lambda: iterator + 1, + "subtract": lambda: iterator - (-1), + "iadd": lambda: iterator.__iadd__(1), + "isub": lambda: iterator.__isub__(1), +} +result = operations[operation]() +output = result.value() +index = 0 if operation == "copy" else 1 +address = vector[index].getPayloadAddress() +assert output.getPayloadAddress() == address +# Returned wrappers must themselves propagate ownership to further results. +assert result.copy().copy().value()._redirection_owner() is source +# Subtraction of iterators returns a distance, not an iterator to tag. +assert result - vector.begin() == index + +destination = spacecraft.Spacecraft() +destination.scStateOutMsg = output +del source, vector +gc.collect() +assert reference() is not None +payload = messaging.SCStatesMsgPayload() +payload.r_BN_N = [1.0, 2.0, 3.0] # [m] +destination.scStateOutMsg.write(payload) +assert list(reference().dualHingedRigidBodyConfigLogOutMsgs[index].read().r_BN_N) == [1.0, 2.0, 3.0] + +# Patching the newly registered class must leave other vector results alone. +numbers = messaging.DoubleVector([1.0, 2.0]) # [-] +assert (numbers.begin().copy() + 1).value() == 2.0 +assert numbers.end() - numbers.begin() == 2 +message = messaging.NavTransMsg() +readers = messaging.NavTransMsgInMsgsVector([message.addSubscriber()]) +reader = readers.begin().copy().value() +assert reader.thisown +reader.unsubscribe() +assert readers[0].isSubscribedTo(message) + +del destination +gc.collect() +assert reference() is None +for access in (result.value, result.copy, output._redirection_owner): + with pytest.raises(ReferenceError, match="module owning this message has been deleted"): + access() +print("Late-import iterator checks passed") +''' + environment = os.environ.copy() + package_parent = str(Path(Basilisk.__file__).resolve().parent.parent) + environment["PYTHONPATH"] = os.pathsep.join(filter(None, [package_parent, environment.get("PYTHONPATH")])) + environment["PYTHONMALLOC"] = "debug" + result = subprocess.run( + [sys.executable, "-X", "faulthandler", "-c", script, late_module, operation], + env=environment, capture_output=True, text=True, timeout=120, # [s] + ) + assert result.returncode == 0, result.stdout + result.stderr + assert "Late-import iterator checks passed" in result.stdout diff --git a/src/architecture/messaging/_UnitTest/test_numericVectorBindings.py b/src/architecture/messaging/_UnitTest/test_numericVectorBindings.py new file mode 100644 index 00000000000..a0d69349dc2 --- /dev/null +++ b/src/architecture/messaging/_UnitTest/test_numericVectorBindings.py @@ -0,0 +1,67 @@ +# Copyright (c) 2026, Autonomous Vehicle Systems Lab, University of Colorado at Boulder +# Distributed under the ISC license; see LICENSE. + +"""Exercise numeric vectors shared by payload and module SWIG bindings.""" + +import itertools +import os +from pathlib import Path +import subprocess +import sys + +import Basilisk +import pytest + + +@pytest.mark.parametrize("import_order", list(itertools.permutations( + ["simpleInstrument", "zeroWindModel", "linkBudget"] +))) +def test_storage_vectors_survive_module_imports(import_order): + """Message reads and lookup-table vectors work after each module import.""" + script = r''' +import importlib +import sys + +from Basilisk.architecture import messaging + +payload = messaging.DataStorageStatusMsgPayload() +payload.storedData = messaging.DoubleVector([400.0, 1200.0]) # [bit] +message = messaging.DataStorageStatusMsg().write(payload) +reader = messaging.DataStorageStatusMsgReader() +reader.subscribeTo(message) + +for module_name in sys.argv[1:]: + module = importlib.import_module("Basilisk.simulation." + module_name) + # Each read must use the current shared payload registration, not a proxy + # created before the import that happens to keep a working Python class. + for result in (message.read(), reader()): + values = result.storedData + assert values[0] == 400.0 # [bit] + assert values[-1] == 1200.0 # [bit] + assert list(values[:]) == [400.0, 1200.0] # [bit] + values[0] = 500.0 # [bit] + assert result.storedData[0] == 500.0 # [bit] + values[0] = 400.0 # [bit] + if module_name == "zeroWindModel": + values = module.VectorDouble([1.0, 2.0]) # [-] + assert values[0] == 1.0 + values[1] = 3.0 # [-] + assert list(values[:]) == [1.0, 3.0] + if module_name == "linkBudget": + table = module.LookupTable() + table.f_l = messaging.DoubleVector([2.2, 8.4]) # [GHz] + assert table.f_l[0] == 2.2 # [GHz] + table.f_l[1] = 8.5 # [GHz] + assert list(table.f_l[:]) == [2.2, 8.5] # [GHz] +print("Numeric-vector checks passed") +''' + environment = os.environ.copy() + package_parent = str(Path(Basilisk.__file__).resolve().parent.parent) + environment["PYTHONPATH"] = os.pathsep.join(filter(None, [package_parent, environment.get("PYTHONPATH")])) + environment["PYTHONMALLOC"] = "debug" + result = subprocess.run( + [sys.executable, "-X", "faulthandler", "-c", script, *import_order], + env=environment, capture_output=True, text=True, timeout=120, # [s] + ) + assert result.returncode == 0, result.stdout + result.stderr + assert "Numeric-vector checks passed" in result.stdout diff --git a/src/architecture/messaging/_UnitTest/test_swigIteratorGIL.py b/src/architecture/messaging/_UnitTest/test_swigIteratorGIL.py new file mode 100644 index 00000000000..1ce44e9be64 --- /dev/null +++ b/src/architecture/messaging/_UnitTest/test_swigIteratorGIL.py @@ -0,0 +1,81 @@ +# Copyright (c) 2026, Autonomous Vehicle Systems Lab, University of Colorado at Boulder +# Distributed under the ISC license; see LICENSE. + +"""Check shared SWIG iterator bindings under different module import orders.""" + +import os +from pathlib import Path +import subprocess +import sys + +import Basilisk +import pytest + + +@pytest.mark.parametrize("simulation_first", [False, True]) +def test_swig_iterator_holds_gil_across_import_orders(simulation_first, tmp_path): + """Allocation and boundary errors remain safe regardless of the active wrapper.""" + script = r''' +import importlib +import sys + +if sys.argv[1] == "True": + from Basilisk.utilities import SimulationBaseClass +from Basilisk.architecture import messaging +from Basilisk.simulation import dualHingedRigidBodyStateEffector +if sys.argv[1] == "False": + from Basilisk.utilities import SimulationBaseClass + +source = dualHingedRigidBodyStateEffector.DualHingedRigidBodyStateEffector() +vector = source.dualHingedRigidBodyConfigLogOutMsgs +iterator = vector.iterator() +assert iterator.value().getPayloadAddress() == vector[0].getPayloadAddress() +assert (iterator + 1).value().getPayloadAddress() == vector[1].getPayloadAddress() +assert iterator.copy().value().getPayloadAddress() == vector[0].getPayloadAddress() + +def expect_stop(operation): + try: + operation() + except StopIteration: + return + raise AssertionError("Expected StopIteration") + +exhausted = vector.iterator() +exhausted.incr(len(vector)) +expect_stop(exhausted.value) +expect_stop(exhausted.incr) +expect_stop(lambda: exhausted.advance(1)) +expect_stop(lambda: exhausted.__iadd__(1)) +expect_stop(lambda: exhausted + 1) +start = vector.iterator() +expect_stop(start.decr) +expect_stop(lambda: start.advance(-1)) +expect_stop(lambda: start.__isub__(1)) +expect_stop(lambda: start - 1) + +# Also exercise the threaded entry points directly, independent of which +# Python SwigPyIterator class was registered last in the shared SWIG runtime. +for name in ("Basilisk.architecture._py_sys_model", + "Basilisk.architecture.messaging._SCStatesMsgPayload", + "Basilisk.architecture.messaging._messagingSupport"): + native = importlib.import_module(name) + assert native.SwigPyIterator_value(iterator).getPayloadAddress() == vector[0].getPayloadAddress() + expect_stop(lambda: native.SwigPyIterator_value(exhausted)) + expect_stop(lambda: native.SwigPyIterator_incr(exhausted)) + expect_stop(lambda: native.SwigPyIterator_decr(start)) + +numbers = messaging.DoubleVector([1.0, 2.0]) # [-] +assert numbers.begin().value() == 1.0 +print("Iterator checks passed") +''' + environment = os.environ.copy() + package_parent = str(Path(Basilisk.__file__).resolve().parent.parent) + environment["PYTHONPATH"] = os.pathsep.join(filter(None, [package_parent, environment.get("PYTHONPATH")])) + environment["PYTHONMALLOC"] = "debug" + environment["MPLCONFIGDIR"] = str(tmp_path / "matplotlib") + result = subprocess.run( + [sys.executable, "-X", "faulthandler", "-c", script, str(simulation_first)], + env=environment, capture_output=True, text=True, timeout=120, # [s] + ) + assert result.returncode == 0, result.stdout + result.stderr + assert "Iterator checks passed" in result.stdout diff --git a/src/architecture/messaging/_msgKeepAlive.py b/src/architecture/messaging/_msgKeepAlive.py index 4b2f3b06c44..821c165849e 100644 --- a/src/architecture/messaging/_msgKeepAlive.py +++ b/src/architecture/messaging/_msgKeepAlive.py @@ -16,7 +16,12 @@ # OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. # -"""Keep-alive helpers for C-message (``Msg_C``) readers -- issue #1433. +"""Keep-alive helpers for C-message readers and C++ output-vector views. + +C++ pointer-vector views preserve a weak reference to their storage owner. +Output redirection promotes that reference to a native keep-alive; ordinary +subscriptions retain their existing behavior. Ownership cycles involving +native keep-alives are unsupported and are not detected by these helpers. 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 @@ -55,7 +60,81 @@ all run in the interpreter thread), so the registries need no additional locking. """ +import types import weakref +from functools import wraps + + +def _instance_attributes(value): + """Read stored attributes without invoking dynamic attribute lookup.""" + for cls in type(value).__mro__: + descriptor = vars(cls).get("__dict__") + if descriptor is not None: + if isinstance(descriptor, types.GetSetDescriptorType): + return descriptor.__get__(value, type(value)) + return {} + return {} + + +def preserveCppPointerVectorOwner(vector_type): + """Carry a pointer vector's weak module owner through its native iterators.""" + vector_type._bskPointerVector = True + if vars(vector_type).get("_bskOwnerAwarePointerVector", False): + return + + def wrap(method): + @wraps(method) + def with_owner(self, *args, **kwargs): + reference = _instance_attributes(self).get("_bskMessageOwner") + parent = reference() if reference is not None else None + if reference is not None and parent is None: + raise ReferenceError("The module owning this message has been deleted") + iterator = method(self, *args, **kwargs) + if reference is not None and iterator is not None: + _tag_cpp_pointer_iterator(iterator, reference) + return iterator + return with_owner + + for name in ("iterator", "begin", "end", "rbegin", "rend", "insert", "erase"): + setattr(vector_type, name, wrap(getattr(vector_type, name))) + vector_type._bskOwnerAwarePointerVector = True + + +def _tag_cpp_pointer_iterator(iterator, reference): + """Preserve SWIG iterator APIs while tagging borrowed message results.""" + object.__setattr__(iterator, "_bskMessageOwner", reference) + iterator_type = type(iterator) + if vars(iterator_type).get("_bskOwnerAwareIterator", False): + return + + def wrap(method, returns_iterator): + @wraps(method) + def with_owner(self, *args, **kwargs): + reference = _instance_attributes(self).get("_bskMessageOwner") + parent = reference() if reference is not None else None + if reference is not None and parent is None: + raise ReferenceError("The module owning this message has been deleted") + result = method(self, *args, **kwargs) + if reference is not None: + if returns_iterator and not isinstance(result, int): + # A late module import can replace SWIG's iterator class. + # Tag and wrap the returned class as well as the instance. + _tag_cpp_pointer_iterator(result, reference) + elif hasattr(type(result), "_storage_owner") and not result.thisown: + object.__setattr__(result, "_bskMessageOwner", reference) + return result + return with_owner + + # SWIG shares this iterator class with other vector types. Results without + # an owner tag, including owning value/reader copies, retain their semantics. + for name in ("value", "__next__", "previous"): + setattr(iterator_type, name, wrap(getattr(iterator_type, name), returns_iterator=False)) + # These methods return iterators, except subtraction of two iterators, + # which returns an integer distance and must retain that result unchanged. + for name in ("copy", "incr", "decr", "advance", "__add__", "__sub__", "__iadd__", "__isub__"): + setattr(iterator_type, name, wrap(getattr(iterator_type, name), returns_iterator=True)) + iterator_type._bskOwnerAwareIterator = True + #: Embedded ``Msg_C`` C-address (``int``) -> (weak owner handle, owner token). _owner_by_address = {} diff --git a/src/architecture/messaging/messaging.h b/src/architecture/messaging/messaging.h index 9d9ead1ae6f..0c9cad061eb 100644 --- a/src/architecture/messaging/messaging.h +++ b/src/architecture/messaging/messaging.h @@ -535,6 +535,7 @@ class Message : public MessageBase * @param owner Optional owner of target storage, borrowed on entry. * @param acquire Callback to retain owner; required when owner is supplied. * @param release Callback to release owner; required when owner is supplied. + * @param enclosingOwner Owner of this message, which must not retain itself. * @note Configure redirection before subscribing to this output. The target * must outlive the redirected message and its subscribers unless its * owner is retained. An existing target owner propagates to subscribers. @@ -542,11 +543,15 @@ class Message : public MessageBase void redirectTo(Message& target, void* owner = nullptr, void (*acquire)(void*) = nullptr, - void (*release)(void*) = nullptr) + void (*release)(void*) = nullptr, + void* enclosingOwner = nullptr) { if (this->pointers.payload != target.pointers.payload) { auto reader = target.addSubscriber(); - if (owner && !reader.getSourceHandle()) { + if (enclosingOwner && reader.getSourceHandle() == enclosingOwner) { + reader.setSource(nullptr, nullptr, nullptr); + } + if (owner && owner != enclosingOwner && !reader.getSourceHandle()) { acquire(owner); reader.setSource(owner, acquire, release); } diff --git a/src/architecture/messaging/messagingSupport.i b/src/architecture/messaging/messagingSupport.i index 892e8a15b7d..3980bc3c6ce 100644 --- a/src/architecture/messaging/messagingSupport.i +++ b/src/architecture/messaging/messagingSupport.i @@ -18,6 +18,7 @@ %module(threads="1", package="Basilisk.architecture.messaging") messagingSupport +%include "architecture/_GeneralModuleFiles/swig_python_iterators.ih" %include "std_vector.i" %include "std_string.i" %include "_GeneralModuleFiles/swig_eigen.i" diff --git a/src/architecture/messaging/msgAutoSource/msgInterfacePy.i.in b/src/architecture/messaging/msgAutoSource/msgInterfacePy.i.in index 20c15ff4085..04c553f8bd3 100644 --- a/src/architecture/messaging/msgAutoSource/msgInterfacePy.i.in +++ b/src/architecture/messaging/msgAutoSource/msgInterfacePy.i.in @@ -81,6 +81,10 @@ INSTANTIATE_TEMPLATES({type}, {type}Payload, {baseDir}) %template({type}OutMsgsVector) std::vector, std::allocator> >; %template({type}OutMsgsPtrVector) std::vector*, std::allocator*> >; %template({type}InMsgsVector) std::vector, std::allocator> >; +%pythoncode %{{ +from Basilisk.architecture.messaging import _msgKeepAlive +_msgKeepAlive.preserveCppPointerVectorOwner({type}OutMsgsPtrVector) +%}} {extraPythonContent} %pythoncode %{{ if "{type}" == "EclipseMsg": diff --git a/src/architecture/messaging/newMessaging.ih b/src/architecture/messaging/newMessaging.ih index cc81c9e97ea..2b7d4c50749 100644 --- a/src/architecture/messaging/newMessaging.ih +++ b/src/architecture/messaging/newMessaging.ih @@ -15,6 +15,8 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +%include "architecture/_GeneralModuleFiles/swig_python_iterators.ih" + %pythoncode %{ import numpy as np import pprint @@ -198,6 +200,19 @@ pprint.PrettyPrinter._dispatch[messageTypePayload.__repr__] = _pprint_ ## messag """Return the owner of the currently addressed message storage.""" return self._get_storage_owner(self) + def _redirection_owner(self): + """Resolve an embedded target's module before borrowing its storage.""" + from Basilisk.architecture.messaging import _msgKeepAlive + parent_reference = _msgKeepAlive._instance_attributes(self).get("_bskMessageOwner") + if parent_reference is None: + if not self.thisown: + raise ReferenceError("Cannot redirect to a borrowed message: its storage owner is unknown") + return self._storage_owner() + parent = parent_reference() + if parent is None: + raise ReferenceError("The module owning this message has been deleted") + return self._get_storage_owner(parent) + def addSubscriber(self): """Create a reader that keeps this message alive.""" return self._add_subscriber_with_keepalive(self._storage_owner()) diff --git a/src/architecture/utilities/bskException.swg b/src/architecture/utilities/bskException.swg index eccad4269bf..c40f1955abc 100644 --- a/src/architecture/utilities/bskException.swg +++ b/src/architecture/utilities/bskException.swg @@ -1,3 +1,6 @@ +// Shared binding policy must precede container declarations in every module. +%include "architecture/_GeneralModuleFiles/swig_python_iterators.ih" + %include "exception.i" %{ diff --git a/src/simulation/communication/linkBudget/linkBudget.i b/src/simulation/communication/linkBudget/linkBudget.i index 44197d4a198..f0b570a45b1 100644 --- a/src/simulation/communication/linkBudget/linkBudget.i +++ b/src/simulation/communication/linkBudget/linkBudget.i @@ -37,7 +37,8 @@ // std::vector members; without the std::vector typemap SWIG emits an // un-destructed proxy on access (memory leak, issue #422). %include "std_vector.i" -%template() std::vector; +// Match messagingSupport's type registration, including the default allocator. +%template() std::vector>; %include "linkBudget.h" %include "architecture/msgPayloadDefC/AntennaLogMsgPayload.h" diff --git a/src/simulation/environment/ZeroWindModel/zeroWindModel.i b/src/simulation/environment/ZeroWindModel/zeroWindModel.i index a46022c382f..37c53ce5f33 100644 --- a/src/simulation/environment/ZeroWindModel/zeroWindModel.i +++ b/src/simulation/environment/ZeroWindModel/zeroWindModel.i @@ -51,7 +51,8 @@ struct SCStatesMsg_C; struct WindMsg_C; namespace std { - %template(VectorDouble) vector; + // Match messagingSupport's type registration, including the default allocator. + %template(VectorDouble) vector>; }; %pythoncode %{ diff --git a/src/simulation/onboardDataHandling/instrument/simpleInstrument/simpleInstrument.i b/src/simulation/onboardDataHandling/instrument/simpleInstrument/simpleInstrument.i index dff818e9cf3..9473e9e2dd8 100644 --- a/src/simulation/onboardDataHandling/instrument/simpleInstrument/simpleInstrument.i +++ b/src/simulation/onboardDataHandling/instrument/simpleInstrument/simpleInstrument.i @@ -43,7 +43,8 @@ struct DeviceCmdMsg_C; // without the std::vector typemap SWIG emits an un-destructed proxy on // access (memory leak, issue #422). %include "std_vector.i" -%template() std::vector; +// Match messagingSupport's type registration, including the default allocator. +%template() std::vector>; %include "architecture/msgPayloadDefCpp/DataStorageStatusMsgPayload.h" %pythoncode %{ diff --git a/src/simulation/vizard/dataFileToViz/dataFileToViz.i b/src/simulation/vizard/dataFileToViz/dataFileToViz.i index 8cd15036699..161efbed1bc 100755 --- a/src/simulation/vizard/dataFileToViz/dataFileToViz.i +++ b/src/simulation/vizard/dataFileToViz/dataFileToViz.i @@ -68,5 +68,9 @@ namespace std { %pythoncode %{ import sys +from Basilisk.architecture.messaging import _msgKeepAlive +# These classes replace the messaging wrappers in SWIG's shared type table. +_msgKeepAlive.preserveCppPointerVectorOwner(THROutputMsgOutMsgsPtrVector) +_msgKeepAlive.preserveCppPointerVectorOwner(RWConfigLogMsgOutMsgsPtrVector) protectAllClasses(sys.modules[__name__]) %}