From a7daeebc7ba5bd4ab90549f65c9def24484d5603 Mon Sep 17 00:00:00 2001 From: Dietmar Borgards Date: Sun, 25 Oct 2026 09:51:02 +0000 Subject: [PATCH] fix(isotp): report the owned actor's shutdown timeout before unsubscribing IsoTpChannel.Dispose detached its handler from the actor before disposing the actor it owns. That Dispose is where the actor reports a callback that outlived its shutdown bound -- a TimeoutException through its BackgroundExceptionOccurred -- so the channel's subscribers never heard of it. The handler now comes off after the owned actor's Dispose; for an injected actor nothing runs between the two, so the order is unchanged. Same ordering as #437 for the J1939-TP channel and #414 for the node. An internal ownsActor constructor seam hands an injected actor to the channel to dispose, so the test can give the channel's own actor a short shutdown bound. The test holds the actor's loop until its finally, so the bound is always exceeded, and awaits the report on a signal. Closes #449 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011AJKME1N1ZvnY3yYcPwNoF --- src/CanKit.Pro.IsoTp/IsoTpChannel.cs | 16 ++++++-- .../IsoTp/IsoTpChannelIntegrationTests.cs | 41 +++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/CanKit.Pro.IsoTp/IsoTpChannel.cs b/src/CanKit.Pro.IsoTp/IsoTpChannel.cs index 32d5ae4e..d7013b04 100644 --- a/src/CanKit.Pro.IsoTp/IsoTpChannel.cs +++ b/src/CanKit.Pro.IsoTp/IsoTpChannel.cs @@ -203,15 +203,20 @@ internal sealed class IsoTpChannel : IIsoTpChannel /// so substituting the actor puts all of them on the one clock the test controls. The /// Stopwatch readings that remain here are arrival and transmit *instants* on the /// wire (#112), which are facts about the host and deliberately not on the actor's clock. + /// + /// hands an injected to the channel to + /// dispose, as the one it creates for itself: a seam for a test that needs the channel's own + /// disposal of its actor on a loop with a short shutdown bound (#449). + /// /// internal IsoTpChannel(ICanBusService service, IsoTpEndpoint endpoint, - IsoTpChannelOptions options, bool ownsService, IProtocolActor? actor = null) + IsoTpChannelOptions options, bool ownsService, IProtocolActor? actor = null, bool ownsActor = false) { _service = service ?? throw new ArgumentNullException(nameof(service)); _endpoint = endpoint; _options = options ?? throw new ArgumentNullException(nameof(options)); _ownsService = ownsService; - _ownsActor = actor is null; + _ownsActor = actor is null || ownsActor; // Encode once: EncodeStMin throws on negative values; surfacing at Open keeps the RX // path free of codec throws that ProtocolActor would only raise as BackgroundException. _localStMinRaw = IsoTpFrameCodec.EncodeStMin(_options.LocalStMin); @@ -771,9 +776,12 @@ void FailInFlightSend() // SendAsync can leave its await and enter WaitForBusTxIdleAsync / Release. An injected // actor is not ours to dispose -- the caller may still be running other channels on it -- // but the handler is, so it comes off either way rather than outliving this channel on a - // loop that keeps going. - _actor.BackgroundExceptionOccurred -= OnActorBackgroundException; + // loop that keeps going. For an owned actor only after its Dispose: that is where the + // actor reports a callback that outlived its shutdown bound, as a TimeoutException through + // its BackgroundExceptionOccurred, and unsubscribed first the channel's subscribers never + // heard of it (#449, as #437 for the J1939-TP channel). if (_ownsActor) _actor.Dispose(); + _actor.BackgroundExceptionOccurred -= OnActorBackgroundException; _readerCts.Dispose(); // _sendGate is deliberately not disposed, and not waited for either (#339): the wait diff --git a/tests/CanKit.Pro.Tests/TestCases/IsoTp/IsoTpChannelIntegrationTests.cs b/tests/CanKit.Pro.Tests/TestCases/IsoTp/IsoTpChannelIntegrationTests.cs index f19bf7ae..de284f82 100644 --- a/tests/CanKit.Pro.Tests/TestCases/IsoTp/IsoTpChannelIntegrationTests.cs +++ b/tests/CanKit.Pro.Tests/TestCases/IsoTp/IsoTpChannelIntegrationTests.cs @@ -1337,6 +1337,47 @@ public async Task A_Pending_Receive_Ends_On_Dispose_Although_The_Actor_Is_Busy() } } + // #449: the channel's own actor, disposed while a callback still holds its loop, gives up on + // the join after its shutdown bound and reports that as a TimeoutException through its own + // BackgroundExceptionOccurred. The channel forwards it only while its handler is still + // attached, so the handler comes off after that Dispose, not before (as #437 for the J1939-TP + // channel). The callback is held until the finally, so the bound is always exceeded: nothing + // is asserted against the clock. + [Fact] + public async Task The_Shutdown_Timeout_Of_The_Channels_Own_Actor_Reaches_Its_Subscribers() + { + var session = NewSession(); + using var bus = OpenClassic(session, 0); + using var service = new CanBusService(bus); + using var release = new ManualResetEventSlim(); // declared first, so it outlives the channel + var actor = new ProtocolActor(ActorExecutionMode.DedicatedThread, null, null, shutdownTimeout: TimeSpan.FromMilliseconds(50)); + using var channel = new IsoTpChannel(service, IsoTpEndpoint.Normal(0x123, 0x321), FastOptions(), + ownsService: false, actor, ownsActor: true); // disposes the actor it owns + // Reported on the thread pool (#356, T-2), so possibly after Dispose has returned: awaited + // on a signal. A report the channel never forwards leaves the signal unset and the wait fails. + var timedOut = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + channel.BackgroundExceptionOccurred += (_, ex) => { if (ex is TimeoutException) timedOut.TrySetResult(ex); }; + + var held = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + try + { + actor.Post(() => + { + held.TrySetResult(true); + release.Wait(TimeSpan.FromSeconds(30)); + }); + await held.Task.WaitAsync(ShortTimeout); + channel.Dispose(); // the actor's join gives up on the held callback and reports it + + (await timedOut.Task.WaitAsync(ShortTimeout)).Should().BeOfType( + "the channel owns that actor, and its shutdown timeout is a background failure of the channel's"); + } + finally + { + release.Set(); + } + } + // Review on #275: the inbox's writer stays open now, so a producer callback that passed the // disposed guard before the disposal and writes after it must not put anything in it. The // callback is simulated by calling the producers directly after the disposal.