feat(isotp)!: let IIsoTpChannel.SettleAsync be cancelled with a CancellationToken - #244
Conversation
…llationToken SettleAsync waits for the channel's actor to take everything queued so far, and had no way to give up on that wait: a caller whose own deadline had passed, or whose operation was cancelled, stayed behind whatever the actor was busy with. It now takes an optional CancellationToken and hands it to the actor's PostAsync, which withdraws the wait while it is still queued. The token ends the wait, not the settling: what the demux had buffered has been handed to the actor by then and is handled as usual. The ADR window before 1.3.0 is the last point where an interface member can be changed without a major version, so the signature is replaced rather than overloaded next to the old one. The UDS client's own calls keep the default token: they settle at a deadline decision, and letting a caller's cancellation abort that would change what it decides. BREAKING CHANGE: IIsoTpChannel.SettleAsync gained a CancellationToken parameter. Callers compile unchanged; implementers of IIsoTpChannel and code compiled against 1.2.x must be rebuilt. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UWRpkQzKkNDYz3WiNgWvWU
PR SummaryMedium Risk Overview
XML docs, IsoTp README, and arc42 were updated for that contract. Three integration tests cover already-cancelled tokens, cancel while the actor is busy, and parity with a live token. API approval baseline and the UDS test stub were updated; production UDS Reviewed by Cursor Bugbot for commit 0c74981. 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! |
2 participants
What does this change?
IIsoTpChannel.SettleAsync()waits for the channel's actor to take everything queued so far, and had no way to give up on that wait: a caller whose own deadline had passed, or whose operation was cancelled, stayed behind whatever the actor was busy with. It now takes an optionalCancellationTokenand passes it to the actor'sPostAsync, which since #242 withdraws the wait while it is still queued.The token ends the wait, not the settling. What the demux had buffered has been handed to the actor by then and is handled as usual, so cancelling never leaves a half-drained subscription behind.
The UDS client's own
SettleAsynccalls keep the default token on purpose. They settle at a deadline decision ("is a response there, was there a 0x78"), and letting a caller's cancellation abort the settle would change what that decision sees. That is a behaviour question of its own and not part of this change.This is the second of the two interface members found while going through checklist item 4 of ADR 0001 (API baselines reviewed as a whole before the freeze); the first was
PostAsyncin #242. The window before thev1.3.0tag is the last point where an interface member can change without a major version, so the signature is replaced, not overloaded and not shadowed with[Obsolete].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)The commit is a
featwith!and aBREAKING CHANGE:footer in its message. While the ADR window is open,breakingmaps tominorin.releaserc.json.Checklist
dotnet build CanKit.Pro.sln -c Releasesucceeds (run with-p:CI=true, 0 warnings)dotnet test CanKit.Pro.sln -c Releasepasses (run with--framework net10.0: 1385 passed; thenet48leg was not run locally)IsoTpChannelIntegrationTests)Also run locally:
dotnet format --verify-no-changes(clean) anddotnet packwitheng/verify-packages.py(9 packages). The IsoTp approval baseline was taken from the generated.received.txt; its diff is exactly the one changed signature line.The
IIsoTpChanneltest double inUdsExpiredDeadlineTests, the arc42 class diagram and the IsoTp README follow the new signature.Mutation check
Not passing the token on to
PostAsync(_actor.PostAsync(() => { })) makes both cancellation tests fail: the already-cancelled one fails at once, the one that cancels while the actor is busy fails after its 5-second bound because the settle stays pending until the actor is released. With the token passed on, all fourSettle_*tests pass. The third test, a live token behaving like none, is a parity check and is not expected to catch that mutation.Not covered by a test: the two early returns that ignore the token (channel already disposed, and a call made from the channel's own actor). Both complete at once by design.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UWRpkQzKkNDYz3WiNgWvWU
Generated by Claude Code