Repository navigation
fix(isotp): report the owned actor's shutdown timeout before unsubscribing - #450
Conversation
…ibing IsoTpChannel.Dispose detached its handler from the actor before disposing the actor it owns. That Dispose is where the actor reports a callback that outlived its shutdown bound -- a TimeoutException through its BackgroundExceptionOccurred -- so the channel's subscribers never heard of it. The handler now comes off after the owned actor's Dispose; for an injected actor nothing runs between the two, so the order is unchanged. Same ordering as #437 for the J1939-TP channel and #414 for the node. An internal ownsActor constructor seam hands an injected actor to the channel to dispose, so the test can give the channel's own actor a short shutdown bound. The test holds the actor's loop until its finally, so the bound is always exceeded, and awaits the report on a signal. Closes #449 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AJKME1N1ZvnY3yYcPwNoF
PR SummaryLow Risk Overview An internal Reviewed by Cursor Bugbot for commit a7daeeb. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
What does this change?
IsoTpChannel.Disposedetached its handler from the actor before disposing the actor it owns. ThatDisposeis where the actor reports a callback that outlived its shutdown bound — aTimeoutExceptionthrough itsBackgroundExceptionOccurred— so the channel's subscribers never heard of it. The handler now comes off after the owned actor'sDispose; for an injected actor nothing runs between the two, so it is detached as before. Same ordering as #437 for the J1939-TP channel (#421) and #414 for the node; #448 already did it for the constructor's catch.An internal
ownsActorconstructor seam (as onJ1939TpChannelsince #437) hands an injected actor to the channel to dispose, so the test can give the channel's own actor a 50 ms shutdown bound. The test holds the actor's loop with a callback released only infinally, so the bound is always exceeded — nothing is asserted against the clock — and awaits the report on a signal, since #418 it is dispatched on the thread pool.IsoTpChannelIntegrationTests.The_Shutdown_Timeout_Of_The_Channels_Own_Actor_Reaches_Its_Subscribers_actor.Dispose()(the old order) → the test fails (signal never set,TimeoutExceptionfrom the wait); restored → green.Closes #449
Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no release!in the title, plus aBREAKING CHANGE:footer explaining the migration)Checklist
-p:CI=true, test (net10.0),dotnet format --verify-no-changes, and pack +eng/verify-packages.pyall pass locally (CONTRIBUTING.md § "Before you push") — fullnet10.0suite greenFR-RAW-031,ADR-7), if anyGenerated by Claude Code