Skip to content

test(isotp): dispose the channel and the receive CTS deterministically - #90

Merged
dborgards merged 2 commits into
mainfrom
test/dispose-hygiene
Sep 11, 2026
Merged

dborgards merged 2 commits into
mainfrom
test/dispose-hygiene

Conversation

@dborgards

Copy link
Copy Markdown
Owner

Clears the three open alerts on code scanning, all in IsoTpChannelIntegrationTests.cs and all the same class of test hygiene.

Alert Rule Line
#274 cs/dispose-not-called-on-throw 417
#275 cs/dispose-not-called-on-throw 418
#276 cs/local-not-disposed 798

#274 / #275 — the channel

Dispose missed if exception is thrown by call to method ReceiveAsync. The channel was created with a plain var and closed only by the two explicit Dispose() calls the test makes deliberately, one line below the ReceiveAsync() that could throw first.

It now carries a using as well. Since the behaviour under test is that Dispose is idempotent, a third call at scope exit is by definition a no-op — the test still proves exactly what it did before, and no longer leaks the channel for the rest of the run when an assertion fails.

#276 — the receive CTS

Disposable 'CancellationTokenSource' is created but not disposed. It was constructed inline purely to reach .Token, leaving the timer it holds to the finalizer. Now owned by the test.

Scope: only the three reported sites

The same inline-CTS spelling appears 10 more times in this file and roughly 65 times across the suite, and CodeQL flagged exactly one of them. I did not sweep the rest:

  • it would be a large mechanical diff in files two other pull requests are currently touching;
  • each occurrence needs a uniquely named local in its scope, so it is not a safe blanket substitution;
  • and in a short-lived xUnit test an undisposed CTS is a released-late timer, not a leak that outlives the process.

If you want the sweep, it should be its own pull request, and it is worth deciding at the same time whether a small test helper is better than 76 using var lines.

Verified

  • dotnet test CanKit.Pro.sln -c Release → 412 passed, 0 failed
  • dotnet build tests/CanKit.Pro.Tests -f net48 → 0 warnings, 0 errors
  • No production code touched; the API approvals do not move.

🤖 Generated with Claude Code

Clears the three open code-scanning alerts on `main`, all in
IsoTpChannelIntegrationTests and all the same class of test hygiene:

  #274, #275  cs/dispose-not-called-on-throw  (lines 417, 418)
  #276        cs/local-not-disposed           (line 798)

The channel in the idempotent-dispose test was created with a plain `var` and
closed only by the two explicit Dispose calls the test makes on purpose, so a
ReceiveAsync that threw would leave it open for the rest of the run. It now
also has a `using`; since the behaviour under test is that Dispose is
idempotent, the third call at scope exit is by definition a no-op.

The CancellationTokenSource at line 798 was constructed inline purely to
reach `.Token`, leaving its timer to the finalizer. It is now owned by the
test.

Only the three reported sites are touched. The same inline-CTS spelling
appears 10 more times in this file and about 65 times across the suite;
sweeping those is a separate decision, not something to smuggle into a fix
for three specific alerts.

Verified: 412/412 on net10.0, and the net48 leg compiles with 0 warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Test-only disposal fixes for static analysis; no production or API changes.

Overview
Clears three CodeQL code-scanning alerts in IsoTp integration tests only; production ISO-TP behavior is unchanged.

In Dispose_Unblocks_Pending_ReceiveAsync, the channel is now using var so it is disposed on scope exit if ReceiveAsync or an assertion fails before the deliberate double Dispose() calls. The third dispose at exit is documented as a no-op given idempotency—the test still proves the same contract.

In Send_Faults_With_The_Bus_Layer_Exception_And_Channel_Remains_Usable, the receive timeout CancellationTokenSource is held in using var recvCts instead of an inline new, so its timer is released deterministically at test end (cs/local-not-disposed).

Reviewed by Cursor Bugbot for commit 6e0a8bd. Bugbot is set up for automated code reviews on this repo. Configure here.

@dborgards
dborgards merged commit bd729e3 into main Sep 11, 2026
10 checks passed
@dborgards
dborgards deleted the test/dispose-hygiene branch September 11, 2026 16:18
dborgards pushed a commit that referenced this pull request Sep 12, 2026
Codex found that `CONTRIBUTING.md`'s central release claim is false, and I had
copied it into CLAUDE.md unverified. Checked against the history rather than
argued about:

    $ git log --first-parent --oneline origin/main | head -3
    52834b2 Merge pull request #91 from dborgards/claude/sort-open-issues-v9a7kk
    bd729e3 Merge pull request #90 from dborgards/test/dispose-hygiene
    c2712ba Merge pull request #89 from dborgards/chore/format-and-gate

Every one has two parents. Nothing is squash-merged; every branch commit is
retained, and semantic-release analyses all of them. So the pull-request title
is not "the commit semantic-release reads" -- the branch commits are.

The practical inversions:

* a `docs:`-titled pull request containing one `feat:` commit publishes a minor
  release, which the old guidance would have called impossible;
* a `BREAKING CHANGE:` footer belongs in the commit message, where the analyser
  reads it. Putting it only in the pull-request body -- which is what I did on
  #93, and told the author was necessary -- does nothing. No harm there, since
  the commit carried it too, but the reasoning was wrong.

Both documents now describe the observed behaviour. CONTRIBUTING.md is
corrected at the source rather than left contradicting the file derived from
it; that is one file beyond this pull request's nominal scope, and leaving the
original assertion standing would have been worse than the scope creep.

Neither document decides the strategy. If the intent is squash merging, that
belongs in the repository settings, and then both texts want revisiting --
noted in CLAUDE.md for the owner rather than assumed either way.

Also from the same review: the packaging gate used a fixed `/tmp/nupkgs`.
Re-running it leaves earlier artifacts in place, and since `verify-packages.py`
checks whatever `*.nupkg` it finds, a change that stops producing a package
could pass on the stale copy. Now packs into a fresh `mktemp -d`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant