From cf24dc083a849d7e5aa448b4daf697719717272d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:02:13 +0000 Subject: [PATCH 01/14] fix(rawcan): stop an expired pending send from poisoning the echo FIFO An expired or cancelled SendConfirmed call stayed in its key's pending FIFO until SendWithEchoConfirmAsync's `finally` ran. Because the pending TCS is created with RunContinuationsAsynchronously, that `finally` runs a scheduling turn later on a pool thread, and until it did the resolved entry was still the oldest entry for its key -- so the next byte-identical send lost its echo to it and timed out as well. One timeout cascaded into the next. Three changes, each closing part of it (FR-RAW-031, FR-RAW-033): * The cancellation/timeout registration unlinks the entry under _pendingGate before it completes the TCS, so an entry stops being matchable the instant it is resolved. The `finally` stays: it is still the cleanup for the paths that never reach the registration (rejection, an exception out of Transmit). * TryMatchEcho skips -- and unlinks -- entries whose TCS is already completed instead of taking list.First unconditionally. With the above they should not be reachable; this keeps a single stale entry from silently eating one echo and blocking the FIFO for every later one. * PendingKey includes the frame kind and the flags that identify a frame on the wire (Ext, Rtr, Error), so a standard 0x100 and an extended 0x100 with the same payload no longer share one FIFO. Brs and Esi are deliberately excluded: an adapter may report them differently on the echo than the caller asked for, and two sends differing only in those bits are interchangeable for confirmation anyway -- including them could only cause spurious timeouts. Tests: ControllableBus gains an OnTransmitting hook, which runs on the transmitting thread while CanBusService holds its pending-send lock. That makes the FIFO-poisoning case deterministic rather than a race with a pool thread: the first send is resolved from inside the second send's Transmit, so its own async cleanup cannot run until the second send's echo has already been matched. All three new tests were confirmed to fail against the unfixed service (the poisoning test five times out of five) and to pass with it. Closes #24 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- src/CanKit.Pro.RawCan/CanBusService.cs | 66 ++++++++++--- src/CanKit.Pro.RawCan/PendingSend.cs | 39 ++++++-- .../Infrastructure/ControllableBus.cs | 15 +++ .../TestCases/TxConfirmTests.cs | 97 +++++++++++++++++++ 4 files changed, 199 insertions(+), 18 deletions(-) diff --git a/src/CanKit.Pro.RawCan/CanBusService.cs b/src/CanKit.Pro.RawCan/CanBusService.cs index 4af3750..ce9fd5c 100644 --- a/src/CanKit.Pro.RawCan/CanBusService.cs +++ b/src/CanKit.Pro.RawCan/CanBusService.cs @@ -42,8 +42,10 @@ public sealed class CanBusService : ICanBusService // holding _hubsGate while delivering. private volatile Subscription[] _snapshot = Array.Empty(); - // Pending SendConfirmed calls awaiting an echo match, keyed by (ID, payload) so multiple - // concurrent byte-identical sends are matched FIFO instead of crashing/cross-matching + // Pending SendConfirmed calls awaiting an echo match, keyed by everything that identifies + // the frame on the wire (see PendingKey) so multiple concurrent identical sends are matched + // FIFO instead of crashing/cross-matching, and two sends that merely *look* alike -- a + // standard and an extended 0x100 with the same payload -- do not share one FIFO at all // (FR-RAW-031). Guarded by its own lock, separate from _gate, so TX-confirm churn never // contends with subscription registry churn (and vice versa). private readonly object _pendingGate = new(); @@ -298,7 +300,7 @@ private async Task SendApproximatedAsync(CanFrame frame, Cancell private async Task SendWithEchoConfirmAsync(CanFrame frame, TimeSpan timeout, CancellationToken cancellationToken) { - var pending = new PendingSend(new PendingKey(frame.ID, frame.Data)); + var pending = new PendingSend(new PendingKey(frame.ID, frame.Data, frame.Flags, frame.FrameKind)); int accepted; try @@ -353,13 +355,29 @@ private async Task SendWithEchoConfirmAsync(CanFrame frame, Time } } - private static async Task WaitForPendingAsync(PendingSend pending, TimeSpan timeout, CancellationToken cancellationToken) + private async Task WaitForPendingAsync(PendingSend pending, TimeSpan timeout, CancellationToken cancellationToken) { using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); // Registration fires on whichever comes first: caller cancellation or our own timeout. using var registration = timeoutCts.Token.Register(static state => { - var (p, ct) = ((PendingSend, CancellationToken))state!; + var (service, p, ct) = ((CanBusService, PendingSend, CancellationToken))state!; + + // Unlink *before* completing, and here rather than in SendWithEchoConfirmAsync's + // `finally`. The Tcs completes its awaiter asynchronously + // (RunContinuationsAsynchronously), so that `finally` runs a scheduling turn later + // on a pool thread; until it did, this expired entry stayed the FIFO head for its + // key and swallowed the echo of the next byte-identical send, turning one timeout + // into a cascade of them. Unlinking under _pendingGate before the Tcs is completed + // closes that window entirely: from the instant this entry is resolved it is no + // longer matchable. The `finally` stays as the cleanup for every path that does not + // pass through here (rejection, an exception out of Transmit). + // + // Monitor is reentrant, so this is safe even when the cancellation is triggered + // from a thread that already holds _pendingGate (a caller cancelling from inside a + // subscription predicate while a synchronous echo is being dispatched, say). + service.RemovePending(p); + if (ct.IsCancellationRequested) { p.Tcs.TrySetCanceled(ct); @@ -374,7 +392,7 @@ private static async Task WaitForPendingAsync(PendingSend pendin FailureReason = TxConfirmFailureReason.Timeout, }); } - }, (pending, cancellationToken)); + }, (this, pending, cancellationToken)); timeoutCts.CancelAfter(timeout); return await pending.Tcs.Task.ConfigureAwait(false); @@ -413,17 +431,41 @@ private void RemovePending(PendingSend pending) private void TryMatchEcho(in CanFrameView echoView) { - var key = new PendingKey(echoView.ID, echoView.Data); + var key = new PendingKey(echoView.ID, echoView.Data, echoView.Flags, echoView.FrameKind); PendingSend? matched = null; lock (_pendingGate) { - if (_pending.TryGetValue(key, out var list) && list.First is { } node) + if (_pending.TryGetValue(key, out var list)) { - matched = node.Value; - list.RemoveFirst(); // FIFO: oldest pending send for this key matches first - matched.Node = null; - Interlocked.Decrement(ref _pendingCount); + // FIFO: the oldest pending send for this key matches first -- but only if it is + // still waiting. An entry whose Tcs is already completed has been resolved by + // some other path and can no longer consume anything; matching it would silently + // drop this echo (TrySetResult no-ops on a completed Tcs) and leave the send it + // actually belonged to waiting for an echo that has already come and gone. + // + // Every resolution path unlinks under this same lock before it completes the + // Tcs, so a completed entry should not be reachable here at all. This stays as + // the second guard for that invariant -- and, because it unlinks what it skips, + // it also stops such an entry from blocking the FIFO for every later echo + // instead of only for this one. + for (var node = list.First; node is not null;) + { + var next = node.Next; + var candidate = node.Value; + + list.Remove(node); + candidate.Node = null; + Interlocked.Decrement(ref _pendingCount); + + if (!candidate.Tcs.Task.IsCompleted) + { + matched = candidate; + break; + } + + node = next; + } if (list.Count == 0) _pending.Remove(key); diff --git a/src/CanKit.Pro.RawCan/PendingSend.cs b/src/CanKit.Pro.RawCan/PendingSend.cs index 9e46c3c..0bec611 100644 --- a/src/CanKit.Pro.RawCan/PendingSend.cs +++ b/src/CanKit.Pro.RawCan/PendingSend.cs @@ -1,27 +1,49 @@ using System; using System.Threading; using System.Threading.Tasks; +using CanKit.Abstractions.API.Can.Definitions; +using CanKit.Abstractions.API.Common.Definitions; namespace CanKit.Pro.RawCan { /// - /// Identifies a pending echo-matched send by the two fields an echo frame is matched against: - /// CAN ID and payload content. Multiple concurrent sends with an identical key are matched - /// strictly FIFO (oldest pending send first) against arriving echoes with the same key — - /// see 's pending-send tracking (FR-RAW-031). This mirrors, at the + /// Identifies a pending echo-matched send by the fields an echo frame is matched against: CAN + /// ID, payload content, frame kind, and the flags that decide *which frame this is* on the + /// wire. Multiple concurrent sends with an identical key are matched strictly FIFO (oldest + /// pending send first) against arriving echoes with the same key — see + /// 's pending-send tracking (FR-RAW-031). This mirrors, at the /// L2 echo-matching layer, the exact class of bug the review flagged for the ISO-TP prototype's /// deadline queue crashing on identical in-flight frames. /// internal readonly struct PendingKey : IEquatable { + // The flags that make two frames different frames rather than the same frame transmitted + // differently. Ext decides which ID space the ID lives in -- without it a standard 0x100 + // and an extended 0x100 with the same payload match each other, and one send's echo + // confirms the other's caller. Rtr separates a remote request from a data frame with the + // same (empty) payload, and Error an error frame from a data frame. + // + // Brs and Esi are deliberately *not* part of the identity: they are link-layer + // transmission attributes, and an adapter may report an echo whose BRS/ESI differ from + // what the caller asked for (ESI in particular reflects the controller's error state, not + // the caller's request). Two sends that differ in nothing else are interchangeable for + // confirmation purposes anyway -- the FIFO hands each of them exactly one echo -- so + // including these bits could only turn a matched echo into a spurious timeout, which is + // the very failure this key exists to prevent. + private const FrameFlags IdentityFlags = FrameFlags.Ext | FrameFlags.Rtr | FrameFlags.Error; + private readonly int _id; private readonly byte[] _payload; + private readonly FrameFlags _flags; + private readonly CanFrameType _frameKind; private readonly int _hash; - public PendingKey(int id, ReadOnlyMemory payload) + public PendingKey(int id, ReadOnlyMemory payload, FrameFlags flags, CanFrameType frameKind) { _id = id; _payload = payload.ToArray(); + _flags = flags & IdentityFlags; + _frameKind = frameKind; // Manual combine: System.HashCode isn't available on netstandard2.0 without an extra // package reference, and this hash never needs to be cryptographically strong. @@ -29,6 +51,8 @@ public PendingKey(int id, ReadOnlyMemory payload) { var hash = 17; hash = hash * 31 + _id; + hash = hash * 31 + (int)_flags; + hash = hash * 31 + (int)_frameKind; foreach (var b in _payload) hash = hash * 31 + b; _hash = hash; @@ -36,7 +60,10 @@ public PendingKey(int id, ReadOnlyMemory payload) } public bool Equals(PendingKey other) - => _id == other._id && _payload.AsSpan().SequenceEqual(other._payload); + => _id == other._id + && _flags == other._flags + && _frameKind == other._frameKind + && _payload.AsSpan().SequenceEqual(other._payload); public override bool Equals(object? obj) => obj is PendingKey other && Equals(other); diff --git a/tests/CanKit.Pro.Tests/Infrastructure/ControllableBus.cs b/tests/CanKit.Pro.Tests/Infrastructure/ControllableBus.cs index 2fc306d..e30235a 100644 --- a/tests/CanKit.Pro.Tests/Infrastructure/ControllableBus.cs +++ b/tests/CanKit.Pro.Tests/Infrastructure/ControllableBus.cs @@ -115,6 +115,19 @@ public static ControllableBus DeferredEchoCapable(string session) /// public DeferredEchoQueue DeferredEchoes { get; } + /// + /// Invoked for every accepted transmit, on the transmitting thread, after the frame is + /// counted and before its echo is raised or parked. + /// + /// + /// The point of the hook is *where* it runs, not that it runs: CanBusService transmits + /// while holding its pending-send lock, so anything done here happens with that lock held and + /// with the transmitting send already registered. That is what lets a test act on the pending + /// list at an instant no other thread can change it — a background continuation that wants the + /// same lock simply waits until this returns — instead of racing one. + /// + public Action? OnTransmitting { get; set; } + /// Number of frames handed to . public int TransmitCount => Volatile.Read(ref _transmitCount); @@ -155,6 +168,8 @@ public int Transmit(in CanFrame frame) Interlocked.Increment(ref _transmitCount); if (!AcceptTransmit) return 0; + OnTransmitting?.Invoke(frame); + if (EchoAcceptedFrames) { // Synchronous is the default because that is what a real echo-mode adapter does, and diff --git a/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs b/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs index d8e6da0..1263593 100644 --- a/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs @@ -1,6 +1,7 @@ using System; using System.Diagnostics; using System.Linq; +using System.Threading; using System.Threading.Tasks; using CanKit.Abstractions.API.Can; using CanKit.Abstractions.API.Can.Definitions; @@ -147,6 +148,102 @@ public async Task Echo_Bus_Matches_Concurrent_Identical_Frames_Individually_With sender.TransmitCount.Should().Be(n); } + // FR-RAW-031/033: a pending send that has already been resolved -- here by cancellation, in + // the field usually by its own timeout -- must leave the echo FIFO at the moment it is + // resolved. While it stayed there it was the oldest entry for its key, so the *next* + // byte-identical send lost its echo to it and timed out too: one timeout cascading into the + // next. + // + // The setup is what makes this deterministic instead of a race against a pool thread. The + // first send is resolved from inside the second send's Transmit, i.e. on the transmitting + // thread while CanBusService still holds its pending-send lock. The first send's own async + // cleanup wants that same lock, so it cannot run until this Transmit returns -- by which time + // the second send's echo has already been matched, inside the same call. If the resolution + // path does not unlink the entry itself, the expired entry is therefore *guaranteed*, not + // merely likely, to be the FIFO head when that echo arrives. + [Fact] + public async Task Echo_Bus_Does_Not_Let_A_Resolved_Send_Consume_A_Later_Identical_Echo() + { + using var sender = OpenEcho(); + using var service = new CanBusService(sender); + + var frame = CanFrame.Classic(0x600, new byte[] { 7, 7 }); + using var cancelFirst = new CancellationTokenSource(); + + // The first send stays pending: its echo never comes back. + sender.EchoAcceptedFrames = false; + var first = service.SendConfirmed(frame, TimeSpan.FromSeconds(30), cancelFirst.Token); + sender.TransmitCount.Should().Be(1, + "SendConfirmed registers the pending entry and transmits before it awaits anything"); + + sender.EchoAcceptedFrames = true; + sender.OnTransmitting = _ => cancelFirst.Cancel(); + + var second = await service.SendConfirmed(frame, ShortTimeout); + + second.Confirmed.Should().BeTrue( + "the echo belongs to the only send still waiting for one, not to the cancelled entry"); + second.IsApproximated.Should().BeFalse(); + second.FailureReason.Should().Be(TxConfirmFailureReason.None); + + await Assert.ThrowsAnyAsync(() => first); + } + + // FR-RAW-031: the echo is matched on what identifies the frame, not on its ID alone. A + // standard 0x100 and an extended 0x100 carrying the same payload are two different frames on + // the wire; keyed on the ID alone they share one FIFO, so the extended frame's echo confirms + // whichever of the two was sent first and the other waits for an echo that has already been + // consumed. + [Fact] + public async Task Echo_Bus_Does_Not_Confirm_A_Standard_Send_From_An_Extended_Echo_With_The_Same_Id() + { + using var sender = OpenEcho(); + using var service = new CanBusService(sender); + sender.EchoAcceptedFrames = false; // every echo in this test is delivered by hand + + var payload = new byte[] { 0xAB }; + var standard = service.SendConfirmed(CanFrame.Classic(0x100, payload), TimeSpan.FromSeconds(30)); + var extended = service.SendConfirmed( + CanFrame.Classic(0x100, payload, isExtendedFrame: true), TimeSpan.FromSeconds(30)); + sender.TransmitCount.Should().Be(2); + + sender.RaiseObserved(CanFrame.Classic(0x100, payload, isExtendedFrame: true), isEcho: true); + + var completed = await Task.WhenAny(standard, extended).WaitAsync(ShortTimeout); + completed.Should().BeSameAs(extended, "an extended-ID echo confirms the extended-ID send"); + (await completed).Confirmed.Should().BeTrue(); + standard.IsCompleted.Should().BeFalse("no echo for the standard-ID frame has arrived yet"); + + service.Dispose(); // resolves the send left outstanding on purpose + await Assert.ThrowsAnyAsync(() => standard); + } + + // FR-RAW-031, the same point for the frame kind: a Classic and a CAN-FD frame with the same ID + // and the same payload bytes are not interchangeable, and one's echo must not confirm the + // other. + [Fact] + public async Task Echo_Bus_Does_Not_Confirm_A_Classic_Send_From_A_Can_Fd_Echo_With_The_Same_Id() + { + using var sender = OpenEcho(); + using var service = new CanBusService(sender); + sender.EchoAcceptedFrames = false; + + var payload = new byte[] { 0xAB }; + var classic = service.SendConfirmed(CanFrame.Classic(0x100, payload), TimeSpan.FromSeconds(30)); + var fd = service.SendConfirmed(CanFrame.Fd(0x100, payload), TimeSpan.FromSeconds(30)); + sender.TransmitCount.Should().Be(2); + + sender.RaiseObserved(CanFrame.Fd(0x100, payload), isEcho: true); + + var completed = await Task.WhenAny(classic, fd).WaitAsync(ShortTimeout); + completed.Should().BeSameAs(fd, "a CAN-FD echo confirms the CAN-FD send"); + (await completed).Confirmed.Should().BeTrue(); + classic.IsCompleted.Should().BeFalse("no echo for the Classic frame has arrived yet"); + + service.Dispose(); + await Assert.ThrowsAnyAsync(() => classic); + } + // FR-RAW-033: a send whose echo will never arrive fails observably (Confirmed = false, // FailureReason = Timeout) within the configured timeout, not an indefinite hang. [Fact] From 458ff08ec57935815a3bc39796a0457cb9ea987a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:10:03 +0000 Subject: [PATCH 02/14] perf(rawcan): copy each payload once per frame, and stop copying echo payloads at all Two allocations on the per-frame dispatch path that nothing needed (#53, findings 2 and 3). The payload copy that protects a buffered frame from the adapter's RX lease being disposed under it was made once per *matching subscription*. The reason for it is a property of the frame, not of the subscriber, and what a subscriber receives is a ReadOnlyMemory it may only read -- so the copy is now made by the first subscription that buffers the frame and reused by the rest. With n subscriptions matching, that is n-1 array allocations and copies per frame that no longer happen. A frame nobody matches still allocates nothing, and the predicate is still handed the aliasing view rather than the copy. TryMatchEcho built its lookup key by copying the echo frame's payload, i.e. once for every echo frame the adapter reports while any send is outstanding, purely to ask whether anything was waiting for it. PendingKey now distinguishes the two uses: ForPendingSend copies, because the entry outlives the caller's frame; ForEchoLookup borrows, because the key is dropped again before the lock is released. Also corrects the comment above IsoTpChannel's own payload copy, which described a hazard that does not exist -- the subscription hands out memory it owns, not the adapter's lease. The copy itself stays, and now has the reason it actually has: that array is shared with every other subscription that matched the frame, while the ISO-TP state machine wants a byte[] of its own to keep across awaits. Tests: the shared-copy test asserts the two subscriptions' payloads are the same array (and still not the adapter's), and fails against a per-subscription copy; the echo-lookup test measures allocated bytes per unmatched echo frame and reads 88 B/frame without the fix against a 64-byte budget. The existing test that covered CanFrameEvent's byte-wise equality through two subscriptions' distinct buffers keeps that coverage as a direct test over two arrays, since the demux no longer produces the distinct-buffer case by itself. Refs #53 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- src/CanKit.Pro.IsoTp/IsoTpChannel.cs | 9 ++- src/CanKit.Pro.RawCan/CanBusService.cs | 17 ++++- src/CanKit.Pro.RawCan/PendingSend.cs | 28 ++++++-- src/CanKit.Pro.RawCan/Subscription.cs | 20 +++++- .../TestCases/RawCanSubscriptionTests.cs | 66 ++++++++++++++++--- .../TestCases/TxConfirmTests.cs | 41 ++++++++++++ 6 files changed, 159 insertions(+), 22 deletions(-) diff --git a/src/CanKit.Pro.IsoTp/IsoTpChannel.cs b/src/CanKit.Pro.IsoTp/IsoTpChannel.cs index 061dca3..a99f4ca 100644 --- a/src/CanKit.Pro.IsoTp/IsoTpChannel.cs +++ b/src/CanKit.Pro.IsoTp/IsoTpChannel.cs @@ -352,9 +352,12 @@ private async Task RunReaderAsync() .ConfigureAwait(false)) { var frame = frameEvent.Frame; - // Copy defensively: CanFrameView.Data may reference a reused buffer once we - // hand control back to the subscription, and the RX state machine will keep the - // payload alive across await points via the reassembly buffer. + // Not the hazard the previous comment described: the subscription already hands + // out a payload it owns, so nothing the adapter does can corrupt it. What it hands + // out is one array shared by every subscription that matched the frame, and it is + // a ReadOnlyMemory while the state machine below wants a byte[] it keeps + // across await points -- so this stays a copy, now as this channel's private + // buffer rather than as protection against the RX lease. var payload = frame.Data.ToArray(); var addrExt = _endpoint.UsesAddressExtension; // Endpoint uses an address-extension byte and the first byte does not match: diff --git a/src/CanKit.Pro.RawCan/CanBusService.cs b/src/CanKit.Pro.RawCan/CanBusService.cs index ce9fd5c..a4041a8 100644 --- a/src/CanKit.Pro.RawCan/CanBusService.cs +++ b/src/CanKit.Pro.RawCan/CanBusService.cs @@ -174,6 +174,15 @@ private void OnFrameObserved(object? sender, CanReceiveDataView e) var view = e.CanFrame; var isEcho = e.IsEcho; var receiveTimestamp = e.ReceiveTimestamp; + + // One owned payload copy per *frame*, created by the first subscription that actually + // buffers it and reused by every later one, instead of one copy per matching + // subscription. The copy exists because the view aliases the adapter's RX lease (see + // Subscription.TryDeliver); nothing about that reason is per-subscriber, and what the + // subscribers get handed is a ReadOnlyMemory they may only read. A frame nobody + // matches still allocates nothing at all. + byte[]? ownedPayload = null; + foreach (var subscription in subscriptions) { // A subscription's filter predicate is caller-supplied and may throw. Isolate each @@ -185,7 +194,7 @@ private void OnFrameObserved(object? sender, CanReceiveDataView e) // of being silently swallowed. try { - subscription.TryDeliver(view, isEcho, receiveTimestamp); + subscription.TryDeliver(view, isEcho, receiveTimestamp, ref ownedPayload); } catch (Exception ex) { @@ -300,7 +309,7 @@ private async Task SendApproximatedAsync(CanFrame frame, Cancell private async Task SendWithEchoConfirmAsync(CanFrame frame, TimeSpan timeout, CancellationToken cancellationToken) { - var pending = new PendingSend(new PendingKey(frame.ID, frame.Data, frame.Flags, frame.FrameKind)); + var pending = new PendingSend(PendingKey.ForPendingSend(frame.ID, frame.Data, frame.Flags, frame.FrameKind)); int accepted; try @@ -431,7 +440,9 @@ private void RemovePending(PendingSend pending) private void TryMatchEcho(in CanFrameView echoView) { - var key = new PendingKey(echoView.ID, echoView.Data, echoView.Flags, echoView.FrameKind); + // Aliases the echo frame's payload rather than copying it: this runs for every echo + // frame the adapter reports, and the key is dropped again before the lock is released. + var key = PendingKey.ForEchoLookup(echoView.ID, echoView.Data, echoView.Flags, echoView.FrameKind); PendingSend? matched = null; lock (_pendingGate) diff --git a/src/CanKit.Pro.RawCan/PendingSend.cs b/src/CanKit.Pro.RawCan/PendingSend.cs index 0bec611..f45359d 100644 --- a/src/CanKit.Pro.RawCan/PendingSend.cs +++ b/src/CanKit.Pro.RawCan/PendingSend.cs @@ -33,15 +33,33 @@ namespace CanKit.Pro.RawCan private const FrameFlags IdentityFlags = FrameFlags.Ext | FrameFlags.Rtr | FrameFlags.Error; private readonly int _id; - private readonly byte[] _payload; + private readonly ReadOnlyMemory _payload; private readonly FrameFlags _flags; private readonly CanFrameType _frameKind; private readonly int _hash; - public PendingKey(int id, ReadOnlyMemory payload, FrameFlags flags, CanFrameType frameKind) + /// + /// Key for an entry that is about to be stored: it copies the payload, because the caller's + /// frame -- and on the RX side the adapter's lease behind it -- may be reused or disposed + /// long before the entry is matched. + /// + public static PendingKey ForPendingSend(int id, ReadOnlyMemory payload, FrameFlags flags, CanFrameType frameKind) + => new(id, payload.ToArray(), flags, frameKind); + + /// + /// Key for looking one up, which aliases instead of copying it: + /// every echo frame would otherwise cost an array allocation on the dispatch hot path just + /// to ask a question. Safe only because the key never leaves the lookup — it is used to + /// probe the dictionary and then dropped, never stored, so it cannot outlive the frame it + /// borrows from. + /// + public static PendingKey ForEchoLookup(int id, ReadOnlyMemory payload, FrameFlags flags, CanFrameType frameKind) + => new(id, payload, flags, frameKind); + + private PendingKey(int id, ReadOnlyMemory payload, FrameFlags flags, CanFrameType frameKind) { _id = id; - _payload = payload.ToArray(); + _payload = payload; _flags = flags & IdentityFlags; _frameKind = frameKind; @@ -53,7 +71,7 @@ public PendingKey(int id, ReadOnlyMemory payload, FrameFlags flags, CanFra hash = hash * 31 + _id; hash = hash * 31 + (int)_flags; hash = hash * 31 + (int)_frameKind; - foreach (var b in _payload) + foreach (var b in _payload.Span) hash = hash * 31 + b; _hash = hash; } @@ -63,7 +81,7 @@ public bool Equals(PendingKey other) => _id == other._id && _flags == other._flags && _frameKind == other._frameKind - && _payload.AsSpan().SequenceEqual(other._payload); + && _payload.Span.SequenceEqual(other._payload.Span); public override bool Equals(object? obj) => obj is PendingKey other && Equals(other); diff --git a/src/CanKit.Pro.RawCan/Subscription.cs b/src/CanKit.Pro.RawCan/Subscription.cs index fe2710b..12b027c 100644 --- a/src/CanKit.Pro.RawCan/Subscription.cs +++ b/src/CanKit.Pro.RawCan/Subscription.cs @@ -126,6 +126,14 @@ public void Reconfigure(Func? predicate) /// here would require a larger API change. /// /// + /// That copy is made once per frame, not once per subscription: + /// carries it across the service's dispatch loop, and the + /// first subscription that buffers the frame is the one that creates it. Every matching + /// subscription therefore hands its reader the same array, exposed — as always — as a + /// that a subscriber may read but not write. A frame no + /// subscription accepts still allocates nothing. + /// + /// /// The predicate is deliberately handed the *aliasing* event rather than the copy, so a /// rejected frame costs no allocation at all — the copy is made only once the frame is /// known to be going into the buffer. Both carry the same @@ -133,7 +141,14 @@ public void Reconfigure(Func? predicate) /// payload, which is why the predicate must not retain what it is given. /// /// - internal void TryDeliver(in CanFrameView view, bool isEcho, TimeSpan receiveTimestamp) + /// The observed frame, aliasing the adapter's RX lease. + /// Whether the bus reported this frame as the host's own echo. + /// The adapter's receive timestamp for this frame. + /// + /// The service-owned copy of this frame's payload, shared across one dispatch of one + /// frame. Null until some subscription accepts the frame; set by whichever does so first. + /// + internal void TryDeliver(in CanFrameView view, bool isEcho, TimeSpan receiveTimestamp, ref byte[]? ownedPayload) { if (isEcho && !_includeEcho) return; @@ -148,7 +163,8 @@ internal void TryDeliver(in CanFrameView view, bool isEcho, TimeSpan receiveTime return; } - var owned = new CanFrameView(view.FrameKind, view.ID, view.Data.ToArray(), view.Flags); + ownedPayload ??= view.Data.ToArray(); + var owned = new CanFrameView(view.FrameKind, view.ID, ownedPayload, view.Flags); _channel.Writer.TryWrite(new CanFrameEvent(owned, isEcho, receiveTimestamp)); } diff --git a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs index 50c8667..e2d09ed 100644 --- a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs @@ -812,13 +812,67 @@ public void Reconfigure_Does_Not_Change_The_Echo_Choice() DrainIds(loud).Should().Equal(0x600, 0x601); } + // The payload copy that shields a buffered frame from the adapter's RX lease is made once per + // *frame*, not once per matching subscription: the reason for it -- the lease may be disposed + // before anyone reads -- is a property of the frame, and what a subscriber receives is a + // read-only view either way. With n subscriptions matching, this is n-1 array allocations and + // copies per frame that no longer happen on the dispatch hot path. + [Fact] + public void Two_Subscriptions_Receiving_The_Same_Frame_Share_One_Payload_Copy() + { + using var bus = ControllableBus.EchoCapable(NewSession()); + using var service = new CanBusService(bus); + using var first = service.Subscribe(); + using var second = service.Subscribe(); + + var source = new byte[] { 1, 2, 3, 4 }; + bus.RaiseObserved(CanFrame.Classic(0x123, source), isEcho: false); + + first.TryRead(out var a).Should().BeTrue(); + second.TryRead(out var b).Should().BeTrue(); + + MemoryMarshal.TryGetArray(a.Frame.Data, out var segA).Should().BeTrue(); + MemoryMarshal.TryGetArray(b.Frame.Data, out var segB).Should().BeTrue(); + ReferenceEquals(segA.Array, segB.Array).Should().BeTrue( + "one copy per frame is shared by every subscription that buffers it"); + + // Still a copy, not the caller's own array: that is the whole point of making one. + ReferenceEquals(segA.Array, source).Should().BeFalse( + "the buffered payload must not alias the frame the adapter handed out"); + a.Frame.Data.ToArray().Should().Equal(source); + } + // CanFrameEvent equality must compare payload *bytes*, not which array holds them. // // CanFrameView is a record struct over a ReadOnlyMemory, and its generated equality // tests the memory segment rather than the contents. Delegating to it looked harmless and was - // not: TryDeliver allocates a fresh array per delivered frame, so the same frame fanned out to - // two subscriptions produced two events that compared unequal — the opposite of what `a == b` - // means for a value type. + // not: two events describing the same frame over two different arrays compared unequal — the + // opposite of what `a == b` means for a value type. The events are built here rather than + // drained from two subscriptions (which is how the bug was originally found) so that the + // distinct-buffer case stays covered no matter how many copies the demux makes. + [Fact] + public void Events_Over_Distinct_Buffers_With_Equal_Bytes_Are_Equal() + { + var timestamp = TimeSpan.FromMilliseconds(7); + var a = new CanFrameEvent( + new CanFrameView(CanFrameType.Can20, 0x123, new byte[] { 1, 2, 3, 4 }, FrameFlags.None), + isEcho: false, timestamp); + var b = new CanFrameEvent( + new CanFrameView(CanFrameType.Can20, 0x123, new byte[] { 1, 2, 3, 4 }, FrameFlags.None), + isEcho: false, timestamp); + + MemoryMarshal.TryGetArray(a.Frame.Data, out var segA).Should().BeTrue(); + MemoryMarshal.TryGetArray(b.Frame.Data, out var segB).Should().BeTrue(); + ReferenceEquals(segA.Array, segB.Array).Should().BeFalse( + "distinct buffers by construction -- that is exactly the case that used to break"); + + a.Should().Be(b); + (a == b).Should().BeTrue(); + a.GetHashCode().Should().Be(b.GetHashCode(), "equal values must hash equally"); + } + + // ... and the same frame fanned out to two subscriptions still produces equal events, which is + // how the equality bug above surfaced in the first place. [Fact] public void Two_Subscriptions_Receiving_The_Same_Frame_Produce_Equal_Events() { @@ -835,12 +889,6 @@ public void Two_Subscriptions_Receiving_The_Same_Frame_Produce_Equal_Events() first.TryRead(out var a).Should().BeTrue(); second.TryRead(out var b).Should().BeTrue(); - // Distinct buffers by construction -- that is exactly the case that used to break. - MemoryMarshal.TryGetArray(a.Frame.Data, out var segA).Should().BeTrue(); - MemoryMarshal.TryGetArray(b.Frame.Data, out var segB).Should().BeTrue(); - ReferenceEquals(segA.Array, segB.Array).Should().BeFalse( - "each subscription buffers its own copy"); - a.Should().Be(b); (a == b).Should().BeTrue(); a.GetHashCode().Should().Be(b.GetHashCode(), "equal values must hash equally"); diff --git a/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs b/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs index 1263593..2f00a30 100644 --- a/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs @@ -378,4 +378,45 @@ public async Task Outstanding_SendConfirmed_Resolves_As_BusOff_Immediately_On_Fa sw.Elapsed.Should().BeLessThan(TimeSpan.FromSeconds(5), "the BusOff path must resolve the confirmation immediately, not via the 30 s timeout"); } + + // Echo matching runs for every echo frame the adapter reports while any send is outstanding, + // and it used to copy that frame's payload into the lookup key just to ask whether anything + // was waiting for it. The key never leaves the lookup, so it can borrow the payload instead. + // + // Measured rather than asserted structurally, because "does not allocate" is precisely the + // claim: the frames below deliberately do not match the outstanding send, so every one of them + // takes the full lookup path. GetAllocatedBytesForCurrentThread is exact for this thread, and + // RaiseObserved delivers synchronously on it, so the only noise is the fixed per-call cost of + // raising the event -- far below the 64-byte payload a copy would add each time. + [Fact] + public async Task Echo_Lookup_Does_Not_Copy_The_Payload_Of_Every_Echo_Frame() + { + using var sender = OpenEcho(); + using var service = new CanBusService(sender); + sender.EchoAcceptedFrames = false; + + // One outstanding send, so the lookup is actually reached (with none, OnFrameObserved + // short-circuits before building a key at all). + var outstanding = service.SendConfirmed(CanFrame.Classic(0x111, new byte[] { 1 }), + TimeSpan.FromSeconds(30)); + + // A 64-byte payload on an ID nothing is waiting for: reached, hashed, compared, no match. + var unmatched = CanFrame.Fd(0x222, new byte[64]); + + const int warmup = 50; + const int measured = 500; + for (var i = 0; i < warmup; i++) sender.RaiseObserved(unmatched, isEcho: true); + + var before = GC.GetAllocatedBytesForCurrentThread(); + for (var i = 0; i < measured; i++) sender.RaiseObserved(unmatched, isEcho: true); + var perFrame = (GC.GetAllocatedBytesForCurrentThread() - before) / (double)measured; + + perFrame.Should().BeLessThan(64, + "the lookup key must borrow the echo payload rather than copy it"); + + outstanding.IsCompleted.Should().BeFalse("none of those echoes matched the pending send"); + + service.Dispose(); + await Assert.ThrowsAnyAsync(() => outstanding); + } } From 45e10c72c580ffeb1995c5997298bb1683552374 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:15:07 +0000 Subject: [PATCH 03/14] fix(rawcan)!: reject CanIdFilter ranges and masks outside their ID space Range(0x18FEF100, 0x18FEF1FF) with the idType forgotten built a filter that matched nothing and reported nothing: Matches() only ever sees IDs already clipped to the filter's ID space, so a standard-space filter over a 29-bit ID is unsatisfiable by construction. The same holds for an acceptance pair whose accCode & accMask requires a bit above 0x7FF (standard) or 0x1FFFFFFF (extended) to be set. Both now throw ArgumentOutOfRangeException naming the likely fix, instead of silently accepting no frames (#53, finding 4). An acceptance *mask* reaching above the ID space stays legal: it merely requires those bits to be zero, which every real ID satisfies. The clipping in Overlaps and the full-width bit walk in RangeIntersectsMask are kept although the factories now reject the inputs that made them load-bearing. They are what makes Overlaps correct independently of how a filter was built, and removing a guard because a new one shadows it is how this repository has acquired regressions before. Their comments now say which line of defence they are. Three existing tests drove out-of-space filters through the factories to reach that clipping and become construction-rejection tests here; the case that a mask above the ID space is still usable keeps its own test. Refs #53 BREAKING CHANGE: CanIdFilter.Range and CanIdFilter.Mask now throw ArgumentOutOfRangeException for bounds or acceptance pairs outside the target ID space. Code that built such a filter was matching nothing at all -- the fix is to pass CanFilterIDType.Extend for 29-bit IDs, or to correct the bound. Filters already inside their ID space are unaffected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- src/CanKit.Pro.RawCan/CanIdFilter.cs | 51 +++++++++++++++++- .../TestCases/CanIdFilterOverlapTests.cs | 54 ++++++++++++------- 2 files changed, 83 insertions(+), 22 deletions(-) diff --git a/src/CanKit.Pro.RawCan/CanIdFilter.cs b/src/CanKit.Pro.RawCan/CanIdFilter.cs index c0e9e84..6a24bc6 100644 --- a/src/CanKit.Pro.RawCan/CanIdFilter.cs +++ b/src/CanKit.Pro.RawCan/CanIdFilter.cs @@ -61,9 +61,25 @@ private CanIdFilter(Kind kind, uint a, uint b, CanFilterIDType idType) /// Minimum ID, inclusive. /// Maximum ID, inclusive. /// Standard or extended ID space. + /// is below . + /// + /// A bound lies outside 's ID space (0x7FF for standard, + /// 0x1FFFFFFF for extended). Such a filter can never match anything, because + /// only ever sees IDs already clipped to that space — the usual + /// cause is a 29-bit ID passed without . + /// public static CanIdFilter Range(uint from, uint to, CanFilterIDType idType = CanFilterIDType.Standard) { if (to < from) throw new ArgumentException("'to' must be greater than or equal to 'from'.", nameof(to)); + + // Fail loudly rather than never matching. A filter built from an out-of-space bound -- + // Range(0x18FEF100, ...) with the idType forgotten is the canonical one -- silently + // accepted no frames and reported nothing, which is the most expensive way for this + // kind of mistake to be found. + var maxId = MaxId(idType); + if (from > maxId) throw OutOfIdSpace(nameof(from), from, idType, maxId); + if (to > maxId) throw OutOfIdSpace(nameof(to), to, idType, maxId); + return new CanIdFilter(Kind.Range, from, to, idType); } @@ -74,8 +90,33 @@ public static CanIdFilter Range(uint from, uint to, CanFilterIDType idType = Can /// Acceptance code. /// Acceptance mask; only the set bits are compared. /// Standard or extended ID space. + /// + /// The pair requires a bit outside 's ID space to be set -- + /// (accCode & accMask) reaches above 0x7FF (standard) or 0x1FFFFFFF (extended). + /// No real CAN ID has those bits set, so the filter could never match. A mask that reaches + /// above the ID space is fine on its own: it then merely requires those bits to be zero, + /// which every ID already satisfies. + /// public static CanIdFilter Mask(uint accCode, uint accMask, CanFilterIDType idType = CanFilterIDType.Standard) - => new CanIdFilter(Kind.Mask, accCode, accMask, idType); + { + var maxId = MaxId(idType); + var required = accCode & accMask; + if ((required & ~maxId) != 0) + throw new ArgumentOutOfRangeException(nameof(accCode), accCode, + $"This filter requires ID bits outside the {idType} ID space to be set " + + $"(accCode & accMask = 0x{required:X}, the space ends at 0x{maxId:X}), so no frame could ever " + + "match it. Pass CanFilterIDType.Extend for a 29-bit ID."); + + return new CanIdFilter(Kind.Mask, accCode, accMask, idType); + } + + private static uint MaxId(CanFilterIDType idType) + => idType == CanFilterIDType.Extend ? ID_EXT_MASK : ID_STD_MASK; + + private static ArgumentOutOfRangeException OutOfIdSpace(string paramName, uint value, CanFilterIDType idType, uint maxId) + => new(paramName, value, + $"0x{value:X} is outside the {idType} ID space (0x0..0x{maxId:X}), so no frame could ever match " + + "this filter. Pass CanFilterIDType.Extend for a 29-bit ID."); /// /// Returns true when matches this filter. @@ -109,7 +150,13 @@ public bool Overlaps(CanIdFilter other) // otherwise a range/mask that reaches past 0x7FF (standard) or 0x1FFFFFFF (extended) // can be reported as overlapping another filter purely on the out-of-space portion, // which no real frame could ever match. - var maxId = IdType == CanFilterIDType.Extend ? ID_EXT_MASK : ID_STD_MASK; + // + // Range and Mask now reject the inputs that made this load-bearing, so the clipping + // below and the full-width walk in RangeIntersectsMask are the second line of defence + // rather than the first. They are kept deliberately: they are what makes this correct + // independently of the factories, and an acceptance mask reaching above the ID space + // is still perfectly legal (it constrains those bits to zero, which every ID meets). + var maxId = MaxId(IdType); return (_kind, other._kind) switch { diff --git a/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs b/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs index 21115da..c3cddea 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs @@ -83,40 +83,54 @@ public void Range_And_Mask_Filters_That_Overlap_Are_Detected() overlappingMask.Overlaps(range).Should().BeTrue("overlap must be symmetric regardless of argument order"); } + // An acceptance mask may reach above the ID space: those bits are then required to be *zero*, + // which every real CAN ID already satisfies, so the filter stays perfectly usable. Only a + // filter that requires an out-of-space bit to be one is impossible, and that one is rejected + // at construction (see the tests below). [Fact] - public void Range_And_Mask_Filters_Honor_Acceptance_Mask_Bits_Above_The_29Bit_Id_Space() + public void A_Mask_Reaching_Above_The_Id_Space_Still_Overlaps_A_Range_It_Shares_Ids_With() { - // No real CAN ID ever has bit 29 set (IDs are at most 29 bits wide), so a mask that - // requires bit 29 to be 1 can never actually be satisfied by any ID -- including every ID - // in 'range'. The range/mask overlap check must honor that acceptance-mask bit even though - // it falls outside the bits a valid range bound can vary over. var range = CanIdFilter.Range(0x100, 0x10F, CanFilterIDType.Extend); - var unsatisfiableMask = CanIdFilter.Mask(accCode: 0x20000100, accMask: 0x20000700, idType: CanFilterIDType.Extend); + var mask = CanIdFilter.Mask(accCode: 0x100, accMask: 0x20000700, idType: CanFilterIDType.Extend); - range.Overlaps(unsatisfiableMask).Should().BeFalse(); - unsatisfiableMask.Overlaps(range).Should().BeFalse("overlap must be symmetric regardless of argument order"); + range.Overlaps(mask).Should().BeTrue(); + mask.Overlaps(range).Should().BeTrue("overlap must be symmetric regardless of argument order"); } + // A filter outside its own ID space never matched anything and reported nothing, which made a + // forgotten idType (a 29-bit ID left on the Standard default) as good as invisible. Both + // factories reject it instead. Matches() only ever sees IDs already clipped to the space, so + // there is no reading under which such a filter could have been meant. [Fact] - public void Range_Filters_Whose_Numeric_Overlap_Lies_Entirely_Above_The_Standard_11Bit_Space_Do_Not_Overlap() + public void Range_Rejects_Bounds_Outside_The_Standard_11Bit_Space() { - // [0x7F0, 0x900] and [0x800, 0x810] intersect numerically, but Matches() only ever sees - // 11-bit standard IDs (<= 0x7FF), so no standard frame can ever match the second filter. - var a = CanIdFilter.Range(0x7F0, 0x900); - var b = CanIdFilter.Range(0x800, 0x810); + var forgottenIdType = () => CanIdFilter.Range(0x18FEF100, 0x18FEF1FF); + forgottenIdType.Should().Throw() + .WithMessage("*Extend*", "the message must name the fix, not just the fault"); - a.Overlaps(b).Should().BeFalse(); - b.Overlaps(a).Should().BeFalse("overlap must be symmetric regardless of argument order"); + var upperBoundEscapes = () => CanIdFilter.Range(0x7F0, 0x900); + upperBoundEscapes.Should().Throw(); } [Fact] - public void Range_Filters_Whose_Numeric_Overlap_Lies_Entirely_Above_The_Extended_29Bit_Space_Do_Not_Overlap() + public void Range_Rejects_Bounds_Outside_The_Extended_29Bit_Space() { - var a = CanIdFilter.Range(0x1FFFFFF0, 0x20000100, CanFilterIDType.Extend); - var b = CanIdFilter.Range(0x20000000, 0x20000010, CanFilterIDType.Extend); + var act = () => CanIdFilter.Range(0x1FFFFFF0, 0x20000100, CanFilterIDType.Extend); + act.Should().Throw(); - a.Overlaps(b).Should().BeFalse(); - b.Overlaps(a).Should().BeFalse("overlap must be symmetric regardless of argument order"); + var entirelyOutside = () => CanIdFilter.Range(0x20000000, 0x20000010, CanFilterIDType.Extend); + entirelyOutside.Should().Throw(); + } + + [Fact] + public void Mask_Rejects_A_Code_Requiring_A_Bit_Outside_The_Id_Space() + { + // Bit 29 set in both code and mask: no CAN ID has that bit, so nothing could ever match. + var extended = () => CanIdFilter.Mask(accCode: 0x20000100, accMask: 0x20000700, idType: CanFilterIDType.Extend); + extended.Should().Throw(); + + var standard = () => CanIdFilter.Mask(accCode: 0x18FEF100, accMask: 0x1FFFFF00); + standard.Should().Throw(); } [Fact] From 11cd32cc0c20ae1aa6bb0e208391d7c7eb48c59f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:21:39 +0000 Subject: [PATCH 04/14] feat(rawcan): give the callback Subscribe an onError channel, and stop the pump from failing unobserved An exception out of the callback overload's onNext was routed to BackgroundExceptionOccurred only when the service happened to be the concrete CanBusService. For any other ICanBusService it was dropped -- and it had to be, because that fault channel is an event on the interface and only the type declaring an event can raise it. There was no way for this extension to report anything at all through a foreign implementation (#53, finding 6). So the extension now takes an optional onError. When given it is the single destination, in preference to the service's own event: a caller who passes it has said where these belong, and one failure arriving through two channels is its own surprise. For a foreign service it is the only destination there is, which the XML docs now say outright rather than leaving the gap implicit. The pump loop is also wrapped: a failure of the enumeration itself (as opposed to one onNext call) used to fault the pump task, which Dispose joins for at most two seconds and then abandons -- so nothing was left to observe it. It reports through the same channel now. The bounded join stays bounded: an onNext that never returns cannot be cancelled from here, and waiting longer would only move the hang into the caller's Dispose. Also replaces the two Interlocked.Exchange calls on Subscription's volatile _criteria field with plain volatile writes (#53, finding 5). Nothing reads the previous value and there is no compare-and-swap; volatile already gives the atomic, unreordered publication FR-RAW-014 needs, and Interlocked implied a guarantee that was never in play. No behaviour change. Tests: a foreign ICanBusService fake proves the handler failure now reaches onError and that delivery continues, and a second test proves onError wins over the service event. Both fail (by timing out on the report) when the onError branch is taken out. API approval regenerated from the .received.txt. Refs #53 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- .../CanBusServiceExtensions.cs | 98 ++++++++++++++----- src/CanKit.Pro.RawCan/Subscription.cs | 13 ++- .../CanKit.Pro.RawCan.approved.txt | 2 +- .../TestCases/RawCanSubscriptionTests.cs | 87 ++++++++++++++++ 4 files changed, 172 insertions(+), 28 deletions(-) diff --git a/src/CanKit.Pro.RawCan/CanBusServiceExtensions.cs b/src/CanKit.Pro.RawCan/CanBusServiceExtensions.cs index 41acb26..6aaa211 100644 --- a/src/CanKit.Pro.RawCan/CanBusServiceExtensions.cs +++ b/src/CanKit.Pro.RawCan/CanBusServiceExtensions.cs @@ -23,10 +23,10 @@ public static class CanBusServiceExtensions /// can never delay delivery to other subscriptions or to the bus's own /// FrameObserved event, because the dispatch hot path never waits on a /// subscriber's consumer. An exception thrown by is isolated - /// per frame -- delivery continues with the next frame -- and, when - /// is a , routed through - /// , the same fault channel every - /// other background failure in this service uses. + /// per frame -- delivery continues with the next frame -- and reported through + /// if one was given, otherwise through + /// when + /// is a . /// /// The service to subscribe on. /// Invoked for each accepted frame, in arrival order. @@ -39,18 +39,34 @@ public static class CanBusServiceExtensions /// Whether the callback also sees the local host's own transmit echoes; false by default, /// for the reason given on . /// + /// + /// Where a failure of -- or of the delivery pump itself -- is + /// reported. When given it is the single destination, in preference to the service's fault + /// event, because a caller who passes it has said where these belong. + /// + /// It is also the *only* destination available for an + /// implementation other than : + /// is an event, and only the type + /// that declares it can raise it, so this extension cannot report through a foreign + /// implementation's fault channel however much it would like to. Without + /// such an exception has nowhere to go and is dropped -- the + /// alternative, letting it out of the pump, would silently end delivery instead. + /// + /// /// Disposing this stops the subscription and the background delivery task. public static IDisposable Subscribe( this ICanBusService service, Action onNext, Func? predicate = null, int? bufferCapacity = null, - bool includeEcho = false) + bool includeEcho = false, + Action? onError = null) { if (service is null) throw new ArgumentNullException(nameof(service)); if (onNext is null) throw new ArgumentNullException(nameof(onNext)); - return new CallbackSubscription(service.Subscribe(predicate, bufferCapacity, includeEcho), service, onNext); + return new CallbackSubscription( + service.Subscribe(predicate, bufferCapacity, includeEcho), service, onNext, onError); } private sealed class CallbackSubscription : IDisposable @@ -60,7 +76,11 @@ private sealed class CallbackSubscription : IDisposable private readonly AsyncLocal _isOnPump = new(); private int _disposed; - public CallbackSubscription(ISubscription subscription, ICanBusService service, Action onNext) + public CallbackSubscription( + ISubscription subscription, + ICanBusService service, + Action onNext, + Action? onError) { _subscription = subscription; _pumpTask = Task.Run(async () => @@ -70,29 +90,56 @@ public CallbackSubscription(ISubscription subscription, ICanBusService service, // inside it would deadlock until the timeout. Same reentrancy guard as // ProtocolActor.Dispose / _isOnLoop. _isOnPump.Value = true; - await foreach (var frameEvent in _subscription.Frames.ConfigureAwait(false)) + try { - // Completing the channel writer (Subscription.Dispose) does not drop - // items already buffered. Without these checks a self-dispose from - // onNext would skip the pump join and still deliver every remaining - // queued frame after Dispose has returned. - if (Volatile.Read(ref _disposed) != 0) break; - - try - { - onNext(frameEvent); - } - catch (Exception ex) + await foreach (var frameEvent in _subscription.Frames.ConfigureAwait(false)) { - if (service is CanBusService concrete) - concrete.RaiseBackgroundException(ex); - } + // Completing the channel writer (Subscription.Dispose) does not drop + // items already buffered. Without these checks a self-dispose from + // onNext would skip the pump join and still deliver every remaining + // queued frame after Dispose has returned. + if (Volatile.Read(ref _disposed) != 0) break; + + try + { + onNext(frameEvent); + } + catch (Exception ex) + { + Report(service, onError, ex); + } - if (Volatile.Read(ref _disposed) != 0) break; + if (Volatile.Read(ref _disposed) != 0) break; + } + } + catch (Exception ex) + { + // The pump itself failed, not just one handler call. Dispose's join is + // bounded and may already have given up on this task, so nothing else is + // ever going to look at it: report here rather than leave a faulted task + // that nobody observes. + Report(service, onError, ex); } }); } + private static void Report(ICanBusService service, Action? onError, Exception ex) + { + if (onError is not null) + { + // A failing error handler must not take the pump down with it; there is by + // definition nowhere left to report that to. + try { onError(ex); } catch { /* best-effort */ } + return; + } + + if (service is CanBusService concrete) + concrete.RaiseBackgroundException(ex); + + // Otherwise: a foreign ICanBusService and no onError. See the remarks on + // Subscribe -- its fault event is not ours to raise. + } + public void Dispose() { if (Interlocked.Exchange(ref _disposed, 1) != 0) return; // idempotent @@ -103,6 +150,11 @@ public void Dispose() // Best-effort bounded join so a caller who disposes and then immediately tears // down surrounding state doesn't race the last in-flight onNext call; matches the // same dispose-teardown idiom used for background readers throughout this codebase. + // + // Bounded on purpose, and still bounded: an onNext that never returns cannot be + // cancelled from here, so waiting longer would only move the hang into the caller's + // Dispose. What the timeout leaves behind is now a task that reports its own + // failures (see the pump's catch) instead of one nothing will ever observe. try { _pumpTask.Wait(TimeSpan.FromSeconds(2)); } catch { /* best-effort */ } } } diff --git a/src/CanKit.Pro.RawCan/Subscription.cs b/src/CanKit.Pro.RawCan/Subscription.cs index 12b027c..d59a4b6 100644 --- a/src/CanKit.Pro.RawCan/Subscription.cs +++ b/src/CanKit.Pro.RawCan/Subscription.cs @@ -87,20 +87,25 @@ internal Subscription( _channel = Channel.CreateBounded(options); } + // A plain assignment to a volatile field, not Interlocked.Exchange: the whole criteria + // object is swapped in one reference write, no caller reads the previous value, and the + // dispatch path only ever reads. Volatile is exactly the guarantee that buys -- the write + // is atomic and cannot be reordered past what the new criteria object was built from + // (FR-RAW-014). Interlocked said the same thing while implying a compare-and-swap that + // never existed. + /// public void Reconfigure(CanIdFilter filter) { ThrowIfDisposed(); - Interlocked.Exchange(ref _criteria, new FilterCriteria(filter, null)); + _criteria = new FilterCriteria(filter, null); } /// public void Reconfigure(Func? predicate) { ThrowIfDisposed(); - Interlocked.Exchange( - ref _criteria, - predicate is null ? FilterCriteria.AcceptAll : new FilterCriteria(null, predicate)); + _criteria = predicate is null ? FilterCriteria.AcceptAll : new FilterCriteria(null, predicate); } /// diff --git a/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt b/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt index 0328072..8b2c669 100644 --- a/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt +++ b/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt @@ -19,7 +19,7 @@ namespace CanKit.Pro.RawCan } public static class CanBusServiceExtensions { - public static System.IDisposable Subscribe(this CanKit.Pro.RawCan.ICanBusService service, System.Action onNext, System.Func? predicate = null, int? bufferCapacity = default, bool includeEcho = false) { } + public static System.IDisposable Subscribe(this CanKit.Pro.RawCan.ICanBusService service, System.Action onNext, System.Func? predicate = null, int? bufferCapacity = default, bool includeEcho = false, System.Action? onError = null) { } } public readonly struct CanFrameEvent : System.IEquatable { diff --git a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs index e2d09ed..d186602 100644 --- a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs @@ -323,6 +323,93 @@ public async Task Callback_Subscribe_Handler_Exception_Is_Surfaced_And_Delivery_ await secondReceived.Task.WaitAsync(ShortTimeout); } + // The fault channel above is an *event on ICanBusService*, and only the type declaring an + // event can raise it -- so for any implementation other than CanBusService the extension has + // no way to report a failing handler, and used to drop it. onError is that way. + [Fact] + public async Task Callback_Subscribe_Reports_A_Handler_Failure_Through_OnError_For_A_Foreign_Service() + { + var session = NewSession(); + using var sender = Open(session, 0); + using var receiver = Open(session, 1); + using var inner = new CanBusService(receiver); + using var service = new ForeignCanBusService(inner); + + var observed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var secondReceived = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var calls = 0; + + using var subscription = service.Subscribe( + _ => + { + if (Interlocked.Increment(ref calls) == 1) throw new InvalidOperationException("boom"); + secondReceived.TrySetResult(true); + }, + onError: ex => observed.TrySetResult(ex)); + + sender.Transmit(CanFrame.Classic(0x100, new byte[] { 1 })); + var ex = await observed.Task.WaitAsync(ShortTimeout); + ex.Should().BeOfType().Which.Message.Should().Be("boom"); + + // Still isolated per frame, exactly as with the service's own fault channel. + sender.Transmit(CanFrame.Classic(0x100, new byte[] { 2 })); + await secondReceived.Task.WaitAsync(ShortTimeout); + } + + // With onError given it is the single destination: the caller said where these belong, and a + // report arriving twice through two channels is its own kind of surprise. + [Fact] + public async Task Callback_Subscribe_OnError_Takes_Precedence_Over_The_Service_Fault_Event() + { + var session = NewSession(); + using var sender = Open(session, 0); + using var receiver = Open(session, 1); + using var service = new CanBusService(receiver); + + var throughEvent = 0; + service.BackgroundExceptionOccurred += (_, _) => Interlocked.Increment(ref throughEvent); + + var observed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using var subscription = service.Subscribe( + _ => throw new InvalidOperationException("boom"), + onError: ex => observed.TrySetResult(ex)); + + sender.Transmit(CanFrame.Classic(0x100, new byte[] { 1 })); + await observed.Task.WaitAsync(ShortTimeout); + + await Task.Delay(100); // give a second, wrong report time to arrive + Volatile.Read(ref throughEvent).Should().Be(0, "onError is the destination the caller chose"); + } + + // An ICanBusService that is not a CanBusService: everything forwarded, so the only thing that + // differs from using the concrete service is that its fault event is not ours to raise. + private sealed class ForeignCanBusService(ICanBusService inner) : ICanBusService + { + public ICanBus Bus => inner.Bus; + + public int SubscriptionCount => inner.SubscriptionCount; + + // Never raised: this fake has no faults of its own, and the point of the test above is + // precisely that CanBusServiceExtensions cannot raise it either. +#pragma warning disable CS0067 + public event EventHandler? BackgroundExceptionOccurred; +#pragma warning restore CS0067 + + public ISubscription Subscribe(Func? predicate = null, int? bufferCapacity = null, bool includeEcho = false) + => inner.Subscribe(predicate, bufferCapacity, includeEcho); + + public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) + => inner.Subscribe(filter, bufferCapacity, includeEcho); + + public IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions() + => inner.FindOverlappingFilterSubscriptions(); + + public Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, CancellationToken cancellationToken = default) + => inner.SendConfirmed(frame, timeout, cancellationToken); + + public void Dispose() { /* the inner service is owned by the test */ } + } + // FR-RAW-012: creating and disposing N subscriptions leaves no entries in the service registry. [Fact] public void Disposing_Subscriptions_Leaves_No_Registry_Entries() From 44628e9bc09777105fda6298e227d3fa6e670df7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:24:39 +0000 Subject: [PATCH 05/14] test(rawcan): fail the build if CanKit ever claims the borrowed 6002..6005 error codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ProtocolErrorCodes occupies CanKitErrorCode 6002..6005, which the pinned CanKit (0.5.6, verified against the packaged enum: the 6000 range holds only TransportOperationFailed = 6001) does not define (#53, finding 7). The numbers are not moved. They are chosen so that an upstream adoption of these four codes deletes ProtocolErrorCodes and changes nothing for callers, which is what docs/upstream-candidates.md § 1 promises, and any other range gives that up in exchange for a collision risk that is smaller but not gone -- CanKit allocates in 1000-blocks up to 9000 and could grow into whatever range we picked. What the finding is right about is that the collision would be silent: an upstream 6002 would make ProtocolTimeout render an unrelated name and compare equal to an unrelated failure, with nothing failing. So the risk gets a tripwire instead of a relocation -- the test fails on the CanKit bump that claims one of the four, while that bump is still a one-line version change under review. Verified to have teeth by pointing it at 6001, which upstream does define: it fails with the message the real case would print. Refs #53 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- .../TestCases/Nfr006ErrorArchitectureTests.cs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/CanKit.Pro.Tests/TestCases/Nfr006ErrorArchitectureTests.cs b/tests/CanKit.Pro.Tests/TestCases/Nfr006ErrorArchitectureTests.cs index 262317c..c473123 100644 --- a/tests/CanKit.Pro.Tests/TestCases/Nfr006ErrorArchitectureTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/Nfr006ErrorArchitectureTests.cs @@ -77,4 +77,37 @@ public void Protocol_Exceptions_Map_To_The_Documented_ErrorCodes() (new CanOpenTransportException("x")) .ErrorCode.Should().Be(CanKitErrorCode.TransportOperationFailed); } + + // ProtocolErrorCodes deliberately occupies CanKitErrorCode 6002..6005, continuing the 6000 + // range CanKit reserves for transport and protocol errors but does not populate past 6001. + // docs/upstream-candidates.md explains why: the numbers are the ones an upstream adoption of + // these four codes would produce, so adopting them there deletes that file and changes nothing + // else. + // + // What makes that a bet rather than a plan is that CanKit could define one of those numbers + // for something of its own. Nothing would break loudly if it did -- ProtocolTimeout would + // simply start rendering an unrelated upstream name, and two different failures would compare + // equal. This is the tripwire: it fails on the CanKit bump that claims one of them, while it + // is still a one-line version change under review rather than a mystery in a released package. + [Fact] + public void The_Borrowed_Protocol_Error_Codes_Are_Still_Unclaimed_Upstream() + { + var borrowed = new[] + { + ProtocolErrorCodes.ProtocolTimeout, + ProtocolErrorCodes.ProtocolPeerAbort, + ProtocolErrorCodes.ProtocolNegativeResponse, + ProtocolErrorCodes.AddressClaimFailed, + }; + + foreach (var code in borrowed) + { + Enum.IsDefined(typeof(CanKitErrorCode), code).Should().BeFalse( + $"CanKitErrorCode {(int)code} is claimed by the CanKit version this build pins, so " + + "ProtocolErrorCodes now collides with it. Either upstream adopted these four codes " + + "-- in which case ProtocolErrorCodes and docs/upstream-candidates.md § 1 go away and " + + "callers switch to the enum members -- or it took the number for something else, and " + + "these four move to a range it does not use."); + } + } } From 46db7cbe3a89f2bcccb195f042c7a5f55175896d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:34:46 +0000 Subject: [PATCH 06/14] feat(rawcan)!: replace the overlap tuple pair with a named FilterOverlap carrying the shared ID range FindOverlappingFilterSubscriptions() returned IReadOnlyList<(ISubscription First, ISubscription Second)>. The relation is symmetric -- there is no first and no second, only two subscriptions that share ID space -- so the element names read as if the order meant something, and they dragged a [return: TupleElementNames] attribute into the public API surface. More to the point, the pair could not grow a third piece of information without changing the return type again (#82). So it returns IReadOnlyList, and the type carries what a caller diagnosing an unexpected overlap actually wants: *where* the two collide. LowestSharedId/HighestSharedId are the smallest and largest CAN ID both filters accept -- exact for range filters, an inclusive hull for acceptance-mask filters, which accept a scattered set. It destructures to (A, B) for callers that only want the pair. Computing the range is a generalisation of the search Overlaps already ran, not a second one: the same bit walk now returns the smallest or largest satisfying ID instead of just whether one exists, and all four range/mask combinations reduce to one call of it. Overlaps delegates, so there is a single implementation rather than two that can drift. Per ADR 0001 this is a replacement rather than an addition: breaking changes are expected before the 1.3.0 tag, they map to a minor bump, and src/ deliberately contains no [Obsolete] member -- the additive route the issue originally prescribed would have added the first one, permanently, with no major version scheduled to remove it. This supersedes that section of #82, as recorded there. Tests: the reported range is checked against a brute-force sweep of the entire 11-bit ID space for all 169 ordered pairs of 13 filters, which also pins the Overlaps behaviour the shared search had to reproduce; the existing overlap tests all still pass unchanged. Inverting the high witness's preference fails three of them, so the range assertions have teeth. Five test fakes implementing ICanBusService, the two docs pages and the FR-RAW-041 rows in the SRS and arc42 move with the signature. API approval regenerated from the .received.txt; the TupleElementNames attribute is gone from the surface. Closes #82 BREAKING CHANGE: ICanBusService.FindOverlappingFilterSubscriptions() now returns IReadOnlyList instead of IReadOnlyList<(ISubscription First, ISubscription Second)>. Positional destructuring -- foreach (var (a, b) in ...) -- keeps working unchanged. Member access moves from pair.First/pair.Second to overlap.A/overlap.B, and the overlapping ID range is available as overlap.LowestSharedId/HighestSharedId. Implementations of ICanBusService must update the member's return type. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- docs/architecture/arc42-CanKit.Pro.md | 15 ++- docs/getting-started.md | 10 +- docs/requirements/SRS-CanKit.Pro.md | 2 +- src/CanKit.Pro.Addressing/README.md | 3 +- src/CanKit.Pro.RawCan/CanBusService.cs | 8 +- src/CanKit.Pro.RawCan/CanIdFilter.cs | 110 ++++++++++++++---- src/CanKit.Pro.RawCan/FilterOverlap.cs | 48 ++++++++ src/CanKit.Pro.RawCan/ICanBusService.cs | 13 ++- .../CanKit.Pro.RawCan.approved.txt | 19 +-- .../TestCases/CanIdFilterOverlapTests.cs | 101 +++++++++++++++- .../IsoTp/IsoTpChannelIntegrationTests.cs | 8 +- .../TestCases/J1939TpTests.cs | 2 +- .../TestCases/RawCanSubscriptionTests.cs | 2 +- 13 files changed, 283 insertions(+), 58 deletions(-) create mode 100644 src/CanKit.Pro.RawCan/FilterOverlap.cs diff --git a/docs/architecture/arc42-CanKit.Pro.md b/docs/architecture/arc42-CanKit.Pro.md index 68ddaed..0127768 100644 --- a/docs/architecture/arc42-CanKit.Pro.md +++ b/docs/architecture/arc42-CanKit.Pro.md @@ -430,7 +430,7 @@ flowchart TB | Multi-Protokoll-Demux | (2) | Ein RX-Strom → N unabhängige gefilterte Consumer, **ohne** konkurrierendes `ReceiveAsync`. **Umgesetzt** im neuen Paket `CanKit.Pro.RawCan` (`ICanBusService`/`CanBusService` + `ISubscription`): je Subscription ein eigener bounded Drop-Oldest-Channel (FR-RAW-011), Fast-Path `CanIdFilter` (ID-Range/Maske) neben generischem `Func` (FR-RAW-010/013), deterministisches Dispose (FR-RAW-012). Das gelieferte Element ist `CanFrameEvent` = Frame + `IsEcho` + Empfangszeitstempel; Echos werden nur an Subscriptions ausgeliefert, die sie mit `includeEcho: true` angefordert haben (FR-RAW-015, [#23](https://github.com/dborgards/CanKit.Pro/issues/23)). Baut ausschließlich auf `ICanBus.FrameObserved`, kein Adapter-Eingriff. | `ICanBusService.Subscribe(filter, includeEcho) → ISubscription { IAsyncEnumerable Frames; }` | `FR-RAW-010..013`, `FR-RAW-015` | | Frame-Ownership-Vertrag | (1) | Verbindliche Lease-Regeln (siehe 8.1); verhindert Use-after-free/Double-Dispose. **Kernmechanik umgesetzt** (`OwnMemory`-Fix, `CanFrame.Duplicate`, Virtual-Hub-Broadcast per Kopie); ausstehend: TX-Lease für übrige L0-Adapter/ISO-TP-Scheduler. | Vertragsdoku + `OwnMemory`-Fix (Review §1.5) | `FR-RAW-OWN-*` | | TX-Confirm | (4) | Einheitliche „gesendet"-Bestätigung, egal ob Hardware-Echo vorhanden. **Umgesetzt** in `CanKit.Pro.RawCan` (`ICanBusService.SendConfirmed`): FIFO-Echo-Matching je (ID, Payload) für gleichzeitige inhaltsgleiche Sendevorgänge (FR-RAW-031), dokumentierte Treiber-Akzeptanz-Approximation ohne Echo (FR-RAW-032), beobachtbare Fehlschläge statt Hängen bei Timeout/BusOff/Ablehnung (FR-RAW-033), konfigurierbarer Timeout je Aufruf (FR-RAW-034). | `TxConfirmation { Confirmed; Timestamp; IsApproximated; FailureReason; }` | `FR-RAW-030..034` | -| Adressierungs-Helfer | – | 11/29-bit, Extended/Mixed/NormalFixed (bislang nur als Einzelfall in `IsoTpEndpoint` vorhanden). **Umgesetzt** als eigenständiges, abhängigkeitsfreies Paket `CanKit.Pro.Addressing`: validierte 11-/29-Bit-ID-Prüfung (`CanIdRange`), allgemeine J1939-PGN/Priorität/PDU-Format/Quelladresse-Komposition/-Dekomposition (`J1939Id`/`J1939Fields`, FR-RAW-040) — verallgemeinert die zuvor auf eine feste Diagnose-PGN beschränkte 29-Bit-Konstruktion aus `IsoTpEndpoint.CreateNormalFixed`. Zusätzlich `CanIdFilter.Overlaps` sowie `ICanBusService.FindOverlappingFilterSubscriptions()` in `CanKit.Pro.RawCan` zur Erkennung überlappender Subscription-Filter (FR-RAW-041, Should). | ID-Bau/-Zerlegung, PGN/Prio-Helfer | `FR-RAW-ADDR-*` | +| Adressierungs-Helfer | – | 11/29-bit, Extended/Mixed/NormalFixed (bislang nur als Einzelfall in `IsoTpEndpoint` vorhanden). **Umgesetzt** als eigenständiges, abhängigkeitsfreies Paket `CanKit.Pro.Addressing`: validierte 11-/29-Bit-ID-Prüfung (`CanIdRange`), allgemeine J1939-PGN/Priorität/PDU-Format/Quelladresse-Komposition/-Dekomposition (`J1939Id`/`J1939Fields`, FR-RAW-040) — verallgemeinert die zuvor auf eine feste Diagnose-PGN beschränkte 29-Bit-Konstruktion aus `IsoTpEndpoint.CreateNormalFixed`. Zusätzlich `CanIdFilter.Overlaps` sowie `ICanBusService.FindOverlappingFilterSubscriptions()` in `CanKit.Pro.RawCan` zur Erkennung überlappender Subscription-Filter (FR-RAW-041, Should); jeder Treffer wird als benannter `FilterOverlap` (beide Subscriptions plus geteilter ID-Bereich) gemeldet. | ID-Bau/-Zerlegung, PGN/Prio-Helfer | `FR-RAW-ADDR-*` | | Aktor-/Threading-Modell | (3) | Genau ein Bearbeitungs-Thread/Mailbox pro Protokollinstanz; kein geteilter mutabler State. **Umgesetzt** als eigenständiges, abhängigkeitsfreies Paket `CanKit.Pro.Actor` (siehe ADR-6): ereignisgetriebener Loop (kein Busy-Loop, FR-RAW-022), je Instanz wählbarer Ausführungskontext (`ActorExecutionMode`: `DedicatedThread`/`ThreadPool`/`SynchronizationContext`, FR-RAW-024), `BackgroundExceptionOccurred` als einziger Kanal für Hintergrundfehler (FR-RAW-023). Vom ISO-TP-Prototyp noch nicht genutzt. | `IProtocolActor { Post(msg); PostAsync(msg); Schedule(delay, cb); }` | `FR-RAW-ACTOR-*` | | Fehler-/Timeout-Infrastruktur | – | Einheitliche Deadline-Verwaltung (ersetzt verstreute ISO-TP-`Deadline`s) und gepushte Bus-Fehlerzustände. **Umgesetzt** als eigenständiges Paket `CanKit.Pro.Reliability` (siehe ADR-11), aufbauend auf `CanKit.Pro.Actor`: `IDeadlineScheduler`/`DeadlineScheduler`/`Deadline` ist eine wiederverwendbare Deadline-Primitive, deren Ablauf über `IProtocolActor.Schedule` auf dem Aktor-Loop tatsächlich eingeplant, geprüft und gemeldet wird — behebt die Klasse „Deadlines werden gepflegt, aber nie geprüft" (Review §1.1 Punkt 10, FR-RAW-050); die Pending→{Expired\|Completed\|Cancelled}-Auflösung ist per `Interlocked`-CAS genau einmal entscheidbar, Ausnahmen aus `onExpired` laufen über den bestehenden `BackgroundExceptionOccurred`-Kanal (kein zweiter Fehlerkanal). `BusStateMonitor`/`BusStateChangedEventArgs`/`BusStateExtensions` pusht `ICanBus.BusState`-Übergänge (ErrWarning/ErrPassive/BusOff sowie Erholung) an Protokollinstanzen — zuverlässig über einen selbst-rearmenden Poll auf dem Aktor-`Schedule` (Standard 50 ms) statt eines freilaufenden Timers, ergänzt um `ErrorFrameReceived`/`FaultOccurred` als Latenz-Hinweise (FR-RAW-051). FR-RAW-052 (reservierte/ungültige Protokollwerte) ist bewusst **zurückgestellt** und dem ISO-TP-Codec-Fix FR-TP-007 zugeordnet, nicht als generische L2-Primitive gebaut. | `IDeadlineScheduler`, `DeadlineScheduler`/`Deadline`, `BusStateMonitor`, `BusStateExtensions` | `FR-RAW-050..051` | @@ -1154,11 +1154,14 @@ STmin-Grenzwerte, SN-Folge, N_Bs/N_Cr-Timeouts gegen Virtual. Umbau bestehender Adapter/Transporte in dieser Umsetzung). Zusätzlich `CanIdFilter.Overlaps` und `ICanBusService.FindOverlappingFilterSubscriptions()` in `CanKit.Pro.RawCan` (FR-RAW-041, Should): erkennt überlappende Range/Mask-Filter unter den aktuell registrierten Subscriptions - als Fehldiagnose-Hilfe bei falsch konfigurierten Protokollinstanzen — Range/Range und - Mask/Mask-Überlappung über direkte Intervall-/Bitvergleiche, Range/Mask-Überlappung über eine - bitweise Existenzsuche (O(Bitbreite), kein Aufzählen einzelner ID-Werte). Abgesichert per - Unit-Test (`tests/CanKit.Tests/TestCases/AddressingTests.cs`, - `tests/CanKit.Tests/TestCases/CanIdFilterOverlapTests.cs`). + als Fehldiagnose-Hilfe bei falsch konfigurierten Protokollinstanzen. Jeder Treffer ist ein + `FilterOverlap` — die beiden Subscriptions (symmetrisch, daher `A`/`B` statt `First`/`Second`) + und der geteilte ID-Bereich, bei Mask-Filtern als einschließende Hülle einer gestreuten Menge. + Alle Kombinationen laufen über dieselbe bitweise Suche (O(Bitbreite), kein Aufzählen einzelner + ID-Werte), die den kleinsten bzw. größten gemeinsamen ID direkt mitliefert, statt nur dessen + Existenz. Abgesichert per Unit-Test (`tests/CanKit.Tests/TestCases/AddressingTests.cs`, + `tests/CanKit.Tests/TestCases/CanIdFilterOverlapTests.cs`, inkl. Abgleich gegen eine + Brute-Force-Absuche des gesamten 11-Bit-ID-Raums). ### ADR-11 (umgesetzt): Fehler-/Timeout-Infrastruktur als eigenständiges Paket - **Kontext:** L3-Protokolle brauchen zeitgebundene Zustandsübergänge (ISO-TP N_Bs/N_Cr, J1939-, diff --git a/docs/getting-started.md b/docs/getting-started.md index fb20bf8..9e2fe3f 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -88,10 +88,16 @@ CANopen node on a flagging adapter does see its own PDOs and heartbeats. If two instances were meant to have disjoint ID spaces, you can check rather than hope: ```csharp -foreach (var (first, second) in service.FindOverlappingFilterSubscriptions()) - logger.Warning("Overlapping subscriptions: {A} and {B}", first, second); +foreach (var overlap in service.FindOverlappingFilterSubscriptions()) + logger.Warning("Overlapping subscriptions {A} and {B}, sharing IDs 0x{Low:X}..0x{High:X}", + overlap.A, overlap.B, overlap.LowestSharedId, overlap.HighestSharedId); ``` +Each result is a `FilterOverlap`: the two subscriptions that share ID space — the relation is +symmetric, so `A` and `B` say nothing beyond registration order — and the range they share. For two +range filters every ID in between is shared as well; for acceptance-code/mask filters the bounds are +a hull around a scattered set. If you only want the pair, it destructures: `var (a, b) = overlap;`. + ## Did the frame actually go out? `ICanBus.Transmit` tells you the driver accepted the frame, which is not the same thing. Where the diff --git a/docs/requirements/SRS-CanKit.Pro.md b/docs/requirements/SRS-CanKit.Pro.md index 400bac9..b6f773c 100644 --- a/docs/requirements/SRS-CanKit.Pro.md +++ b/docs/requirements/SRS-CanKit.Pro.md @@ -373,7 +373,7 @@ Verweise auf Architektur-Bausteine nutzen die in Abschnitt 2.1 definierten Schic | FR-RAW-010..015 | L2 – *Demultiplex-Hub/Subscription-Manager* (umgesetzt in `CanKit.Pro.RawCan`), aufbauend auf L1 `ICanBus.FrameObserved` (`src/core/CanKit.Abstractions/API/Can/ICanBus.cs`) | Virtual-Loopback-Integrationstest, Lasttest | | FR-RAW-020..024 | L2 – *Protokollinstanz-Aktor/Scheduler* (umgesetzt als eigenständiges `CanKit.Pro.Actor`, `IProtocolActor`/`ProtocolActor`); Referenzimplementierung noch **nicht** umgestellt in L3 `IsoTpScheduler` (`src/transports/CanKit.Transport.IsoTp/IsoTpScheduler.cs`, funktional defekt, s. Review §1.1) | Stress-/Nebenläufigkeitstest | | FR-RAW-030..034 | L2 – *TX-Confirm-Abstraktion* (umgesetzt in `CanKit.Pro.RawCan`), aufbauend auf L1 `CanFeature.Echo`, `ITransceiver.Transmit` | Virtual-Loopback-Integrationstest (mit/ohne Echo) | -| FR-RAW-040..041 | L2 – *Adressierungs-Helfer* (umgesetzt als eigenständiges `CanKit.Pro.Addressing`: `CanIdRange`, `J1939Id`/`J1939Fields`; FR-RAW-041 als `CanIdFilter.Overlaps`/`ICanBusService.FindOverlappingFilterSubscriptions()` in `CanKit.Pro.RawCan`) | Unit-Test | +| FR-RAW-040..041 | L2 – *Adressierungs-Helfer* (umgesetzt als eigenständiges `CanKit.Pro.Addressing`: `CanIdRange`, `J1939Id`/`J1939Fields`; FR-RAW-041 als `CanIdFilter.Overlaps`/`ICanBusService.FindOverlappingFilterSubscriptions()` in `CanKit.Pro.RawCan`, Ergebnis je Treffer als benannter `FilterOverlap` mit beiden Subscriptions und dem geteilten ID-Bereich) | Unit-Test | | FR-RAW-050..052 | L2 – *Fehler-/Timeout-Infrastruktur* (FR-RAW-050/051 umgesetzt als eigenständiges `CanKit.Pro.Reliability`: `IDeadlineScheduler`/`DeadlineScheduler`/`Deadline` als aktorgetriebene Deadline-Primitive, deren Ablauf über `IProtocolActor.Schedule` tatsächlich geprüft und gemeldet wird (FR-RAW-050); `BusStateMonitor`/`BusStateChangedEventArgs`/`BusStateExtensions` für gepushte `ICanBus.BusState`-Übergänge (FR-RAW-051), aufbauend auf `CanKit.Pro.Actor` und L1 `ICanBus.BusState`). FR-RAW-052 (reservierte/ungültige Protokollwerte) bleibt **zurückgestellt** und dem künftigen ISO-TP-Codec-Fix FR-TP-007 zugeordnet (Review §1.1 Punkt 6), da protokollspezifisch statt generische L2-Primitive. | Unit-Test, Integrationstest | | FR-TP-001..020 | L3 – ISO-TP-Transport, `CanKit.Transport.IsoTp` (`IsoTpChannelCore`, `IsoTpScheduler`, `FrameCodec`, `Router`, `Deadline`/`QueuedDeadline`) | Unit-Test (Codec/Timing), Virtual-Loopback-Integrationstest, HIL-Stichprobe | | FR-TP-030..035 | L3 – *J1939-Transport* (geplant, neues Paket `CanKit.Transport.J1939` analog `IIsoTpRegister`-Muster) | Virtual-Loopback-Integrationstest | diff --git a/src/CanKit.Pro.Addressing/README.md b/src/CanKit.Pro.Addressing/README.md index c68b36e..e0a68f0 100644 --- a/src/CanKit.Pro.Addressing/README.md +++ b/src/CanKit.Pro.Addressing/README.md @@ -58,7 +58,8 @@ J1939Name.CompareClaimPriority(name, sameName); // 0; lower unsigned NAME wins a `CanKit.Pro.RawCan`'s `CanIdFilter` also gained an `Overlaps(CanIdFilter other)` method and `ICanBusService.FindOverlappingFilterSubscriptions()` (FR-RAW-041, Should): a diagnostic to catch misconfigured protocol instances whose ID-range/mask subscriptions were meant to be disjoint but -overlap. +overlap. Each hit comes back as a `FilterOverlap` naming the two subscriptions and the range of CAN +IDs they share, so the report says where the collision is and not only that there is one. ## Install diff --git a/src/CanKit.Pro.RawCan/CanBusService.cs b/src/CanKit.Pro.RawCan/CanBusService.cs index a4041a8..df24fb9 100644 --- a/src/CanKit.Pro.RawCan/CanBusService.cs +++ b/src/CanKit.Pro.RawCan/CanBusService.cs @@ -97,13 +97,13 @@ public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, b => AddSubscription(idFilter: filter, predicate: null, bufferCapacity, includeEcho); /// - public IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions() + public IReadOnlyList FindOverlappingFilterSubscriptions() { // Snapshot read, same lock-free discipline as the dispatch hot path -- this is a // diagnostic call, not something exercised per-frame, but there's no reason to take // _gate for a read when the existing snapshot already gives a consistent view. var subscriptions = _snapshot; - var overlaps = new List<(ISubscription, ISubscription)>(); + var overlaps = new List(); for (var i = 0; i < subscriptions.Length; i++) { @@ -111,8 +111,8 @@ public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, b for (var j = i + 1; j < subscriptions.Length; j++) { if (subscriptions[j].IsDisposed || subscriptions[j].IdFilter is not { } filterJ) continue; - if (filterI.Overlaps(filterJ)) - overlaps.Add((subscriptions[i], subscriptions[j])); + if (filterI.TryGetSharedIdRange(filterJ, out var lowest, out var highest)) + overlaps.Add(new FilterOverlap(subscriptions[i], subscriptions[j], lowest, highest)); } } diff --git a/src/CanKit.Pro.RawCan/CanIdFilter.cs b/src/CanKit.Pro.RawCan/CanIdFilter.cs index 6a24bc6..eb7ce66 100644 --- a/src/CanKit.Pro.RawCan/CanIdFilter.cs +++ b/src/CanKit.Pro.RawCan/CanIdFilter.cs @@ -141,8 +141,24 @@ public bool Matches(in CanFrameView frame) /// different spaces (Standard vs. Extended) never overlap, since a /// frame is never both. /// - public bool Overlaps(CanIdFilter other) + public bool Overlaps(CanIdFilter other) => TryGetSharedIdRange(other, out _, out _); + + /// + /// As , but also reports *where* the two filters collide: the + /// smallest and largest CAN ID both accept. For two range filters every ID in between is + /// shared too; for acceptance-mask filters the pair is an inclusive hull, since a mask + /// filter accepts a scattered set rather than a contiguous run. + /// + /// + /// Internal because is what carries this outward today. It can + /// be promoted to the public surface later without breaking anything if callers want to + /// ask two bare filters directly. + /// + internal bool TryGetSharedIdRange(CanIdFilter other, out uint lowestSharedId, out uint highestSharedId) { + lowestSharedId = 0; + highestSharedId = 0; + if (IdType != other.IdType) return false; // Matches() only ever sees IDs already clipped to the ID space (via @@ -152,34 +168,67 @@ public bool Overlaps(CanIdFilter other) // which no real frame could ever match. // // Range and Mask now reject the inputs that made this load-bearing, so the clipping - // below and the full-width walk in RangeIntersectsMask are the second line of defence - // rather than the first. They are kept deliberately: they are what makes this correct + // below and the full-width walk in SharedIdsIn are the second line of defence rather + // than the first. They are kept deliberately: they are what makes this correct // independently of the factories, and an acceptance mask reaching above the ID space // is still perfectly legal (it constrains those bits to zero, which every ID meets). var maxId = MaxId(IdType); + // Every combination reduces to the same question -- which IDs in [lo, hi] satisfy + // (id & mask) == (code & mask) -- with a range contributing bounds and an acceptance + // filter contributing a code/mask pair. A range/range pair constrains no bits, so it + // passes an all-zero mask and the bounds answer on their own. return (_kind, other._kind) switch { - (Kind.Range, Kind.Range) => Math.Max(_a, other._a) <= Math.Min(Math.Min(_b, other._b), maxId), + (Kind.Range, Kind.Range) => SharedIdsIn( + Math.Max(_a, other._a), Math.Min(Math.Min(_b, other._b), maxId), + code: 0, mask: 0, out lowestSharedId, out highestSharedId), // Two acceptance-mask filters overlap iff, on every bit position both masks // constrain, the two required bit patterns agree, and some in-space ID exists // that also satisfies whichever bits either mask alone constrains -- bit // positions constrained by neither filter are always satisfiable by some ID. (Kind.Mask, Kind.Mask) => (_a & _b & other._b) == (other._a & _b & other._b) - && RangeIntersectsMask(0, maxId, (_a & _b) | (other._a & other._b), _b | other._b), - (Kind.Range, Kind.Mask) => RangeIntersectsMask(_a, Math.Min(_b, maxId), other._a, other._b), - (Kind.Mask, Kind.Range) => RangeIntersectsMask(other._a, Math.Min(other._b, maxId), _a, _b), + && SharedIdsIn(0, maxId, (_a & _b) | (other._a & other._b), _b | other._b, + out lowestSharedId, out highestSharedId), + (Kind.Range, Kind.Mask) => SharedIdsIn( + _a, Math.Min(_b, maxId), other._a, other._b, out lowestSharedId, out highestSharedId), + (Kind.Mask, Kind.Range) => SharedIdsIn( + other._a, Math.Min(other._b, maxId), _a, _b, out lowestSharedId, out highestSharedId), _ => false, }; } - // Does some ID in [lo, hi] satisfy (id & mask) == (code & mask)? Bit-by-bit existence - // search from the MSB down, tracking whether the prefix built so far is still exactly - // equal to lo's/hi's prefix ("tight"); once neither bound is tight anymore, every - // remaining ID satisfying the (now unconstrained-by-range) mask trivially exists, so the - // search terminates early rather than enumerating actual ID values. Runs in O(bit-width): - // at most one branch stays "tight" past any given level, so this never actually branches - // into an exponential search despite the naive-looking recursion. + // Does some ID in [lo, hi] satisfy (id & mask) == (code & mask), and if so, which is the + // smallest and which the largest? Both are the same search (see FindWitness) run with + // opposite preferences, so answering "where do they collide" costs a second O(bit-width) + // walk over what "do they collide at all" already had to establish. + private static bool SharedIdsIn(uint lo, uint hi, uint code, uint mask, out uint lowest, out uint highest) + { + lowest = 0; + highest = 0; + + if (lo > hi) return false; // empty range once clipped to the ID space + if (!FindWitness(lo, hi, code, mask, preferHigh: false, out lowest)) return false; + + // Cannot fail once the low witness exists: same feasibility, opposite preference. + FindWitness(lo, hi, code, mask, preferHigh: true, out highest); + return true; + } + + // Is there an ID in [lo, hi] satisfying (id & mask) == (code & mask), and what is the + // smallest (preferHigh: false) or largest (preferHigh: true) such ID? Bit-by-bit search + // from the MSB down, tracking whether the prefix built so far is still exactly equal to + // lo's/hi's prefix ("tight"); once neither bound is tight anymore the range constrains + // nothing further, so the remaining bits are filled in directly -- the mask's required + // bits from the code, the free ones all 0 for the smallest ID and all 1 for the largest -- + // rather than enumerating actual ID values. Runs in O(bit-width): at most one branch stays + // "tight" past any given level, so this never actually branches into an exponential search + // despite the naive-looking recursion. + // + // Preferring a value per bit and falling back to the other is what makes the result the + // true minimum/maximum rather than just some witness: at the most significant bit still + // free, a 0 (respectively 1) beats every completion that puts the opposite bit there, so + // taking it whenever *any* completion below exists is exact. // // Walks the full 32 bits (not just the 29 bits of a valid extended CAN ID): Matches() // performs a plain (id & _b) == (_a & _b) with no restriction on which bits of _b/_a are @@ -187,16 +236,26 @@ public bool Overlaps(CanIdFilter other) // with bits set above bit 28 -- which can never be satisfied by any real CAN ID, since // id's high bits are always 0 -- must be honored here too, or a range/mask pair could be // reported as overlapping when no ID in the range could actually satisfy Matches. - private static bool RangeIntersectsMask(uint lo, uint hi, uint code, uint mask) + private static bool FindWitness(uint lo, uint hi, uint code, uint mask, bool preferHigh, out uint witness) { - if (lo > hi) return false; // empty range once clipped to the ID space + return Search(31, true, true, 0u, out witness); - return Exists(31, true, true); - - bool Exists(int bit, bool loTight, bool hiTight) + bool Search(int bit, bool loTight, bool hiTight, uint prefix, out uint result) { - if (bit < 0) return true; - if (!loTight && !hiTight) return true; + if (bit < 0) + { + result = prefix; + return true; + } + + if (!loTight && !hiTight) + { + // Neither bound constrains the remaining bits any more, so fill them in. + var remaining = bit >= 31 ? uint.MaxValue : (1u << (bit + 1)) - 1; + var required = code & mask & remaining; + result = prefix | (preferHigh ? required | (remaining & ~mask) : required); + return true; + } var b = 1u << bit; var loBit = (lo & b) != 0; @@ -204,14 +263,17 @@ bool Exists(int bit, bool loTight, bool hiTight) var masked = (mask & b) != 0; var forcedBit = (code & b) != 0; - bool TryBit(bool v) + bool TryBit(bool v, out uint r) { + r = 0; if (loTight && !v && loBit) return false; // would fall below lo while still tight if (hiTight && v && !hiBit) return false; // would exceed hi while still tight - return Exists(bit - 1, loTight && v == loBit, hiTight && v == hiBit); + return Search(bit - 1, loTight && v == loBit, hiTight && v == hiBit, + v ? prefix | b : prefix, out r); } - return masked ? TryBit(forcedBit) : TryBit(false) || TryBit(true); + if (masked) return TryBit(forcedBit, out result); + return TryBit(preferHigh, out result) || TryBit(!preferHigh, out result); } } } diff --git a/src/CanKit.Pro.RawCan/FilterOverlap.cs b/src/CanKit.Pro.RawCan/FilterOverlap.cs new file mode 100644 index 0000000..957d3ae --- /dev/null +++ b/src/CanKit.Pro.RawCan/FilterOverlap.cs @@ -0,0 +1,48 @@ +namespace CanKit.Pro.RawCan +{ + /// + /// Two registered -based subscriptions whose ID spaces intersect, and + /// the range of CAN IDs on which they do (FR-RAW-041) — the result of + /// . + /// + /// + /// The relation is symmetric: and are the two subscriptions + /// that share ID space, in registration order, and swapping them describes the same overlap. + /// The names deliberately say nothing more than that — an earlier version of this API returned + /// a (First, Second) tuple, whose element names read as if the order carried meaning. + /// + /// One of the two overlapping subscriptions. + /// The other one. + /// + /// The smallest CAN ID both filters accept. + /// + /// + /// The largest CAN ID both filters accept. Together with this is + /// the answer to "where do they collide?", which is what a caller diagnosing a misconfigured + /// set of protocol instances is actually after. + /// + /// For two range filters every ID in between is shared as well. For acceptance-code/mask + /// filters it is an inclusive hull: every shared ID lies within these bounds, but the IDs in + /// between need not all be shared, because a mask filter accepts a scattered set rather than a + /// contiguous run. + /// + /// + public readonly record struct FilterOverlap( + ISubscription A, + ISubscription B, + uint LowestSharedId, + uint HighestSharedId) + { + /// + /// Destructures just the two subscriptions, for callers that only want to name the pair: + /// foreach (var (a, b) in service.FindOverlappingFilterSubscriptions()). + /// + /// Receives . + /// Receives . + public void Deconstruct(out ISubscription a, out ISubscription b) + { + a = A; + b = B; + } + } +} diff --git a/src/CanKit.Pro.RawCan/ICanBusService.cs b/src/CanKit.Pro.RawCan/ICanBusService.cs index 943cbe7..760a2bc 100644 --- a/src/CanKit.Pro.RawCan/ICanBusService.cs +++ b/src/CanKit.Pro.RawCan/ICanBusService.cs @@ -122,13 +122,18 @@ public interface ICanBusService : IDisposable /// /// Diagnostic: finds every pair of currently registered, still-undisposed - /// -based subscriptions whose ID spaces overlap (FR-RAW-041, - /// "Should") -- helps catch misconfiguration when multiple protocol instances were meant - /// to have disjoint ID ranges but don't. Subscriptions registered via the generic + /// -based subscriptions whose ID spaces overlap, and the range of + /// CAN IDs each pair shares (FR-RAW-041, "Should") -- helps catch misconfiguration when + /// multiple protocol instances were meant to have disjoint ID ranges but don't. + /// Subscriptions registered via the generic /// predicate overload are opaque /// and are not analyzable, so they are skipped. /// - IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions(); + /// + /// One per overlapping pair, each naming the two subscriptions + /// and the ID range on which they collide. Empty when no two filters share ID space. + /// + IReadOnlyList FindOverlappingFilterSubscriptions(); /// /// Sends and asynchronously confirms it was actually sent, using diff --git a/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt b/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt index 8b2c669..8458bb8 100644 --- a/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt +++ b/tests/CanKit.Pro.Tests/ApiApprovals/CanKit.Pro.RawCan.approved.txt @@ -9,10 +9,7 @@ namespace CanKit.Pro.RawCan public int SubscriptionCount { get; } public event System.EventHandler? BackgroundExceptionOccurred; public void Dispose() { } - [return: System.Runtime.CompilerServices.TupleElementNames(new string[] { - "First", - "Second"})] - public System.Collections.Generic.IReadOnlyList> FindOverlappingFilterSubscriptions() { } + public System.Collections.Generic.IReadOnlyList FindOverlappingFilterSubscriptions() { } public System.Threading.Tasks.Task SendConfirmed(CanKit.Abstractions.API.Can.Definitions.CanFrame frame, System.TimeSpan? timeout = default, System.Threading.CancellationToken cancellationToken = default) { } public CanKit.Pro.RawCan.ISubscription Subscribe(CanKit.Pro.RawCan.CanIdFilter filter, int? bufferCapacity = default, bool includeEcho = false) { } public CanKit.Pro.RawCan.ISubscription Subscribe(System.Func? predicate = null, int? bufferCapacity = default, bool includeEcho = false) { } @@ -41,15 +38,21 @@ namespace CanKit.Pro.RawCan public static CanKit.Pro.RawCan.CanIdFilter Mask(uint accCode, uint accMask, CanKit.Abstractions.API.Common.Definitions.CanFilterIDType idType = 0) { } public static CanKit.Pro.RawCan.CanIdFilter Range(uint from, uint to, CanKit.Abstractions.API.Common.Definitions.CanFilterIDType idType = 0) { } } + public readonly struct FilterOverlap : System.IEquatable + { + public FilterOverlap(CanKit.Pro.RawCan.ISubscription A, CanKit.Pro.RawCan.ISubscription B, uint LowestSharedId, uint HighestSharedId) { } + public CanKit.Pro.RawCan.ISubscription A { get; init; } + public CanKit.Pro.RawCan.ISubscription B { get; init; } + public uint HighestSharedId { get; init; } + public uint LowestSharedId { get; init; } + public void Deconstruct(out CanKit.Pro.RawCan.ISubscription a, out CanKit.Pro.RawCan.ISubscription b) { } + } public interface ICanBusService : System.IDisposable { CanKit.Abstractions.API.Can.ICanBus Bus { get; } int SubscriptionCount { get; } event System.EventHandler? BackgroundExceptionOccurred; - [return: System.Runtime.CompilerServices.TupleElementNames(new string[] { - "First", - "Second"})] - System.Collections.Generic.IReadOnlyList> FindOverlappingFilterSubscriptions(); + System.Collections.Generic.IReadOnlyList FindOverlappingFilterSubscriptions(); System.Threading.Tasks.Task SendConfirmed(CanKit.Abstractions.API.Can.Definitions.CanFrame frame, System.TimeSpan? timeout = default, System.Threading.CancellationToken cancellationToken = default); CanKit.Pro.RawCan.ISubscription Subscribe(CanKit.Pro.RawCan.CanIdFilter filter, int? bufferCapacity = default, bool includeEcho = false); CanKit.Pro.RawCan.ISubscription Subscribe(System.Func? predicate = null, int? bufferCapacity = default, bool includeEcho = false); diff --git a/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs b/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs index c3cddea..1864b58 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs @@ -146,8 +146,105 @@ public void FindOverlappingFilterSubscriptions_Reports_Overlapping_Registered_Su var overlaps = service.FindOverlappingFilterSubscriptions(); overlaps.Should().ContainSingle(); - var pair = overlaps[0]; - new[] { pair.First, pair.Second }.Should().BeEquivalentTo(new[] { a, b }); + var overlap = overlaps[0]; + new[] { overlap.A, overlap.B }.Should().BeEquivalentTo(new[] { a, b }); + + // The point of the named type over the old (First, Second) tuple: it can say *where* they + // collide, which is what someone looking at an unexpected overlap wants to know. + overlap.LowestSharedId.Should().Be(0x180); + overlap.HighestSharedId.Should().Be(0x1FF); + + // The two subscriptions still destructure directly, for callers that only want the pair. + var (first, second) = overlap; + first.Should().BeSameAs(overlap.A); + second.Should().BeSameAs(overlap.B); + } + + // Two acceptance-mask filters accept scattered ID sets, so the reported range is the inclusive + // hull: both bounds are shared, and every shared ID lies between them, but the IDs in between + // need not be. + [Fact] + public void An_Overlap_Between_Mask_Filters_Reports_The_Hull_Of_The_Shared_Ids() + { + using var bus = Open(NewSession(), 0); + using var service = new CanBusService(bus); + + // Shared IDs are exactly those with 0x100 set and 0x200 clear: 0x100..0x1FF and + // 0x500..0x5FF (bit 0x400 is unconstrained by either filter). + using var a = service.Subscribe(CanIdFilter.Mask(accCode: 0x100, accMask: 0x100)); + using var b = service.Subscribe(CanIdFilter.Mask(accCode: 0x000, accMask: 0x200)); + + var overlap = service.FindOverlappingFilterSubscriptions().Should().ContainSingle().Subject; + + overlap.LowestSharedId.Should().Be(0x100); + overlap.HighestSharedId.Should().Be(0x5FF); + } + + // Overlaps and the shared-ID range are decided by a bit walk that never looks at an actual ID, + // and both now come out of one search. This checks that search against the definition: sweep + // the entire standard 11-bit ID space, ask Matches directly, and compare. 13 filters, every + // ordered pair, 2048 IDs each. + // + // Driven through the service rather than the filters, because the range is reported on + // FilterOverlap; Reconfigure re-points the same two subscriptions instead of opening 169 buses. + [Fact] + public void Overlap_And_Reported_Range_Agree_With_A_Brute_Force_Sweep_Of_The_Id_Space() + { + var filters = new[] + { + CanIdFilter.Range(0x000, 0x7FF), + CanIdFilter.Range(0x100, 0x1FF), + CanIdFilter.Range(0x180, 0x2FF), + CanIdFilter.Range(0x300, 0x3FF), + CanIdFilter.Range(0x000, 0x000), + CanIdFilter.Range(0x7FF, 0x7FF), + CanIdFilter.Mask(accCode: 0x000, accMask: 0x000), // constrains nothing: matches every ID + CanIdFilter.Mask(accCode: 0x100, accMask: 0x100), + CanIdFilter.Mask(accCode: 0x000, accMask: 0x200), + CanIdFilter.Mask(accCode: 0x123, accMask: 0x7FF), // exactly one ID + CanIdFilter.Mask(accCode: 0x100, accMask: 0x700), + CanIdFilter.Mask(accCode: 0x555, accMask: 0x555), + CanIdFilter.Mask(accCode: 0x040, accMask: 0x0C0), + }; + + using var bus = Open(NewSession(), 0); + using var service = new CanBusService(bus); + using var subA = service.Subscribe(filters[0]); + using var subB = service.Subscribe(filters[0]); + + for (var i = 0; i < filters.Length; i++) + { + for (var j = 0; j < filters.Length; j++) + { + var a = filters[i]; + var b = filters[j]; + subA.Reconfigure(a); + subB.Reconfigure(b); + + uint? lowest = null; + uint? highest = null; + for (uint id = 0; id <= 0x7FF; id++) + { + var view = new CanFrameView(CanFrameType.Can20, (int)id, ReadOnlyMemory.Empty, FrameFlags.None); + if (!a.Matches(view) || !b.Matches(view)) continue; + lowest ??= id; + highest = id; + } + + var overlaps = service.FindOverlappingFilterSubscriptions(); + var because = $"filters[{i}] and filters[{j}]"; + + if (lowest is null) + { + overlaps.Should().BeEmpty($"no ID matches both of {because}"); + continue; + } + + overlaps.Should().ContainSingle(because); + overlaps[0].LowestSharedId.Should().Be(lowest.Value, $"lowest shared ID of {because}"); + overlaps[0].HighestSharedId.Should().Be(highest!.Value, $"highest shared ID of {because}"); + } + } } [Fact] diff --git a/tests/CanKit.Pro.Tests/TestCases/IsoTp/IsoTpChannelIntegrationTests.cs b/tests/CanKit.Pro.Tests/TestCases/IsoTp/IsoTpChannelIntegrationTests.cs index e7f21d1..90bac47 100644 --- a/tests/CanKit.Pro.Tests/TestCases/IsoTp/IsoTpChannelIntegrationTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/IsoTp/IsoTpChannelIntegrationTests.cs @@ -1395,7 +1395,7 @@ public ISubscription Subscribe(Func? predicate = null, int? public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) => _inner.Subscribe(filter, bufferCapacity, includeEcho); - public IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions() + public IReadOnlyList FindOverlappingFilterSubscriptions() => _inner.FindOverlappingFilterSubscriptions(); public Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, @@ -1434,7 +1434,7 @@ public ISubscription Subscribe(Func? predicate = null, int? public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) => _inner.Subscribe(filter, bufferCapacity, includeEcho); - public IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions() + public IReadOnlyList FindOverlappingFilterSubscriptions() => _inner.FindOverlappingFilterSubscriptions(); public async Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, @@ -1500,7 +1500,7 @@ public ISubscription Subscribe(Func? predicate = null, int? public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) => _inner.Subscribe(filter, bufferCapacity, includeEcho); - public IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions() + public IReadOnlyList FindOverlappingFilterSubscriptions() => _inner.FindOverlappingFilterSubscriptions(); public async Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, @@ -1566,7 +1566,7 @@ public ISubscription Subscribe(Func? predicate = null, int? public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) => _inner.Subscribe(filter, bufferCapacity, includeEcho); - public IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions() + public IReadOnlyList FindOverlappingFilterSubscriptions() => _inner.FindOverlappingFilterSubscriptions(); public async Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, diff --git a/tests/CanKit.Pro.Tests/TestCases/J1939TpTests.cs b/tests/CanKit.Pro.Tests/TestCases/J1939TpTests.cs index 2d07e0f..f8f49e3 100644 --- a/tests/CanKit.Pro.Tests/TestCases/J1939TpTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/J1939TpTests.cs @@ -1401,7 +1401,7 @@ public ISubscription Subscribe(Func? predicate = null, int? public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) => _inner.Subscribe(filter, bufferCapacity, includeEcho); - public IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions() + public IReadOnlyList FindOverlappingFilterSubscriptions() => _inner.FindOverlappingFilterSubscriptions(); public Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, diff --git a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs index d186602..62b042f 100644 --- a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs @@ -401,7 +401,7 @@ public ISubscription Subscribe(Func? predicate = null, int? public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) => inner.Subscribe(filter, bufferCapacity, includeEcho); - public IReadOnlyList<(ISubscription First, ISubscription Second)> FindOverlappingFilterSubscriptions() + public IReadOnlyList FindOverlappingFilterSubscriptions() => inner.FindOverlappingFilterSubscriptions(); public Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, CancellationToken cancellationToken = default) From 1bba7d782f0e10ecad971a0ec9800ee9f1822143 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:36:19 +0000 Subject: [PATCH 07/14] docs(rawcan): record why SendConfirmed keeps Transmit inside the pending-send lock The last of the collected RawCan findings (#53) asks for ICanBus.Transmit to move out of _pendingGate, so a blocking vendor driver cannot stall the adapter's RX thread in TryMatchEcho. It is not done, and the reason belongs next to the lock rather than in a closed issue: holding the lock across register + transmit is what makes the pending FIFO's order equal transmission order, which is the whole of FR-RAW-031's "no cross-matching of byte-identical concurrent sends". An echo-mode adapter also re-enters this lock from inside Transmit already, and a driver that genuinely blocks there is unusable for every other consumer of ICanBus too, since the same call sits on their dispatch path. Refs #53 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- src/CanKit.Pro.RawCan/CanBusService.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/CanKit.Pro.RawCan/CanBusService.cs b/src/CanKit.Pro.RawCan/CanBusService.cs index df24fb9..5323b44 100644 --- a/src/CanKit.Pro.RawCan/CanBusService.cs +++ b/src/CanKit.Pro.RawCan/CanBusService.cs @@ -331,6 +331,15 @@ private async Task SendWithEchoConfirmAsync(CanFrame frame, Time // enqueue step (Transmit is expected to be a fast, non-blocking enqueue, same // assumption every other caller of ICanBus.Transmit already makes), never across // the echo wait, so unrelated sends are not serialized against each other. + // + // A review finding (#53) asked for Transmit to move out of this lock, so that a + // blocking vendor driver cannot stall the adapter's RX thread in TryMatchEcho. It + // is deliberately not done: the atomicity above is the whole reason the FIFO order + // means anything, an echo-mode adapter re-enters this lock from inside Transmit + // anyway, and no driver that blocks in Transmit could be used with this service in + // any case -- the same call is on the dispatch path of every other consumer of + // ICanBus. If one ever has to be, the fix is a queue in front of the driver, not a + // pending list whose order no longer matches the wire. lock (_pendingGate) { if (_pendingDisposed) From e2a8a09a13469de6d82a5c0e8539eb46583e8ddf Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 06:50:51 +0000 Subject: [PATCH 08/14] test(rawcan): pin the callback Subscribe argument guards, and the high witness below a range bound Two tests from the Codecov patch report. Only the first of them closes a coverage item; the second is worth having anyway and the report's remaining lines are argued rather than padded. The argument guards on the callback Subscribe overload had no test at all. They are the whole contract for a null service or handler: without them the null reaches the pump task and surfaces later as a NullReferenceException on a background thread, with nothing pointing at the call that caused it. Both are now asserted by parameter name. This is the CanBusServiceExtensions item in the report (2 missing, 1 partial) and it is gone. The second pins the high witness in the shape the existing brute-force sweep never produced: a range whose upper bound is not itself a shared ID. Range [0x000, 0x040] against a mask requiring bits 4..7 clear shares only 0x000..0x00F, so HighestSharedId is 0x00F rather than the range's 0x040 -- the case where the walk must abandon the high bound instead of riding it down. The test cross-checks both bounds against an enumeration of the shared set, so it cannot agree with a wrong implementation. To be accurate about what that second test did not do: the branch Codecov flags on CanIdFilter.cs is the `bit >= 31` guard on the shift, not the preferHigh ternary a line below it, which was already covered. See the comment added there for why that guard cannot be reached. Suite 597/597, build 0 warnings / 0 errors with CI=true, format clean. Refs #53, #82. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- src/CanKit.Pro.RawCan/CanIdFilter.cs | 8 +++++ .../TestCases/CanIdFilterOverlapTests.cs | 29 +++++++++++++++++++ .../TestCases/RawCanSubscriptionTests.cs | 18 ++++++++++++ 3 files changed, 55 insertions(+) diff --git a/src/CanKit.Pro.RawCan/CanIdFilter.cs b/src/CanKit.Pro.RawCan/CanIdFilter.cs index eb7ce66..1c425aa 100644 --- a/src/CanKit.Pro.RawCan/CanIdFilter.cs +++ b/src/CanKit.Pro.RawCan/CanIdFilter.cs @@ -251,6 +251,14 @@ bool Search(int bit, bool loTight, bool hiTight, uint prefix, out uint result) if (!loTight && !hiTight) { // Neither bound constrains the remaining bits any more, so fill them in. + // + // The bit >= 31 arm cannot be reached from here and is a guard, not a case: + // Search starts with both bounds tight, and getting both loose costs at least + // one bit, so this branch always runs at bit <= 30. It stays because the + // alternative is silently wrong rather than loud -- C# masks the shift count, + // so 1u << 32 evaluates to 1 instead of overflowing, and the fill-in would + // quietly produce a one-bit "remaining" mask. Coverage reports it as a half + // branch for that reason. var remaining = bit >= 31 ? uint.MaxValue : (1u << (bit + 1)) - 1; var required = code & mask & remaining; result = prefix | (preferHigh ? required | (remaining & ~mask) : required); diff --git a/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs b/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs index 1864b58..8d0e330 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CanIdFilterOverlapTests.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using CanKit.Abstractions.API.Can; using CanKit.Abstractions.API.Can.Definitions; using CanKit.Abstractions.API.Common.Definitions; @@ -247,6 +248,34 @@ public void Overlap_And_Reported_Range_Agree_With_A_Brute_Force_Sweep_Of_The_Id_ } } + // The high witness is only interesting when the largest shared ID is *not* the range's own + // upper bound -- that is the case where the walk must abandon the high bound and fill the + // remaining bits itself. The existing brute-force sweep never produced that shape, so the + // preferHigh fill-in went unexercised in the riskiest code in this change. + // + // Here the mask requires bits 4..7 clear, so within [0x000, 0x040] only 0x000..0x00F qualify: + // the highest shared ID is 0x00F, well below the range's 0x040. + [Fact] + public void The_Highest_Shared_Id_Is_Found_When_It_Lies_Below_The_Range_Upper_Bound() + { + using var bus = Open(NewSession(), 0); + using var service = new CanBusService(bus); + + using var range = service.Subscribe(CanIdFilter.Range(0x000, 0x040)); + using var mask = service.Subscribe(CanIdFilter.Mask(accCode: 0x000, accMask: 0x0F0)); + + var overlap = service.FindOverlappingFilterSubscriptions().Should().ContainSingle().Subject; + + overlap.LowestSharedId.Should().Be(0x000u); + overlap.HighestSharedId.Should().Be(0x00Fu); + + // Independently: those bounds really are the extremes of the shared set. + var shared = Enumerable.Range(0x000, 0x041).Select(i => (uint)i) + .Where(id => (id & 0x0F0u) == 0x000u).ToArray(); + shared.Min().Should().Be(overlap.LowestSharedId); + shared.Max().Should().Be(overlap.HighestSharedId); + } + [Fact] public void FindOverlappingFilterSubscriptions_Ignores_Predicate_Based_Subscriptions() { diff --git a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs index 62b042f..e6eba6a 100644 --- a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs @@ -326,6 +326,24 @@ public async Task Callback_Subscribe_Handler_Exception_Is_Surfaced_And_Delivery_ // The fault channel above is an *event on ICanBusService*, and only the type declaring an // event can raise it -- so for any implementation other than CanBusService the extension has // no way to report a failing handler, and used to drop it. onError is that way. + // The two argument guards on the callback overload had no test. They are the whole contract + // for a null service or handler: without them the null reaches the pump task and surfaces + // later as a NullReferenceException on a background thread, with nothing pointing at the + // call that caused it. + [Fact] + public void Callback_Subscribe_Rejects_A_Null_Service_Or_Handler() + { + using var bus = Open(NewSession(), 0); + using var service = new CanBusService(bus); + + ICanBusService nullService = null!; + Action noService = () => nullService.Subscribe(_ => { }); + Action noHandler = () => service.Subscribe((Action)null!); + + noService.Should().Throw().WithParameterName("service"); + noHandler.Should().Throw().WithParameterName("onNext"); + } + [Fact] public async Task Callback_Subscribe_Reports_A_Handler_Failure_Through_OnError_For_A_Foreign_Service() { From 2a870132db545593914e2f03e423c3860e0942f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 07:08:57 +0000 Subject: [PATCH 09/14] fix(rawcan): stop a cancellation from waiting on an unrelated send's driver call Review found that the #24 fix bought its guarantee at too high a price. It unlinked the expired entry under _pendingGate before completing the Tcs, and that runs on whichever thread trips the token -- so a caller's own CancellationTokenSource.Cancel() blocked until an unrelated concurrent send returned from _bus.Transmit, which SendWithEchoConfirmAsync holds the same lock across. Cancelling one send is not something that should wait on another send's adapter. The unlink was also unnecessary. TryMatchEcho already skips and unlinks entries whose Tcs is completed, and that skip -- not the eager unlink -- is what actually fixes #24: an echo arriving before the `finally` runs walks past the expired entry to the live one behind it. Removing the eager unlink therefore promotes that loop from a redundant second guard to the mechanism, which is what its comment now says. The previous comment claimed a completed entry was unreachable there; with the eager unlink gone that is no longer true, and leaving it would have been the same stale-narrative defect this branch has already had to correct twice. Scope, because the test says so and the code comment should too: the caller's SendConfirmed task still completes only after the `finally` unlinks, and that unlink does take the lock. That coupling predates this fix and follows from holding _pendingGate across Transmit at all -- the deliberate decision recorded next to the lock and argued on #53. This change is responsible for the cancellation path, and that is what it fixes. Covered by Cancelling_One_Send_Does_Not_Block_On_An_Unrelated_Send_Inside_Transmit, which parks a second send inside Transmit via the OnTransmitting hook and asserts causally -- Cancel() returned while the other send was still in the driver -- rather than against a stopwatch, since elapsed time is not measurable on a shared runner (#92). Verified to fail against the previous revision with exactly that assertion, and to pass with this one. Suite 598/598, build 0 warnings / 0 errors with CI=true, format clean. Refs #24, #53. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- src/CanKit.Pro.RawCan/CanBusService.cs | 54 ++++++++++-------- .../TestCases/TxConfirmTests.cs | 56 +++++++++++++++++++ 2 files changed, 88 insertions(+), 22 deletions(-) diff --git a/src/CanKit.Pro.RawCan/CanBusService.cs b/src/CanKit.Pro.RawCan/CanBusService.cs index 5323b44..e7fe7c6 100644 --- a/src/CanKit.Pro.RawCan/CanBusService.cs +++ b/src/CanKit.Pro.RawCan/CanBusService.cs @@ -379,23 +379,26 @@ private async Task WaitForPendingAsync(PendingSend pending, Time // Registration fires on whichever comes first: caller cancellation or our own timeout. using var registration = timeoutCts.Token.Register(static state => { - var (service, p, ct) = ((CanBusService, PendingSend, CancellationToken))state!; - - // Unlink *before* completing, and here rather than in SendWithEchoConfirmAsync's - // `finally`. The Tcs completes its awaiter asynchronously - // (RunContinuationsAsynchronously), so that `finally` runs a scheduling turn later - // on a pool thread; until it did, this expired entry stayed the FIFO head for its - // key and swallowed the echo of the next byte-identical send, turning one timeout - // into a cascade of them. Unlinking under _pendingGate before the Tcs is completed - // closes that window entirely: from the instant this entry is resolved it is no - // longer matchable. The `finally` stays as the cleanup for every path that does not - // pass through here (rejection, an exception out of Transmit). + var (p, ct) = ((PendingSend, CancellationToken))state!; + + // Complete the Tcs *without touching _pendingGate*. An earlier revision of the #24 + // fix unlinked here first, reasoning that an entry must stop being matchable the + // instant it is resolved. It does -- but taking the lock to achieve that runs on + // whichever thread trips the token, and SendWithEchoConfirmAsync holds that lock + // across _bus.Transmit. A caller's own CancellationTokenSource.Cancel() therefore + // blocked until an unrelated send's driver call returned. Cancelling one send is + // not something that should wait on another send's adapter. // - // Monitor is reentrant, so this is safe even when the cancellation is triggered - // from a thread that already holds _pendingGate (a caller cancelling from inside a - // subscription predicate while a synchronous echo is being dispatched, say). - service.RemovePending(p); - + // Unlinking here is also unnecessary. What makes the still-linked expired entry + // harmless is TryMatchEcho skipping (and unlinking) entries whose Tcs is already + // completed -- see the loop there, which is the mechanism that fixes #24. An echo + // arriving in the window before the `finally` unlinks walks past this entry to the + // live one behind it instead of being swallowed. + // + // What this does *not* change: the caller's SendConfirmed task still completes only + // after that `finally`, and the unlink there does take the lock. That coupling is + // older than this fix and follows from holding _pendingGate across Transmit at all + // -- see the note on that lock, and #53. if (ct.IsCancellationRequested) { p.Tcs.TrySetCanceled(ct); @@ -410,7 +413,7 @@ private async Task WaitForPendingAsync(PendingSend pending, Time FailureReason = TxConfirmFailureReason.Timeout, }); } - }, (this, pending, cancellationToken)); + }, (pending, cancellationToken)); timeoutCts.CancelAfter(timeout); return await pending.Tcs.Task.ConfigureAwait(false); @@ -464,11 +467,18 @@ private void TryMatchEcho(in CanFrameView echoView) // drop this echo (TrySetResult no-ops on a completed Tcs) and leave the send it // actually belonged to waiting for an echo that has already come and gone. // - // Every resolution path unlinks under this same lock before it completes the - // Tcs, so a completed entry should not be reachable here at all. This stays as - // the second guard for that invariant -- and, because it unlinks what it skips, - // it also stops such an entry from blocking the FIFO for every later echo - // instead of only for this one. + // This skip is the mechanism that fixes #24, not a redundant guard. The + // timeout and cancellation path completes its Tcs without taking this lock -- + // deliberately, so a deadline cannot be held up by an unrelated send sitting in + // a slow _bus.Transmit -- and the entry it resolved stays linked until + // SendWithEchoConfirmAsync's `finally` runs a scheduling turn later. Inside + // that window the expired entry is still the FIFO head for its key, and + // without this skip it would swallow the next byte-identical send's echo, + // turning one timeout into a cascade of them. + // + // Unlinking what it skips matters as much as skipping it: otherwise the same + // dead entry would block the FIFO for every later echo rather than only this + // one. for (var node = list.First; node is not null;) { var next = node.Next; diff --git a/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs b/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs index 2f00a30..634379a 100644 --- a/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/TxConfirmTests.cs @@ -189,6 +189,62 @@ public async Task Echo_Bus_Does_Not_Let_A_Resolved_Send_Consume_A_Later_Identica await Assert.ThrowsAnyAsync(() => first); } + // Regression for the fix to #24 itself. An earlier revision unlinked the expired entry under + // _pendingGate *before* completing its Tcs, so resolving a send had to wait for that lock -- + // which SendWithEchoConfirmAsync holds across _bus.Transmit. The visible cost was on the + // caller's own thread: CancellationTokenSource.Cancel() for one send blocked until an + // unrelated send's driver call returned. Completing the Tcs without the lock removes that; + // TryMatchEcho's skip of already-completed entries is what keeps #24 fixed meanwhile. + // + // Scope, stated so this test is not read as promising more than it checks: the caller's + // SendConfirmed task still completes only after the `finally` unlinks, and that unlink does + // take the lock. That coupling predates this change and follows from holding _pendingGate + // across Transmit at all -- the deliberate decision recorded next to that lock (#53). What is + // asserted here is the part this change is responsible for. + // + // Causal, not timed: the claim is "Cancel() returned while the other send was still inside + // Transmit", not "within N ms" (unmeasurable on a shared runner, see #92). The 2 s allowance + // is slack for a slow runner -- with the coupling in place Cancel() cannot return until the + // release below, which happens afterwards. + [Fact] + public async Task Cancelling_One_Send_Does_Not_Block_On_An_Unrelated_Send_Inside_Transmit() + { + using var sender = OpenEcho(); + using var service = new CanBusService(sender); + + using var cancelFirst = new CancellationTokenSource(); + sender.EchoAcceptedFrames = false; + var first = service.SendConfirmed( + CanFrame.Classic(0x610, new byte[] { 1 }), TimeSpan.FromSeconds(30), cancelFirst.Token); + + // A second, unrelated send parks inside Transmit -- and so inside _pendingGate. + using var stuckInTransmit = new ManualResetEventSlim(false); + using var reachedTransmit = new ManualResetEventSlim(false); + sender.OnTransmitting = f => + { + if (f.ID != 0x611) return; + reachedTransmit.Set(); + stuckInTransmit.Wait(TimeSpan.FromSeconds(10)); + }; + var blocked = Task.Run(() => service.SendConfirmed( + CanFrame.Classic(0x611, new byte[] { 2 }), ShortTimeout)); + + reachedTransmit.Wait(TimeSpan.FromSeconds(5)).Should().BeTrue( + "the second send must actually be parked in Transmit for this test to mean anything"); + + var cancelling = Task.Run(() => cancelFirst.Cancel()); + var finished = await Task.WhenAny(cancelling, Task.Delay(TimeSpan.FromSeconds(2))); + var cancelReturnedWhileBlocked = ReferenceEquals(finished, cancelling); + + stuckInTransmit.Set(); + await cancelling; + await blocked; + + cancelReturnedWhileBlocked.Should().BeTrue( + "cancelling one send must not wait on an unrelated send's driver call to return"); + await Assert.ThrowsAnyAsync(() => first); + } + // FR-RAW-031: the echo is matched on what identifies the frame, not on its ID alone. A // standard 0x100 and an extended 0x100 carrying the same payload are two different frames on // the wire; keyed on the ID alone they share one FIFO, so the extended frame's echo confirms From a37a20a89a761cf6398aee3711cbae78a71be3fc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 07:31:28 +0000 Subject: [PATCH 10/14] test(rawcan): cover the pump's own failure path, which Codecov was right about I dismissed the Codecov row for CanBusServiceExtensions.cs as a stale report. It was not stale; my measurement was wrong. The script I checked it with de-duplicated coverage entries by filename, and Cobertura emits one entry per *type* -- so it read the outer static class, saw 100%, and never looked at the nested CallbackSubscription where the uncovered lines are. What was actually uncovered is new behaviour this change introduces and the pull request explicitly claims: the pump wraps the whole `await foreach`, so a failure of the frame stream itself is reported rather than left on a task nobody observes -- Dispose's join is bounded and may already have given up on it. Every existing onError test fails inside the handler, which the inner catch takes, so the outer one was never entered by anything. Two tests, against a service whose subscription throws on enumeration: the failure reaches onError, and with no onError and a service that is not CanBusService -- where there is nowhere to report it, since the interface's fault event is not ours to raise -- it is dropped quietly instead of surfacing on a pool thread, with Dispose still idempotent afterwards. CanBusServiceExtensions.cs is now fully covered, lines and branches, across all nested types, verified with the corrected script rather than the one that produced the wrong answer. The other two files Codecov names are unaffected by the bug -- neither has nested types -- so the arguments for CanIdFilter's unreachable shift guard and PendingKey's collision-only equality short-circuits stand as written. Suite 600/600, build 0 warnings / 0 errors with CI=true, format clean. Refs #53. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- .../TestCases/RawCanSubscriptionTests.cs | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs index e6eba6a..90b1176 100644 --- a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs @@ -401,6 +401,89 @@ public async Task Callback_Subscribe_OnError_Takes_Precedence_Over_The_Service_F // An ICanBusService that is not a CanBusService: everything forwarded, so the only thing that // differs from using the concrete service is that its fault event is not ours to raise. + // The pump wraps the whole `await foreach`, not just the onNext call, so a failure of the + // enumeration itself is reported instead of being left on a task nobody will ever look at -- + // Dispose's join is bounded and may already have given up on it. That is new behaviour in this + // change and it had no test: the existing onError cases all fail inside the handler, which the + // *inner* catch takes, so the outer one was never entered. + [Fact] + public async Task Callback_Subscribe_Reports_A_Failure_Of_The_Frame_Stream_Itself() + { + using var service = new FramesThrowOnEnumerationService(); + + var observed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using var subscription = service.Subscribe(_ => { }, onError: ex => observed.TrySetResult(ex)); + + var ex = await observed.Task.WaitAsync(ShortTimeout); + ex.Should().BeOfType() + .Which.Message.Should().Be("the frame stream itself failed"); + } + + // The same failure with no onError and a service that is not CanBusService: there is nowhere + // to report it -- the interface's fault event is not ours to raise -- so it must be dropped + // quietly rather than thrown on a pool thread. Disposing afterwards must still work. + [Fact] + public void Callback_Subscribe_Drops_A_Stream_Failure_It_Has_Nowhere_To_Report() + { + using var service = new FramesThrowOnEnumerationService(); + + var subscribeAndDispose = () => + { + var subscription = service.Subscribe(_ => { }); + Thread.Sleep(50); // let the pump reach the failure + subscription.Dispose(); + subscription.Dispose(); // idempotent -- the second call must be a no-op + }; + + subscribeAndDispose.Should().NotThrow(); + } + + private sealed class FramesThrowOnEnumerationService : ICanBusService + { + public ICanBus Bus => throw new NotSupportedException(); + + public int SubscriptionCount => 0; + +#pragma warning disable CS0067 + public event EventHandler? BackgroundExceptionOccurred; +#pragma warning restore CS0067 + + public ISubscription Subscribe(Func? predicate = null, int? bufferCapacity = null, bool includeEcho = false) + => new ThrowingSubscription(); + + public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) + => new ThrowingSubscription(); + + public IReadOnlyList FindOverlappingFilterSubscriptions() => Array.Empty(); + + public Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, CancellationToken cancellationToken = default) + => throw new NotSupportedException(); + + public void Dispose() { } + + private sealed class ThrowingSubscription : ISubscription + { + public IAsyncEnumerable Frames => Throwing(); + + public bool TryRead(out CanFrameEvent frameEvent) { frameEvent = default; return false; } + + public void Reconfigure(CanIdFilter filter) { } + + public void Reconfigure(Func? predicate) { } + + public void Dispose() { } + + private static async IAsyncEnumerable Throwing() + { + await Task.Yield(); + throw new InvalidOperationException("the frame stream itself failed"); +#pragma warning disable CS0162 + yield break; // unreachable, but required to make this an iterator +#pragma warning restore CS0162 + } + } + } + private sealed class ForeignCanBusService(ICanBusService inner) : ICanBusService { public ICanBus Bus => inner.Bus; From d213e001d57d51085c694d846bff9a98103bb960 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 07:38:19 +0000 Subject: [PATCH 11/14] test(rawcan): make the stream-failure test deterministic and its disposal exception-safe Two CodeQL findings on the test added in a37a20a, both correct. The subscription was disposed without a `using`, so an exception from anything above the call -- the sleep, or the first Dispose -- would have skipped it. It now has both: `using` for the guarantee, and one explicit call, which is what exercises the idempotence the test is there to check. The Thread.Sleep(50) that CodeQL tripped over deserved to go on its own merits. It was a guess that the pump had reached the throw by then: too short on a loaded runner and the drop path goes unexercised while the test still passes, which is the failure mode this branch has been documenting in #92 all week. The fake now sets an event immediately before its enumerator throws and the test waits on that, so the failure has provably happened before disposal rather than probably. Suite 600/600, build 0 warnings / 0 errors with CI=true, format clean. Refs #53. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- .../TestCases/RawCanSubscriptionTests.cs | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs index 90b1176..e3a61c9 100644 --- a/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/RawCanSubscriptionTests.cs @@ -429,10 +429,16 @@ public void Callback_Subscribe_Drops_A_Stream_Failure_It_Has_Nowhere_To_Report() var subscribeAndDispose = () => { - var subscription = service.Subscribe(_ => { }); - Thread.Sleep(50); // let the pump reach the failure - subscription.Dispose(); - subscription.Dispose(); // idempotent -- the second call must be a no-op + // `using` as well as the explicit call: the explicit one is the idempotence check, + // the `using` makes sure disposal still happens if anything above it throws. + using var subscription = service.Subscribe(_ => { }); + + // Wait for the stream to have actually failed rather than sleeping and hoping -- + // without this the pump might not have reached the throw yet and the drop path + // would go unexercised while the test still passed. + service.StreamFailed.Wait(TimeSpan.FromSeconds(5)).Should().BeTrue(); + + subscription.Dispose(); // the `using` disposes again: the second call must be a no-op }; subscribeAndDispose.Should().NotThrow(); @@ -440,6 +446,10 @@ public void Callback_Subscribe_Drops_A_Stream_Failure_It_Has_Nowhere_To_Report() private sealed class FramesThrowOnEnumerationService : ICanBusService { + // Set immediately before the enumerator throws, so a test can wait for the failure to + // have happened instead of guessing at a delay. + public ManualResetEventSlim StreamFailed { get; } = new(false); + public ICanBus Bus => throw new NotSupportedException(); public int SubscriptionCount => 0; @@ -449,21 +459,21 @@ private sealed class FramesThrowOnEnumerationService : ICanBusService #pragma warning restore CS0067 public ISubscription Subscribe(Func? predicate = null, int? bufferCapacity = null, bool includeEcho = false) - => new ThrowingSubscription(); + => new ThrowingSubscription(StreamFailed); public ISubscription Subscribe(CanIdFilter filter, int? bufferCapacity = null, bool includeEcho = false) - => new ThrowingSubscription(); + => new ThrowingSubscription(StreamFailed); public IReadOnlyList FindOverlappingFilterSubscriptions() => Array.Empty(); public Task SendConfirmed(CanFrame frame, TimeSpan? timeout = null, CancellationToken cancellationToken = default) => throw new NotSupportedException(); - public void Dispose() { } + public void Dispose() => StreamFailed.Dispose(); - private sealed class ThrowingSubscription : ISubscription + private sealed class ThrowingSubscription(ManualResetEventSlim streamFailed) : ISubscription { - public IAsyncEnumerable Frames => Throwing(); + public IAsyncEnumerable Frames => Throwing(streamFailed); public bool TryRead(out CanFrameEvent frameEvent) { frameEvent = default; return false; } @@ -473,9 +483,10 @@ public void Reconfigure(Func? predicate) { } public void Dispose() { } - private static async IAsyncEnumerable Throwing() + private static async IAsyncEnumerable Throwing(ManualResetEventSlim streamFailed) { await Task.Yield(); + streamFailed.Set(); throw new InvalidOperationException("the frame stream itself failed"); #pragma warning disable CS0162 yield break; // unreachable, but required to make this an iterator From f75b40fd58f2efe728983bf6da8f5dc703e9d39c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 07:45:29 +0000 Subject: [PATCH 12/14] fix(rawcan): claim a pending send by completing it, not by asking whether it is complete Review found a check-then-act window in the echo matcher. TryMatchEcho tested candidate.Tcs.Task.IsCompleted under _pendingGate, then completed the winner after releasing it. Since the previous commit the timeout and cancellation path completes without that lock -- on purpose, so a deadline is not held up by an unrelated send inside _bus.Transmit -- so it can land between the two. The echo is then consumed by an entry that lost the race, and a live byte-identical send behind it in the FIFO waits for an echo that has already arrived, and eventually times out. That is #24 again in different clothes: something already resolved still winning against what came after it. Fixing the first instance by moving the completion out from under the lock is what opened this one, which is worth recording rather than tidying away. TrySetResult is the only test that cannot be raced, because it *is* the transition. The walk now claims each candidate by completing it and treats a false return as "someone else got there first", continuing to the next entry instead of consuming the echo on its behalf. Completing under the lock is safe here because the Tcs is created with RunContinuationsAsynchronously: the awaiting continuation is queued, not run inline, so no caller code executes while _pendingGate is held. The old comment claiming the opposite described a hazard that flag already removes. Closed by construction rather than by a test: the window was between two operations that are now one, so there is no interleaving left to reproduce. A test could only hit it probabilistically, and this branch has spent the week arguing against exactly that kind of test (#92). The behaviour it protects -- an echo going to the live send when the FIFO head is already resolved -- stays covered by Echo_Bus_Does_Not_Let_A_Resolved_Send_Consume_A_Later_Identical_Echo. Suite 600/600, build 0 warnings / 0 errors with CI=true, format clean. Refs #24. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- src/CanKit.Pro.RawCan/CanBusService.cs | 64 +++++++++++++++----------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/src/CanKit.Pro.RawCan/CanBusService.cs b/src/CanKit.Pro.RawCan/CanBusService.cs index e7fe7c6..7c98eb7 100644 --- a/src/CanKit.Pro.RawCan/CanBusService.cs +++ b/src/CanKit.Pro.RawCan/CanBusService.cs @@ -455,26 +455,32 @@ private void TryMatchEcho(in CanFrameView echoView) // Aliases the echo frame's payload rather than copying it: this runs for every echo // frame the adapter reports, and the key is dropped again before the lock is released. var key = PendingKey.ForEchoLookup(echoView.ID, echoView.Data, echoView.Flags, echoView.FrameKind); - PendingSend? matched = null; lock (_pendingGate) { if (_pending.TryGetValue(key, out var list)) { - // FIFO: the oldest pending send for this key matches first -- but only if it is - // still waiting. An entry whose Tcs is already completed has been resolved by - // some other path and can no longer consume anything; matching it would silently - // drop this echo (TrySetResult no-ops on a completed Tcs) and leave the send it - // actually belonged to waiting for an echo that has already come and gone. + var confirmation = new TxConfirmation + { + Confirmed = true, + IsApproximated = false, + Timestamp = DateTime.UtcNow, + FailureReason = TxConfirmFailureReason.None, + }; + + // FIFO: the oldest pending send for this key gets the echo -- but only if it + // is still waiting. An entry already resolved by some other path can no longer + // consume anything, and handing it the echo would drop it silently while the + // send it actually belonged to waits for one that has already come and gone. // - // This skip is the mechanism that fixes #24, not a redundant guard. The - // timeout and cancellation path completes its Tcs without taking this lock -- - // deliberately, so a deadline cannot be held up by an unrelated send sitting in - // a slow _bus.Transmit -- and the entry it resolved stays linked until - // SendWithEchoConfirmAsync's `finally` runs a scheduling turn later. Inside - // that window the expired entry is still the FIFO head for its key, and - // without this skip it would swallow the next byte-identical send's echo, - // turning one timeout into a cascade of them. + // Walking past such entries is the mechanism that fixes #24, not a redundant + // guard. The timeout and cancellation path completes its Tcs without taking + // this lock -- deliberately, so a deadline cannot be held up by an unrelated + // send sitting in a slow _bus.Transmit -- and the entry it resolved stays + // linked until SendWithEchoConfirmAsync's `finally` runs a scheduling turn + // later. Inside that window the expired entry is still the FIFO head for its + // key, and without this walk it would swallow the next byte-identical send's + // echo, turning one timeout into a cascade of them. // // Unlinking what it skips matters as much as skipping it: otherwise the same // dead entry would block the FIFO for every later echo rather than only this @@ -488,11 +494,22 @@ private void TryMatchEcho(in CanFrameView echoView) candidate.Node = null; Interlocked.Decrement(ref _pendingCount); - if (!candidate.Tcs.Task.IsCompleted) - { - matched = candidate; - break; - } + // Claim by completing, not by asking first. An `IsCompleted` test followed + // by a TrySetResult is check-then-act: the timeout and cancellation path + // completes without this lock, so it can land between the two, and then + // the echo is consumed by an entry that lost the race while a live send + // behind it in the FIFO waits for an echo that has already arrived. That + // is the same defect as #24 wearing different clothes. + // + // TrySetResult is the only test that cannot be raced, because it *is* the + // transition. A false return means some other path got there first, so + // this candidate never owned the echo and the walk continues to the next. + // + // Safe under the lock precisely because the Tcs is created with + // RunContinuationsAsynchronously: completing it queues the awaiting + // continuation rather than running it inline, so no caller code executes + // while _pendingGate is held. + if (candidate.Tcs.TrySetResult(confirmation)) break; node = next; } @@ -501,15 +518,6 @@ private void TryMatchEcho(in CanFrameView echoView) _pending.Remove(key); } } - - // TrySetResult outside the lock: never invoke TCS continuations while holding a lock. - matched?.Tcs.TrySetResult(new TxConfirmation - { - Confirmed = true, - IsApproximated = false, - Timestamp = DateTime.UtcNow, - FailureReason = TxConfirmFailureReason.None, - }); } private void OnFaultOccurred(object? sender, Exception ex) From 3a0ceddfbbb783b7cc1d2e012fb70fe14c83d6e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:35:37 +0000 Subject: [PATCH 13/14] test(rawcan): cover the two branches Codecov flagged instead of arguing them Both remaining patch-coverage items are closed. I had argued each of them as unreachable-by-construction rather than covering it, twice, and that was the wrong call: one of the two was reachable the moment the code stopped needing a special case, and the other was only unreachable through the path I happened to be testing it from. CanIdFilter: the `bit >= 31` arm is gone rather than covered. It existed because C# masks the shift count, so `1u << 32` evaluates to 1 instead of overflowing and the fill-in would quietly produce a one-bit mask. Shifting in 64 bits and narrowing -- `(uint)((1UL << (bit + 1)) - 1)` -- has room for the carry, so one expression is correct for every bit in 0..31 and there is no arm left to get wrong or to leave half-covered. Verified equivalent to the old ternary for all 32 values of bit. This is the better code independently of coverage: the reader no longer has to know about shift masking to see that it is right. PendingSend: PendingKey.Equals is now tested directly, which needs InternalsVisibleTo on CanKit.Pro.RawCan -- already the convention here, as CanKit.Pro.CANopen and CanKit.Pro.J1939Tp both do the same. My earlier objection that this widens the assembly surface did not survive checking that. The gap was real and the reason it stayed open is worth recording: PendingKey is only reached through a Dictionary, and a dictionary compares hashes first, so two keys differing in ID, flags or frame kind land in different buckets and Equals is never called. The bus-level echo tests cover the behaviour and cannot touch the comparison implementing it -- a bug in one of those short-circuit arms would surface only as a hash collision resolving to the wrong send: rare, non-deterministic, and diagnosed on the wire hours later. Exactly the failure mode this key was introduced to prevent. Fourteen cases: each component varied on its own (ID, Ext, Rtr, Error, frame kind, payload content, payload length, empty payload) with symmetry asserted; Brs and Esi deliberately not changing identity, which is the counterpart claim the type's remarks make; ForEchoLookup agreeing with ForPendingSend for the same frame; ForPendingSend copying the payload so a recycled buffer cannot repoint a stored key; the Equals(object?) override agreeing with the typed one; and the distinctness the FIFO relies on, over a real dictionary. Measured, not assumed -- 0 missing and 0 partial across every line this pull request adds or changes, in all seven touched src files. The measurement itself aggregates per filename across all types including nested ones, which is what the first two attempts got wrong. Build 0 warnings / 0 errors with CI=true, suite 614/614, format clean, packages verified, release config loadable. Refs #24, FR-RAW-031. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- src/CanKit.Pro.RawCan/CanIdFilter.cs | 18 ++- .../CanKit.Pro.RawCan.csproj | 9 ++ .../TestCases/PendingKeyTests.cs | 151 ++++++++++++++++++ 3 files changed, 170 insertions(+), 8 deletions(-) create mode 100644 tests/CanKit.Pro.Tests/TestCases/PendingKeyTests.cs diff --git a/src/CanKit.Pro.RawCan/CanIdFilter.cs b/src/CanKit.Pro.RawCan/CanIdFilter.cs index 1c425aa..af0502a 100644 --- a/src/CanKit.Pro.RawCan/CanIdFilter.cs +++ b/src/CanKit.Pro.RawCan/CanIdFilter.cs @@ -252,14 +252,16 @@ bool Search(int bit, bool loTight, bool hiTight, uint prefix, out uint result) { // Neither bound constrains the remaining bits any more, so fill them in. // - // The bit >= 31 arm cannot be reached from here and is a guard, not a case: - // Search starts with both bounds tight, and getting both loose costs at least - // one bit, so this branch always runs at bit <= 30. It stays because the - // alternative is silently wrong rather than loud -- C# masks the shift count, - // so 1u << 32 evaluates to 1 instead of overflowing, and the fill-in would - // quietly produce a one-bit "remaining" mask. Coverage reports it as a half - // branch for that reason. - var remaining = bit >= 31 ? uint.MaxValue : (1u << (bit + 1)) - 1; + // Shift in 64 bits and narrow, rather than special-casing bit == 31. The + // hazard being avoided is that C# masks the shift count: `1u << 32` is not + // an overflow, it evaluates to 1, so a 32-bit shift here would silently + // produce a one-bit "remaining" mask instead of the all-ones one. `1UL << 32` + // has room for the carry, so the same expression is correct for every + // bit in 0..31 and there is no arm to get wrong -- or, as it turned out, to + // leave permanently half-covered because it cannot be reached from the only + // call site (Search starts with both bounds tight, and loosening both costs + // at least one bit, so this runs at bit <= 30). + var remaining = (uint)((1UL << (bit + 1)) - 1); var required = code & mask & remaining; result = prefix | (preferHigh ? required | (remaining & ~mask) : required); return true; diff --git a/src/CanKit.Pro.RawCan/CanKit.Pro.RawCan.csproj b/src/CanKit.Pro.RawCan/CanKit.Pro.RawCan.csproj index 0e59e45..243ad22 100644 --- a/src/CanKit.Pro.RawCan/CanKit.Pro.RawCan.csproj +++ b/src/CanKit.Pro.RawCan/CanKit.Pro.RawCan.csproj @@ -15,4 +15,13 @@ + + + + <_Parameter1>CanKit.Pro.Tests + + + diff --git a/tests/CanKit.Pro.Tests/TestCases/PendingKeyTests.cs b/tests/CanKit.Pro.Tests/TestCases/PendingKeyTests.cs new file mode 100644 index 0000000..704cf18 --- /dev/null +++ b/tests/CanKit.Pro.Tests/TestCases/PendingKeyTests.cs @@ -0,0 +1,151 @@ +using System; +using System.Collections.Generic; +using CanKit.Abstractions.API.Can.Definitions; +using CanKit.Abstractions.API.Common.Definitions; +using CanKit.Pro.RawCan; +using FluentAssertions; +using Xunit; + +namespace CanKit.Pro.Tests.TestCases; + +/// +/// Direct tests for , the equality contract that decides which arriving +/// echo confirms which pending send (FR-RAW-031). +/// +/// These have to call Equals directly, which is why CanKit.Pro.RawCan makes its +/// internals visible here. Driving the same type through exercises +/// only the dictionary path, and a dictionary compares hashes first: two keys that differ in ID, +/// flags or frame kind land in different buckets and Equals is never called at all. So the +/// bus-level echo tests -- which do cover the *behaviour* -- cannot reach the short-circuit arms +/// of the comparison that implements it, and a bug in one of them would only ever surface as a +/// hash collision resolving to the wrong send: rare, non-deterministic, and diagnosed on the wire. +/// +public class PendingKeyTests +{ + private static readonly byte[] Payload = { 0x11, 0x22, 0x33, 0x44 }; + + private static PendingKey Key( + int id = 0x123, + byte[]? payload = null, + FrameFlags flags = FrameFlags.None, + CanFrameType kind = CanFrameType.Can20) + => PendingKey.ForPendingSend(id, payload ?? Payload, flags, kind); + + [Fact] + public void Two_Sends_Of_The_Same_Frame_Have_Equal_Keys() + { + // Separate payload arrays on purpose: the key must compare content, not reference, or a + // caller's second send of identical bytes would never match its own echo. + var a = Key(payload: new byte[] { 0x11, 0x22, 0x33, 0x44 }); + var b = Key(payload: new byte[] { 0x11, 0x22, 0x33, 0x44 }); + + a.Equals(b).Should().BeTrue(); + a.GetHashCode().Should().Be(b.GetHashCode()); + } + + // Each case differs from the reference key in exactly one component, which is what pins the + // short-circuit arms: a comparison that forgot to test that component would return true here. + // The theory carries a discriminator rather than the key itself -- PendingKey is internal and + // cannot appear in a public signature, and MemberData needs one. + [Theory] + [InlineData("id")] + [InlineData("extended vs standard")] + [InlineData("remote vs data")] + [InlineData("error vs data")] + [InlineData("frame kind")] + [InlineData("payload content")] + [InlineData("payload length")] + [InlineData("empty payload")] + public void Keys_Differing_In_One_Component_Are_Not_Equal(string component) + { + var other = component switch + { + "id" => Key(id: 0x124), + "extended vs standard" => Key(flags: FrameFlags.Ext), + "remote vs data" => Key(flags: FrameFlags.Rtr), + "error vs data" => Key(flags: FrameFlags.Error), + "frame kind" => Key(kind: CanFrameType.CanFd), + "payload content" => Key(payload: new byte[] { 0x11, 0x22, 0x33, 0x45 }), + "payload length" => Key(payload: new byte[] { 0x11, 0x22, 0x33 }), + "empty payload" => Key(payload: Array.Empty()), + _ => throw new ArgumentOutOfRangeException(nameof(component), component, null), + }; + + Key().Equals(other).Should().BeFalse(); + other.Equals(Key()).Should().BeFalse(); // symmetric + } + + [Fact] + public void Brs_And_Esi_Do_Not_Change_Identity() + { + // The counterpart to the cases above: these two flags are link-layer transmission + // attributes, and an adapter may echo a frame whose BRS/ESI differ from what was asked + // for. Including them would turn a matched echo into a spurious timeout. + Key().Equals(Key(flags: FrameFlags.Brs)).Should().BeTrue(); + Key().Equals(Key(flags: FrameFlags.Esi)).Should().BeTrue(); + Key().Equals(Key(flags: FrameFlags.Brs | FrameFlags.Esi)).Should().BeTrue(); + } + + [Fact] + public void The_Lookup_Key_Matches_The_Stored_Key_For_The_Same_Frame() + { + // ForEchoLookup aliases the caller's buffer instead of copying it, so that an arriving + // echo costs no allocation on the dispatch hot path. The two factories must still agree, + // or no echo would ever confirm anything. + var stored = PendingKey.ForPendingSend(0x321, Payload, FrameFlags.Ext, CanFrameType.CanFd); + var lookup = PendingKey.ForEchoLookup(0x321, Payload, FrameFlags.Ext, CanFrameType.CanFd); + + stored.Equals(lookup).Should().BeTrue(); + stored.GetHashCode().Should().Be(lookup.GetHashCode()); + } + + [Fact] + public void ForPendingSend_Copies_The_Payload_So_A_Reused_Buffer_Cannot_Change_The_Key() + { + // The stored key outlives the caller's frame; on the RX side the adapter's lease behind + // it may be recycled. If the key aliased that buffer, mutating it after the send would + // silently repoint the key at a frame nobody sent. + var buffer = new byte[] { 0x01, 0x02 }; + var stored = PendingKey.ForPendingSend(0x100, buffer, FrameFlags.None, CanFrameType.Can20); + + buffer[1] = 0xFF; + + stored.Equals(PendingKey.ForEchoLookup(0x100, new byte[] { 0x01, 0x02 }, + FrameFlags.None, CanFrameType.Can20)).Should().BeTrue(); + stored.Equals(PendingKey.ForEchoLookup(0x100, buffer, + FrameFlags.None, CanFrameType.Can20)).Should().BeFalse(); + } + + [Fact] + public void Equals_Object_Follows_The_Typed_Comparison() + { + // A Dictionary always takes the generic IEquatable path, so this + // override is only reached by a non-generic caller. It still has to agree. + object same = Key(); + object different = Key(id: 0x999); + + Key().Equals(same).Should().BeTrue(); + Key().Equals(different).Should().BeFalse(); + Key().Equals("not a key").Should().BeFalse(); + Key().Equals(null).Should().BeFalse(); + } + + [Fact] + public void Distinct_Keys_Stay_Distinct_As_Dictionary_Entries() + { + // The property the FIFO actually relies on, asserted end to end over the components + // above rather than through the bus. + var map = new Dictionary + { + [Key()] = "reference", + [Key(id: 0x124)] = "other id", + [Key(flags: FrameFlags.Ext)] = "extended", + [Key(kind: CanFrameType.CanFd)] = "fd", + [Key(payload: new byte[] { 0x11, 0x22, 0x33, 0x45 })] = "other payload", + }; + + map.Should().HaveCount(5); + map[PendingKey.ForEchoLookup(0x123, Payload, FrameFlags.None, CanFrameType.Can20)] + .Should().Be("reference"); + } +} From 83b7c59668affa17730dc2b02e545d272376b374 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 09:01:05 +0000 Subject: [PATCH 14/14] test(rawcan): state the foreign-object operands as object so CodeQL reads them right CodeQL raised one error and one warning on the new PendingKey tests, both on the same two assertions: `key.Equals("not a key")` compares incomparable types, and `key.Equals(null)` can never be true. Both rules are correct about the code and wrong about the intent. Those two assertions exist to pin the `obj is PendingKey` guard in the Equals(object?) override -- a foreign reference and null must come back false rather than throwing or matching -- so "this comparison is always false" is the property under test, not a mistake. The rules are aimed at accidental comparisons in production code, where they are worth having. Holding the operands in object? locals says what the assertions mean, "some reference that is not a PendingKey", instead of leaving a string literal sitting in a signature that takes object. Runtime behaviour is identical, so the guard's false arm stays covered: 14/14 in PendingKeyTests, and patch coverage still 0 missing / 0 partial across every line this pull request touches. Not changed: the three "generic catch clause" notes on CanBusServiceExtensions. Catching Exception there is the contract -- an arbitrary caller-supplied onNext or onError must not be able to take the delivery pump down with it -- and each of the three already says so in place. Narrowing them would mean choosing which subscriber bugs are allowed to kill an unrelated subscription. Build 0 warnings / 0 errors with CI=true, suite 614/614, format clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj --- .../TestCases/PendingKeyTests.cs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/CanKit.Pro.Tests/TestCases/PendingKeyTests.cs b/tests/CanKit.Pro.Tests/TestCases/PendingKeyTests.cs index 704cf18..46b0cc8 100644 --- a/tests/CanKit.Pro.Tests/TestCases/PendingKeyTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/PendingKeyTests.cs @@ -120,14 +120,25 @@ public void ForPendingSend_Copies_The_Payload_So_A_Reused_Buffer_Cannot_Change_T public void Equals_Object_Follows_The_Typed_Comparison() { // A Dictionary always takes the generic IEquatable path, so this - // override is only reached by a non-generic caller. It still has to agree. + // override is only reached by a non-generic caller. It still has to agree, including + // on the `obj is PendingKey` guard: a foreign object and null must both come back + // false rather than throwing or matching. + // + // The last two operands are held in `object?` locals rather than written inline as a + // string literal and `null`. CodeQL is right that `key.Equals("...")` compares + // incomparable types and that `key.Equals(null)` can never be true -- that is the + // point of the assertions, and stating the operands as `object?` says "some reference + // that is not a PendingKey", which is the contract being pinned, instead of tripping + // a rule aimed at accidental comparisons in production code. object same = Key(); object different = Key(id: 0x999); + object? foreign = "not a key"; + object? nothing = null; Key().Equals(same).Should().BeTrue(); Key().Equals(different).Should().BeFalse(); - Key().Equals("not a key").Should().BeFalse(); - Key().Equals(null).Should().BeFalse(); + Key().Equals(foreign).Should().BeFalse(); + Key().Equals(nothing).Should().BeFalse(); } [Fact]