From c0bdff51e3df518215a5f0810ab78f12b8681db6 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards <2646931+dborgards@users.noreply.github.com> Date: Fri, 2 Oct 2026 06:00:38 +0200 Subject: [PATCH] fix(canopen): stop a block download's sub-block batch when the transfer ends SendNextBlockDownloadSubBlock sends up to 127 segments as one ordered batch with no way to stop it. When the server aborted, the caller cancelled or the transfer ended otherwise while the batch was still going, only the client session was removed and the remaining segments were sent anyway. The server has no block session then and reads each segment as a command specifier: a seqno of 0x20..0x3F is a classic download initiate, and 0x23, 0x27, 0x2B and 0x2F are expedited writes that land in the dictionary when the bytes that happen to be the multiplexer name a writable object of that width. The batch now asks before each frame whether the transfer's task is complete, which every way of ending it sets and which is readable from the sending task. The upload server's batch is not touched; its peer is a client that ignores stray segments. Co-Authored-By: Claude Sonnet 5.5 --- .../CanOpenNode.SdoBlock.cs | 5 +- src/CanKit.Pro.CANopen/CanOpenNode.cs | 12 ++++ .../CanOpenSdoClientSendFailureTests.cs | 58 +++++++++++++++++++ 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs b/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs index d32f2ee..f8cb57a 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.SdoBlock.cs @@ -585,7 +585,10 @@ private void SendNextBlockDownloadSubBlock(SdoBlockClientSession session) } session.ResumeSeqno = seqno; session.Phase = SdoBlockClientPhase.AwaitSubBlockAck; - _ = SendOrderedControlFrames(TrackSdoBlockClientSend(session), frames.ToArray()); + // Every way the transfer ends completes its task (abort, peer abort, cancel, timeout, + // dispose), and that is readable from the sending task; the session tables are not. + _ = SendOrderedControlFrames(TrackSdoBlockClientSend(session), + () => session.Tcs.Task.IsCompleted, frames.ToArray()); } private void SendBlockDownloadEnd(SdoBlockClientSession session) diff --git a/src/CanKit.Pro.CANopen/CanOpenNode.cs b/src/CanKit.Pro.CANopen/CanOpenNode.cs index adb91f4..f63ef8e 100644 --- a/src/CanKit.Pro.CANopen/CanOpenNode.cs +++ b/src/CanKit.Pro.CANopen/CanOpenNode.cs @@ -2369,6 +2369,17 @@ private Task SendOrderedControlFrames(params (uint CobId, byte[] Payload)[] fram /// private Task SendOrderedControlFrames(Action? onSendCompleted, params (uint CobId, byte[] Payload)[] frames) + => SendOrderedControlFrames(onSendCompleted, shouldStop: null, frames); + + /// + /// As above. , when given, is asked before each frame and ends the + /// batch without sending the rest: a block transfer sends a whole sub-block as one batch, and + /// when the transfer ends meanwhile (the server aborted, the caller cancelled) the segments + /// still to go would reach a server that has no session for them any more, and read as + /// command specifiers. It runs on the sending task, so it must read thread-safe state only. + /// + private Task SendOrderedControlFrames(Action? onSendCompleted, + Func? shouldStop, params (uint CobId, byte[] Payload)[] frames) { return Task.Run(async () => { @@ -2377,6 +2388,7 @@ private Task SendOrderedControlFrames(Action? onSend { foreach (var (cobId, payload) in frames) { + if (shouldStop?.Invoke() == true) return; try { var frame = CanFrame.Classic(unchecked((int)cobId), payload, isExtendedFrame: false); diff --git a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs index 5addbdb..99d1122 100644 --- a/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/CANopen/CanOpenSdoClientSendFailureTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Linq; using System.Threading; using System.Threading.Tasks; using AwesomeAssertions; @@ -50,6 +51,63 @@ await FluentActions.Awaiting(() => act().WithTimeoutAsync(ShortTimeout)) reported.Should().Contain(ex => ex is CanOpenTransportException); } + public static TheoryData BlockDownloadEndings => new() { "the server aborts", "the caller cancels" }; + + // A block download sends a whole sub-block as one ordered batch. When the transfer ends while + // the batch is still going -- the server aborted, or the caller cancelled -- the segments still + // to go must not be sent: the server has no session for them any more and reads each as a + // command specifier, a seqno of 0x20..0x3F being a classic download initiate. + [Theory] + [MemberData(nameof(BlockDownloadEndings))] + public async Task A_Block_Download_That_Ends_Mid_SubBlock_Sends_No_More_Segments(string ending) + { + using var bus = ControllableBus.DeferredEchoCapable($"canopen-sdo-block-end-{Guid.NewGuid():N}"); + var toServer = new List(); + bus.OnTransmitting = frame => + { + if ((uint)frame.ID == CanOpenCobId.SdoRx(0x11)) lock (toServer) toServer.Add(frame.Data.ToArray()); + }; + using var client = CanOpen.OpenNode(bus, 0x7F, new CanOpenNodeOptions { SdoTimeout = TimeSpan.FromSeconds(30) }); + await bus.DeferredEchoes.WaitForEnqueuedAsync(1, ShortTimeout); // the boot-up + bus.DeferredEchoes.ReleaseAll(); + + var payload = Enumerable.Range(0, 70).Select(i => (byte)(0x30 + i)).ToArray(); // ten segments + using var cts = new CancellationTokenSource(); + var download = client.SdoDownloadAsync(0x11, 0x1000, 0x00, payload, SdoTransferMode.Block, cts.Token); + await bus.DeferredEchoes.WaitForEnqueuedAsync(2, ShortTimeout); // the initiate + bus.DeferredEchoes.ReleaseNext(); + bus.RaiseObserved(CanFrame.Classic(unchecked((int)CanOpenCobId.SdoTx(0x11)), + SdoBlockFrames.BuildBlockDownloadInitResponse(0x1000, 0x00, serverCrcSupported: false, blockSize: 10)), + isEcho: false); + await bus.DeferredEchoes.WaitForEnqueuedAsync(3, ShortTimeout); // segment 1, its confirmation held + + if (ending == "the server aborts") + { + bus.RaiseObserved(CanFrame.Classic(unchecked((int)CanOpenCobId.SdoTx(0x11)), + SdoFrames.BuildAbort(0x1000, 0x00, (uint)SdoAbortCode.General)), isEcho: false); + await FluentActions.Awaiting(() => download.WithTimeoutAsync(ShortTimeout)) + .Should().ThrowAsync(); + } + else + { + cts.Cancel(); + await FluentActions.Awaiting(() => download.WithTimeoutAsync(ShortTimeout)) + .Should().ThrowAsync(); + } + + // The transfer is over; let the batch carry on. A batch that ignores that sends nine more. + bus.DeferredEchoes.ReleaseAll(); + // No signal says "nothing more is coming", so this is a negative window: it can only pass + // falsely on a slow host, never fail falsely. + await Task.Delay(TimeSpan.FromMilliseconds(300)); + bus.DeferredEchoes.ReleaseAll(); + + byte[][] sent; + lock (toServer) sent = toServer.ToArray(); + sent.Length.Should().Be(ending == "the server aborts" ? 2 : 3, + "the initiate and segment 1 -- and, after a cancel, the abort -- and nothing after"); + } + public static TheoryData Uploads => new() { "upload", "block upload" }; [Theory]