fix: clean up CA subscription management and ensure proper lifecycle management - #74
Merged
Merged
Conversation
drewr95
requested review from
RaulSMS and
khauersp
and
a lite review from Copilot
August 20, 2026 22:46
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #72 by tying ControllerApplication-registered subscriptions to the CA lifecycle, so removing/replacing a CA no longer leaves behind stale subscriber entries bound to defunct CA instances. It also introduces additional locking around CA lifecycle operations and adds tests that validate subscription cleanup and timer shutdown behavior.
Changes:
- Track ECU subscriptions with an optional
ownerand automatically remove CA-owned subscriptions when a CA is removed. - Update DLL
remove_cato return the removed CA instance so the ECU can stop/detach it and clean up owned subscriptions. - Add tests covering CA removal cleanup, replacement behavior, legacy unsubscribe semantics, and timer shutdown across J1939-21/22 receive paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
j1939/electronic_control_unit.py |
Adds owner-aware subscription management, CA lifecycle lock, and CA removal cleanup. |
j1939/controller_application.py |
Routes CA subscriptions through ECU with ownership and adds lifecycle locking around start/stop/claim scheduling. |
j1939/j1939_21.py |
Changes DLL remove_ca to return the removed CA instance (or None). |
j1939/j1939_22.py |
Changes DLL remove_ca to return the removed CA instance (or None). |
test/test_ecu.py |
Adds tests validating CA-owned subscription cleanup, resubscribe requirements, legacy unsubscribe behavior, timer shutdown, and both receive paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
khauersp
reviewed
Aug 21, 2026
khauersp
approved these changes
Aug 21, 2026
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
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.
Summary
Fixes: #72
This pull request introduces improved lifecycle management for message subscriptions associated with
ControllerApplicationinstances in the J1939 stack. Subscriptions registered through aControllerApplicationare now automatically cleaned up when the CA is removed from itsElectronicControlUnit(ECU), while subscriptions registered directly on the ECU remain unaffected. The changes also ensure thread safety during CA lifecycle operations and add comprehensive tests to verify the new behavior.Lifecycle management and subscription cleanup:
ControllerApplicationare now tracked with anownerreference and are automatically removed when the CA is detached from the ECU. Direct ECU subscriptions remain until explicitly removed. (j1939/controller_application.py,j1939/electronic_control_unit.py) [1] [2] [3]remove_camethod inElectronicControlUnitnow stops the CA, removes all its owned subscriptions, and detaches it from the ECU. The underlying data-link layer'sremove_careturns the removed CA instance for this purpose. (j1939/electronic_control_unit.py,j1939/j1939_21.py,j1939/j1939_22.py) [1] [2] [3]Thread safety improvements:
RLock) to bothControllerApplicationandElectronicControlUnitto protect CA lifecycle operations and prevent race conditions when starting, stopping, or removing CAs. (j1939/controller_application.py,j1939/electronic_control_unit.py) [1] [2]j1939/controller_application.py) [1] [2]Testing and validation:
test/test_ecu.py)These changes increase the robustness and predictability of subscription management, especially in dynamic scenarios where CAs are frequently added, removed, or replaced.