Repository navigation
test(rawcan): a blocking Transmit does not hold up a plain frame - #124
Merged
Merged
Conversation
#102's own first step, and the one it says is worth having whether or not the lock is ever narrowed: a bus whose `Transmit` blocks for a controllable duration, and an assertion about what keeps working while it does. `ControllableBus.OnTransmitting` already provides the block -- it is invoked from inside `Transmit`, hence inside `_pendingGate` -- so no new double was needed. What it measures is **narrower than the ticket states**. #102 says "every subscription stalls with it, including subscriptions that have nothing to do with the frame being sent". `OnFrameObserved` calls `TryMatchEcho`, which takes the same lock, only for a frame flagged as an echo and only while a send is pending; subscription dispatch itself reads a volatile snapshot and takes no lock at all. So: * a plain frame is delivered while the driver blocks -- asserted; * an echo frame parks the RX thread inside TryMatchEcho until the driver returns -- asserted. The ticket's claim follows from the second, not directly: `TryMatchEcho` runs *before* dispatch on the same thread, and on a real adapter that is one RX thread, so everything queued behind the echo waits too. Worth stating that way round, because it names what actually has to change. A third thing fell out of writing it, and is in the test as a comment: `SendConfirmed` runs synchronously up to its first await and the register-plus-transmit step is inside that stretch, so the *caller's* own thread is what sits in the blocking driver call. The test starts the send on its own thread for that reason. The test is a pin, not a decoration: moving `_bus.Transmit` out of the lock as a probe flips the echo assertion and the test fails. Deferred echo mode is required throughout -- with the synchronous default, Transmit's own echo re-enters the lock on the transmitting thread while OnTransmitting still holds it. No product code is touched. Whether the lock should be narrowed at all is a separate question, and one this measurement changes the terms of; that goes back to #102. Gate: build 0 warnings / 0 errors, 659/659, `dotnet format --verify-no-changes` exit 0, 9 packages. Refs #102, #53, #100, FR-RAW-031. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
PR SummaryLow Risk Overview The scenario uses Reviewed by Cursor Bugbot for commit 1a93165. 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66904cba2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…8 leg
Three defects in the first revision, none of them in what it measures.
**The net48 leg did not compile.** `Task.IsCompletedSuccessfully` does not
exist on .NET Framework, and Windows CI is where that leg is built. The
assertion was redundant anyway -- the `await` on the line above already
proves the task completed successfully -- so it is gone and the reason is a
comment.
That one is a gate miss, not a tooling gap: `tests/Directory.Build.props`
documents the opt-in, and running it reproduces the failure in one line
from here.
dotnet build tests/CanKit.Pro.Tests -f net48 \
-p:CanKitProTestNetFrameworkLeg=true -c Release
**A failed assertion could hang the run** (Codex P1, Bugbot). Any failure
before `releaseDriver.Set()` left the send inside `Transmit` holding
`_pendingGate`, and `CanBusService.Dispose` wants that same lock -- so the
process would wedge instead of reporting the original failure. The release
is now in a `finally`, and the driver's own wait is bounded as a second
belt. The irony is that the first version of this test already deadlocked
once, for a different reason, and I did not draw the general lesson from it.
**The pin could pass without seeing the stall** (Codex, Bugbot, same
mechanism). `echoArrival.IsCompleted == false` is equally true of a
`Task.Run` the thread pool has not started, so the assertion would have held
against an implementation that no longer takes the lock across `Transmit`
-- i.e. against the fix it is supposed to detect.
A `FrameObserved` handler registered *before* the service now marks the
moment the echo's RX callback begins; the assertion waits for that first.
What is left between the signal and the lock is straight-line code with no
await, which the comment says rather than glosses over.
Re-measured after the rewrite:
| | result |
|---|---|
| `Transmit` moved out of the lock (the probe) | fails **5 of 5** |
| unloaded | passes |
| 8 CPU burners on 4 cores | passes **6 of 6** |
| net48 compile check | succeeds |
Gate: build 0 warnings / 0 errors, 659/659, `dotnet format
--verify-no-changes` exit 0, 9 packages.
Refs #102, FR-RAW-031.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2f3a9aff2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex pushed on the entry signal again, correctly: it is raised by a multicast handler registered *before* CanBusService, so it can complete before CanBusService.OnFrameObserved has begun, and the echo thread can be descheduled between the two. The signal was outside the contention point, not at it. Following the idiom he pointed at -- "as the preceding test does" -- led somewhere better than a stronger signal: **the preceding test already pins this.** `Frame_Arrival_Is_Stamped_Before_The_Pending_Send_Lock`, one test up in the same file, parks an echo on a dedicated thread, waits for `ThreadState.WaitSleepJoin`, and asserts `TryRead(out _)` is false while the driver holds the lock. That is the same fact I was pinning, with a stronger mechanism, written for #112. Measured rather than assumed, with `_bus.Transmit` moved out of the lock as a probe: | test | under the probe | |---|---| | `Frame_Arrival_Is_Stamped_Before_The_Pending_Send_Lock` (existing) | fails **3 of 3** | | mine, as it stood | passed -- it was never the pin | So the echo half of my test was a duplicate of existing coverage, and the racy assertion Codex and Bugbot found was in the duplicated part. Deleting it answers the finding without building a more elaborate signal for something already asserted elsewhere. What remains is the complement, and it is asserted nowhere else: a *plain* frame is dispatched while the driver blocks. That is what makes #102's "every subscription stalls with it" precise -- OnFrameObserved takes the lock only via TryMatchEcho, i.e. only for an echo while a send is pending, so the blanket stall is one RX thread queueing behind the blocked echo rather than subscriptions being serialised against sends. The test says plainly that it is documentation and not a guard: it passes whether or not Transmit holds the lock, and names the neighbour as the pin. It keeps the `finally` from the previous round, because that lesson was general. Rewritten on the neighbour's setup as well (`EchoAcceptedFrames = false` rather than deferred echo), so the two read as a pair. net48 compile-checked with `-p:CanKitProTestNetFrameworkLeg=true`. Gate: build 0 warnings / 0 errors, 659/659, `dotnet format --verify-no-changes` exit 0, 9 packages. Refs #102, #112, FR-RAW-031. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6d0feac39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d6d0fea. Configure here.
Codex and Bugbot, same point on `d6d0fea`, and correct: `RaiseObserved` for the plain frame ran on the *test* thread. If dispatch ever did take `_pendingGate`, that call would simply block until the driver's own bounded wait expired, dispatch would then complete, `TryRead` would succeed, and the test would pass -- after five seconds -- for precisely the stall it claims to rule out. It asserted "delivered eventually", not "delivered while blocked". That is the third round of one mistake from me on this pull request: an assertion that cannot fail for the reason it names. The first two were in the half that turned out to be a duplicate; this one was in the half that is actually new, so it mattered more. Raised on another thread now, and awaited *before* the driver is released -- completing that await while the transmitting thread is still parked in the driver is the assertion. The driver's own wait is bounded at 30 s rather than ShortTimeout, so if dispatch ever blocks, the 5 s await is what expires first and the test fails on it instead of outliving the block. This time the detection is measured rather than asserted. Mutating `OnFrameObserved` so that *every* frame goes through TryMatchEcho -- i.e. making plain dispatch take the lock, the regression this test exists for: | | result | |---|---| | mutation: every frame takes the pending-send lock | fails **3 of 3** | | unmodified, 8 CPU burners on 4 cores | passes **5 of 5** | | unmodified, unloaded | passes | So the test does guard something after all: the lock-free dispatch path. What it still does not guard is #102's echo stall -- `Frame_Arrival_Is_ Stamped_Before_The_Pending_Send_Lock` does that, and the comment says so. net48 compile-checked with `-p:CanKitProTestNetFrameworkLeg=true`. Gate: build 0 warnings / 0 errors, 659/659, `dotnet format --verify-no-changes` exit 0, 9 packages. Refs #102, #112, FR-RAW-031. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
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
2 participants
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.
What does this change?
#102's first step, which the ticket says is worth having whether or not the lock is ever narrowed. No product code is touched.
What it ended up being is smaller than what it started as, and the route there is the useful part.
The measurement contradicts the ticket's wording
#102 says "every subscription stalls with it — including subscriptions that have nothing to do with the frame being sent". Read against the code and then asserted:
OnFrameObservedtakes_pendingGateonly viaTryMatchEcho, i.e. only for a frame flagged as an echo while a send is pending. Dispatch itself reads a volatile snapshot and takes no lock.So a plain frame is delivered while the driver blocks. The blanket stall is a consequence of one RX thread queueing behind the blocked echo, not of subscriptions being serialised against sends — which is the statement a fix would have to address.
What it guards, measured
Mutating
OnFrameObservedso that every frame goes throughTryMatchEcho— i.e. making plain dispatch take the lock, which is the regression this test exists for:What it does not guard is #102's echo stall. That is pinned one test up, by
Frame_Arrival_Is_Stamped_Before_The_Pending_Send_Lock— which fails 3 of 3 when_bus.Transmitis moved out of the lock, where my own first version of the same assertion passed. The comment in the test says which is which.Half of it was already covered, and I did not check first
The echo half — an echo frame cannot get through while the driver holds the lock — was already asserted twenty lines up, written for #112, with a stronger mechanism (a dedicated thread spun until
ThreadState.WaitSleepJoin). I wrote my own weaker version instead of looking, and that duplicated half is where both bots found a real defect. It is deleted rather than strengthened.Four defects across four revisions, none found by running it
Every revision passed locally on the first try.
Task.IsCompletedSuccessfullyis not on .NET FrameworkTransmitholding_pendingGate,Disposewants the same lock, andManualResetEventSlim.Disposedoes not wake waitersIsCompleted == falseis equally true of aTask.Runthat has not startedThe last three are one class — an assertion that cannot fail for the reason it names — which is why the final round came with a mutation rather than an argument.
The net48 miss was a gate gap on my side, not a tooling gap:
tests/Directory.Build.propsdocuments the opt-in, and it reproduces here in one line.Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no releaseFour
test(rawcan)commits. No release — no product code is touched.Whether the lock should be narrowed at all is a separate question, and this changes its terms; the analysis — including that the ticket's own sketch violates its constraint 1 — is on #102 for the maintainer to decide.
Checklist
dotnet build CanKit.Pro.sln -c Release -p:CI=truesucceeds — 0 warnings, 0 errorsdotnet test CanKit.Pro.sln -c Release --no-build --framework net10.0passes — 659/659-p:CanKitProTestNetFrameworkLeg=truedotnet format CanKit.Pro.sln --verify-no-changesclean (exit 0)dotnet packinto a fresh directory +python3 eng/verify-packages.py— 9 packages checked🤖 Generated with Claude Code
https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Generated by Claude Code