From d5edc88dc7d397e92ae710e414ef19ff7fff125f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 20:29:12 +0000 Subject: [PATCH 1/2] refactor(uds): add request-lock observables for tests UdsClientImpl and UdsFunctionalClient take _requestLock through a new AcquireRequestLockAsync helper. It raises the internal RequestLockContended event when the lock is already held, and UdsClientImpl also raises RequestLockAcquired once it is taken. Tests use them in place of wall-clock sleeps timed to land while another call holds the lock (#171). Neither event has a subscriber in production. The zero-timeout probe takes the caller's token, so a call cancelled before it starts throws rather than taking a free lock, as the plain WaitAsync it replaces did. Refs #171 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s --- src/CanKit.Pro.Uds/UdsClientImpl.cs | 49 ++++++++++++++++++----- src/CanKit.Pro.Uds/UdsFunctionalClient.cs | 17 +++++++- 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/src/CanKit.Pro.Uds/UdsClientImpl.cs b/src/CanKit.Pro.Uds/UdsClientImpl.cs index 58301cc..81288c8 100644 --- a/src/CanKit.Pro.Uds/UdsClientImpl.cs +++ b/src/CanKit.Pro.Uds/UdsClientImpl.cs @@ -53,6 +53,37 @@ internal sealed class UdsClientImpl : IUdsClient private readonly SemaphoreSlim _requestLock = new(1, 1); private readonly CancellationTokenSource _lifetimeCts = new(); + // Test hook: fires whenever a caller finds _requestLock already held and starts waiting on + // it -- the observable a queued call is waiting on, standing in for a wall-clock sleep + // timed to land while an earlier call holds the lock (#171). No-op in production; a test + // subscribes to learn the wait has begun rather than guessing how long the holder needs. + internal event Action? RequestLockContended; + + // Test hook: fires once _requestLock has actually been taken -- contended or not -- so a + // test driving a holder that then blocks on something else (a gated channel double) knows + // the lock is held without guessing how long the acquisition itself takes (#171). + internal event Action? RequestLockAcquired; + + /// + /// Acquires , raising first if + /// it is already held. The non-blocking probe + /// ( with a zero timeout) keeps the + /// common uncontended path free of the event's cost and, more to the point, is what makes + /// the signal mean "a holder is in the way" rather than "a wait was requested" -- the latter + /// would fire on every call, contended or not (#171). The probe takes the token too, so a + /// call cancelled before it starts throws rather than taking a free lock, as the plain + /// it replaced did. + /// + private async Task AcquireRequestLockAsync(CancellationToken cancellationToken) + { + if (!_requestLock.Wait(0, cancellationToken)) + { + RequestLockContended?.Invoke(); + await _requestLock.WaitAsync(cancellationToken).ConfigureAwait(false); + } + RequestLockAcquired?.Invoke(); + } + private byte _currentSession = (byte)UdsSessionType.Default; private TesterPresentKeepAlive? _keepAlive; private int _disposed; @@ -322,7 +353,7 @@ public async Task SecurityAccessAsync(byte requestSeedLevel, Func RequestDownloadAsync( cancellationToken, _lifetimeCts.Token); var linkedToken = linked.Token; - await _requestLock.WaitAsync(linkedToken).ConfigureAwait(false); + await AcquireRequestLockAsync(linkedToken).ConfigureAwait(false); try { return await RequestTransferSetupCoreAsync( @@ -641,7 +672,7 @@ public async Task RequestUploadAsync( cancellationToken, _lifetimeCts.Token); var linkedToken = linked.Token; - await _requestLock.WaitAsync(linkedToken).ConfigureAwait(false); + await AcquireRequestLockAsync(linkedToken).ConfigureAwait(false); try { return await RequestTransferSetupCoreAsync( @@ -729,7 +760,7 @@ public async Task TransferDataAsync(byte blockSequenceCounter, cancellationToken, _lifetimeCts.Token); var linkedToken = linked.Token; - await _requestLock.WaitAsync(linkedToken).ConfigureAwait(false); + await AcquireRequestLockAsync(linkedToken).ConfigureAwait(false); try { return await TransferDataCoreAsync(blockSequenceCounter, data, linkedToken) @@ -779,7 +810,7 @@ public async Task RequestTransferExitAsync( cancellationToken, _lifetimeCts.Token); var linkedToken = linked.Token; - await _requestLock.WaitAsync(linkedToken).ConfigureAwait(false); + await AcquireRequestLockAsync(linkedToken).ConfigureAwait(false); try { await RequestTransferExitCoreAsync(transferRequestParameterRecord, linkedToken) @@ -864,7 +895,7 @@ public async Task DownloadAsync( // SecurityAccessAsync: acquire the lock once, then call the *Core helpers that assume // the lock is held. The public RequestDownload/TransferData/RequestTransferExit APIs // remain unchanged for single-step callers. - await _requestLock.WaitAsync(linkedToken).ConfigureAwait(false); + await AcquireRequestLockAsync(linkedToken).ConfigureAwait(false); try { var download = await RequestTransferSetupCoreAsync( @@ -932,7 +963,7 @@ public async Task UploadAsync( // Same lock discipline as DownloadAsync: one continuous 0x35 → 0x36…0x36 → 0x37 // sequence, so keep-alive traffic cannot desynchronise the ECU's block-sequence counter. - await _requestLock.WaitAsync(linkedToken).ConfigureAwait(false); + await AcquireRequestLockAsync(linkedToken).ConfigureAwait(false); try { _ = await RequestTransferSetupCoreAsync( @@ -989,7 +1020,7 @@ private async Task ExecuteAsync(UdsServiceId serviceId, byte[] request, cancellationToken, _lifetimeCts.Token); var linkedToken = linked.Token; - await _requestLock.WaitAsync(linkedToken).ConfigureAwait(false); + await AcquireRequestLockAsync(linkedToken).ConfigureAwait(false); try { return await ExecuteCoreAsync(serviceId, request, linkedToken).ConfigureAwait(false); diff --git a/src/CanKit.Pro.Uds/UdsFunctionalClient.cs b/src/CanKit.Pro.Uds/UdsFunctionalClient.cs index d4633db..ce9c8de 100644 --- a/src/CanKit.Pro.Uds/UdsFunctionalClient.cs +++ b/src/CanKit.Pro.Uds/UdsFunctionalClient.cs @@ -66,6 +66,21 @@ internal void DelayListenerStart(TimeSpan delay) private TimeSpan _listenerStartDelay; private int _disposed; + // Test hook: fires whenever a call finds _requestLock already held and starts waiting on + // it -- the observable a call queued behind another is waiting on, standing in for a + // wall-clock sleep timed to land while the earlier call's window is still open (#171). + // No-op in production. + internal event Action? RequestLockContended; + + private async Task AcquireRequestLockAsync(CancellationToken cancellationToken) + { + if (!_requestLock.Wait(0, cancellationToken)) + { + RequestLockContended?.Invoke(); + await _requestLock.WaitAsync(cancellationToken).ConfigureAwait(false); + } + } + private UdsFunctionalClient(IsoTpFunctionalClient client, bool ownsClient, TimeSpan responseWindow, TimeSpan responsePendingWindow, ProtocolActor? clock) { @@ -140,7 +155,7 @@ public async Task> SendRawAsync(ReadOnlyMem cancellationToken, _lifetimeCts.Token); var linkedToken = linked.Token; - await _requestLock.WaitAsync(linkedToken).ConfigureAwait(false); + await AcquireRequestLockAsync(linkedToken).ConfigureAwait(false); try { // Disposed while queued behind another call: the lock is released, not used. From 5119a36e2a47ccf6ee986926a78ead71a72b000b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 20:29:12 +0000 Subject: [PATCH 2/2] test(uds): wait on the request lock instead of sleeping Converts the lock and queue rows of the UDS audit (#171, audit item 7) from wall-clock sleeps to the request-lock observables: - UdsClientTests: Dispose_During_InFlight_Request_Does_Not_Race_RequestLock, Dispose_Cancels_Suppress_TesterPresent_Blocked_On_RequestLock and SecurityAccess_Holds_Lock_Across_Seed_And_Key. The last one now needs a keep-alive tick to have actually queued behind SecurityAccess; before, it counted 30 ms periods and could pass with no tick at all. - UdsExpiredDeadlineTests: N_Dispose_Leaves_The_Lock_To_A_Holder_That_Outlasts_The_Wait. - UdsFunctionalClientTests: A_Call_Queued_Behind_Another_Does_Not_Send_After_Dispose. - UdsTransferTests: DownloadAsync_Holds_Exclusive_Lock_Against_Concurrent_TesterPresent. Both TesterPresent calls must be seen queued on the lock while the download holds it, replacing the 2 ms race width. The test asserts that this happened. Adds tests for a pre-cancelled call on a free lock and for a contended lock with no subscriber. An_Invalid_Collection_Window_Transmits_Nothing keeps its 50 ms window, with a comment explaining why: it is a negative check on another bus, and a regression's frame would reach that bus asynchronously. Refs #171 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s --- .../TestCases/Uds/UdsClientTests.cs | 67 +++++++++++++++++-- .../TestCases/Uds/UdsExpiredDeadlineTests.cs | 12 +++- .../TestCases/Uds/UdsFunctionalClientTests.cs | 10 ++- .../TestCases/Uds/UdsTransferTests.cs | 28 ++++++-- 4 files changed, 104 insertions(+), 13 deletions(-) diff --git a/tests/CanKit.Pro.Tests/TestCases/Uds/UdsClientTests.cs b/tests/CanKit.Pro.Tests/TestCases/Uds/UdsClientTests.cs index 4370023..efc94b2 100644 --- a/tests/CanKit.Pro.Tests/TestCases/Uds/UdsClientTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/Uds/UdsClientTests.cs @@ -490,6 +490,22 @@ public async Task SendRaw_With_The_Suppress_Bit_Does_Not_Wait_For_A_Response() } } + // A contended lock with nobody subscribed to RequestLockContended is the production shape + // of every request lock acquisition: the event is a test hook and normally has no listener + // (#171). + [Fact] + public async Task A_Contended_Lock_Needs_No_Subscriber() + { + var (client, _, dispose) = BuildPair(e => e.On(0x22, req => new byte[] { req[1], req[2] })); + using (dispose) + { + using var cts = new CancellationTokenSource(ShortTimeout); + var first = client.ReadDataByIdentifierAsync(0xF190, cts.Token); + var second = client.ReadDataByIdentifierAsync(0xF191, cts.Token); + await Task.WhenAll(first, second); + } + } + // Codex on #150: a suppressed send may still draw a negative response, up to P2 after it. // The next request for the same service waits that window out rather than taking the // negative response as its own. @@ -1190,6 +1206,16 @@ public async Task SecurityAccess_Holds_Lock_Across_Seed_And_Key() using (dispose) { + var impl = (UdsClientImpl)client; + // The property is that a keep-alive tick queued behind SecurityAccess does not + // transmit until the lock is released -- proven by observing at least one tick + // actually contend for the lock while computeKey blocks, not by guessing how many + // 30 ms periods a fixed wait covers (#171). Stronger than the counted-periods guess + // it replaces: that could pass with zero ticks ever firing. + var keepAliveContended = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + impl.RequestLockContended += () => keepAliveContended.TrySetResult(true); + using var keepAlive = client.StartTesterPresentKeepAlive(TimeSpan.FromMilliseconds(30)); var unlock = client.SecurityAccessAsync( @@ -1198,14 +1224,14 @@ public async Task SecurityAccess_Holds_Lock_Across_Seed_And_Key() { keyStarted.TrySetResult(true); // Block inside computeKey (still under the request lock) long enough that - // several keep-alive ticks fire; they must not transmit until unlock ends. + // a keep-alive tick contends for it; it must not transmit until unlock ends. releaseKey.Task.Wait(ShortTimeout); return s.Select(b => (byte)(b ^ 0x55)).ToArray(); }, cancellationToken: new CancellationTokenSource(ShortTimeout).Token); await keyStarted.Task.WaitAsync(ShortTimeout); - await Task.Delay(120); // several keep-alive periods while lock is held + await keepAliveContended.Task.WaitAsync(ShortTimeout); // a keep-alive tick queued behind SecurityAccess releaseKey.TrySetResult(true); await unlock; @@ -1677,9 +1703,15 @@ public async Task Dispose_During_InFlight_Request_Does_Not_Race_RequestLock() P2StarClientMax = TimeSpan.FromSeconds(5), }); using var teardown = dispose; + var impl = (UdsClientImpl)client; + // The ECU never answers (EcuSilent), so once the lock is held the request stays parked + // in the receive: the observable is the lock, not a guess at how long entering the + // receive on top of it takes (#171). + var lockHeld = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + impl.RequestLockAcquired += () => lockHeld.TrySetResult(true); var inFlight = client.ReadDataByIdentifierAsync(0xF190, new CancellationTokenSource(ShortTimeout).Token); - await Task.Delay(50); // enter ReceiveWithTimeout under the request lock + await lockHeld.Task; // holds _requestLock, parked in ReceiveWithTimeout Action act = () => client.Dispose(); act.Should().NotThrow( @@ -1705,15 +1737,20 @@ public async Task Dispose_Cancels_Suppress_TesterPresent_Blocked_On_RequestLock( P2StarClientMax = TimeSpan.FromSeconds(5), }); using var teardown = dispose; + var impl = (UdsClientImpl)client; // Hold the request lock with a silent ECU read so suppress TesterPresent blocks // in WaitAsync rather than racing through Send. + var readHoldsLock = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + impl.RequestLockAcquired += () => readHoldsLock.TrySetResult(true); var inFlight = client.ReadDataByIdentifierAsync(0xF190, new CancellationTokenSource(ShortTimeout).Token); - await Task.Delay(50); + await readHoldsLock.Task; + var testerPresentQueued = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + impl.RequestLockContended += () => testerPresentQueued.TrySetResult(true); var testerPresent = client.TesterPresentAsync(suppressPositiveResponse: true, new CancellationTokenSource(ShortTimeout).Token); - await Task.Delay(30); // park on _requestLock.WaitAsync + await testerPresentQueued.Task; // parked on _requestLock.WaitAsync Action act = () => client.Dispose(); act.Should().NotThrow(); @@ -1726,6 +1763,26 @@ await waitTp.Should().ThrowAsync( await waitRead.Should().ThrowAsync(); } + // ----------------------------------------------------------------------------------- + // #171 — the uncontended fast path of the request lock must honour a token that is + // already cancelled, as the plain WaitAsync it replaced did: a call cancelled before it + // starts never takes the lock. + // ----------------------------------------------------------------------------------- + [Fact] + public async Task An_Already_Cancelled_Call_Does_Not_Take_A_Free_Request_Lock() + { + var (client, _, dispose) = BuildPair(e => e.On(0x22, _ => new byte[] { 0xF1, 0x90, 0x01 })); + using var teardown = dispose; + var impl = (UdsClientImpl)client; + var acquired = false; + impl.RequestLockAcquired += () => acquired = true; + + Func act = () => client.ReadDataByIdentifierAsync(0xF190, new CancellationToken(canceled: true)); + + await act.Should().ThrowAsync(); + acquired.Should().BeFalse("a call cancelled before it starts must not take the request lock"); + } + /// /// Records of the first /// and completes that task only afterwards, diff --git a/tests/CanKit.Pro.Tests/TestCases/Uds/UdsExpiredDeadlineTests.cs b/tests/CanKit.Pro.Tests/TestCases/Uds/UdsExpiredDeadlineTests.cs index a2cf32b..39fc969 100644 --- a/tests/CanKit.Pro.Tests/TestCases/Uds/UdsExpiredDeadlineTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/Uds/UdsExpiredDeadlineTests.cs @@ -368,10 +368,16 @@ public async Task N_Dispose_Leaves_The_Lock_To_A_Holder_That_Outlasts_The_Wait() stampArrivalAtDelivery: true) { Gate = gate }; using var client = NewClient(channel); // a second Dispose is idempotent - ((UdsClientImpl)client).DisposeLockTimeout = TimeSpan.FromMilliseconds(100); - + var impl = (UdsClientImpl)client; + impl.DisposeLockTimeout = TimeSpan.FromMilliseconds(100); + + // The request is gated indefinitely on `gate` (set below), so once it holds the lock it + // stays blocked in the receive: an observable that the lock was taken is enough, with + // no need to also observe the receive itself (#171). + var lockHeld = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + impl.RequestLockAcquired += () => lockHeld.TrySetResult(true); var inFlight = client.ReadDataByIdentifierAsync(0xF190, CancellationToken.None); - await Task.Delay(50); // let the request take the lock and block in the receive + await lockHeld.Task; // the request holds _requestLock and is blocked in the receive client.Dispose(); // returns after its 100 ms wait, the holder still inside diff --git a/tests/CanKit.Pro.Tests/TestCases/Uds/UdsFunctionalClientTests.cs b/tests/CanKit.Pro.Tests/TestCases/Uds/UdsFunctionalClientTests.cs index 27ca4cd..ab0ca89 100644 --- a/tests/CanKit.Pro.Tests/TestCases/Uds/UdsFunctionalClientTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/Uds/UdsFunctionalClientTests.cs @@ -225,10 +225,16 @@ public async Task A_Call_Queued_Behind_Another_Does_Not_Send_After_Dispose() using var functional = UdsFunctionalClient.Create( // a second Dispose is idempotent IsoTpFactory.OpenFunctional(busTester, FunctionalTxId, Ecu1, 0x7EF, FastOptions()), ownsClient: true); + // The observable a queued call is waiting on: the second SendRawAsync finds + // _requestLock already held by the first (its collection window still open) and starts + // waiting on it, rather than a guess at how long the first's window needs (#171). + var secondQueued = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + functional.RequestLockContended += () => secondQueued.TrySetResult(true); + using var cts = new CancellationTokenSource(ShortTimeout); var first = functional.SendRawAsync(new byte[] { 0x22, 0xF1, 0x90 }, TimeSpan.FromMilliseconds(300), cts.Token); var second = functional.SendRawAsync(new byte[] { 0x22, 0xF1, 0x91 }, TimeSpan.FromMilliseconds(300), cts.Token); - await Task.Delay(50); // the first is in its window, the second queued behind it + await secondQueued.Task; // the first is in its window, the second queued behind it functional.Dispose(); @@ -976,6 +982,8 @@ public async Task An_Invalid_Collection_Window_Transmits_Nothing(long windowMill var window = windowMilliseconds == long.MaxValue ? TimeSpan.MaxValue : TimeSpan.FromMilliseconds(windowMilliseconds); Func act = () => functional.DiagnosticSessionControlAsync(UdsSessionType.Extended, window); await act.Should().ThrowAsync(); + // A negative check on another bus: a frame sent by a regression that validated after + // the send would reach busEcus asynchronously, so this keeps its wall window (#171). await Task.Delay(50); seen.Should().Be(0, "nothing was transmitted"); } diff --git a/tests/CanKit.Pro.Tests/TestCases/Uds/UdsTransferTests.cs b/tests/CanKit.Pro.Tests/TestCases/Uds/UdsTransferTests.cs index 4f941aa..d21fd15 100644 --- a/tests/CanKit.Pro.Tests/TestCases/Uds/UdsTransferTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/Uds/UdsTransferTests.cs @@ -596,10 +596,25 @@ public async Task DownloadAsync_Holds_Exclusive_Lock_Against_Concurrent_TesterPr var (client, ecu, dispose) = BuildPair(e => { }); using (dispose) { + var impl = (UdsClientImpl)client; + // The race this test wants is "both TesterPresent calls have actually reached + // _requestLock while the download holds it", not "some number of milliseconds have + // passed" -- a width in milliseconds only ever gives a racer *a chance* to slip in, + // whereas the two contentions are the fact of the attempt (#171). Both are held on + // one signal so a spurious extra contention (there should be exactly two) does not + // let this open before either has actually queued. + var bothContended = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + int contentions = 0; + impl.RequestLockContended += () => + { + if (Interlocked.Increment(ref contentions) >= 2) bothContended.TrySetResult(true); + }; + // Record every SID as the ECU sees it — we assert that no 0x3E appears between // 0x34 and 0x37 (i.e. TesterPresent is not interleaved with the download). var seenSids = new System.Collections.Concurrent.ConcurrentQueue(); + int blockIndex = 0; ecu.On(0x34, req => { seenSids.Enqueue(0x34); @@ -609,9 +624,11 @@ public async Task DownloadAsync_Holds_Exclusive_Lock_Against_Concurrent_TesterPr ecu.On(0x36, req => { seenSids.Enqueue(0x36); - // Small artificial delay per block so a concurrent 3E has a real chance to - // slip in if the lock were not held. - Thread.Sleep(2); + // Only the first block waits: both racers must have genuinely reached the lock + // while the download still holds it, or the lock is not proven exclusive at + // all. Waiting on every block would only re-check what the first already showed. + if (Interlocked.Increment(ref blockIndex) == 1) + bothContended.Task.Wait(ShortTimeout); return new byte[] { req[1] }; }); ecu.On(0x37, req => { seenSids.Enqueue(0x37); return Array.Empty(); }); @@ -628,7 +645,6 @@ public async Task DownloadAsync_Holds_Exclusive_Lock_Against_Concurrent_TesterPr // Fire concurrent TesterPresent calls (both the fire-and-forget suppressed form // and the request/response form) as soon as the download begins. The request // lock must serialise them all after the download finishes. - await Task.Yield(); var tp1 = client.TesterPresentAsync(suppressPositiveResponse: true, new CancellationTokenSource(ShortTimeout).Token); var tp2 = client.TesterPresentAsync(suppressPositiveResponse: false, @@ -636,6 +652,10 @@ public async Task DownloadAsync_Holds_Exclusive_Lock_Against_Concurrent_TesterPr await Task.WhenAll(downloadTask, tp1, tp2); + // The first block's wait is bounded; had it run out, the racers were never proven + // to have queued behind the download, and the order below would prove nothing. + bothContended.Task.IsCompleted.Should().BeTrue( + "both TesterPresent calls must have queued on the lock while the download held it"); var order = seenSids.ToArray(); // First frame must be the RequestDownload (0x34) — no earlier 0x3E. order[0].Should().Be(0x34);