Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion docs/architecture/arc42-CanKit.Pro-en.md
Original file line number Diff line number Diff line change
Expand Up @@ -1141,7 +1141,11 @@ new gap", not "everything proven"; the open items are listed in the script itsel
(FR-RAW-023) — `PostAsync`/`PostAsync<T>` failures instead go through the returned task, since
the caller observes it via `await` anyway. `Dispose` rejects new work immediately
(`ObjectDisposedException`) but still runs already queued work to completion instead of leaving
a `PostAsync` task waiting at dispose time hanging indefinitely. The ISO-TP prototype
a `PostAsync` task waiting at dispose time hanging indefinitely. Its original constructor keeps
draining due timers; the additive `ActorShutdownPolicy.DiscardPendingTimers` constructor option
instead discards pending timers, including due ones, while completing accepted mailbox work.
Timer selection is ordered against disposal; an already selected callback may still finish.
The ISO-TP prototype
(functionally defective, Review §1.1, including a 100 % CPU busy loop and unsynchronised `List`
states) no longer exists; its successor `IsoTpChannel` and the J1939, J1939-TP and CANopen
packages run on `ProtocolActor`. Secured via unit/concurrency test
Expand Down
18 changes: 18 additions & 0 deletions src/CanKit.Pro.Actor/ActorShutdownPolicy.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
namespace CanKit.Pro.Actor
{
/// <summary>Chooses which pending timers a <see cref="ProtocolActor"/> executes during shutdown.</summary>
public enum ActorShutdownPolicy
{
/// <summary>
/// Preserves the original behavior: finish accepted mailbox work, then execute due timers.
/// Timers that are not yet due are discarded. Rearming after disposal is rejected.
/// </summary>
DrainDueTimers = 0,

/// <summary>
/// Finish accepted mailbox work and discard pending timers, including already due ones.
/// A timer selected for execution before disposal may still complete. No new work is accepted.
/// </summary>
DiscardPendingTimers = 1,
}
}
52 changes: 45 additions & 7 deletions src/CanKit.Pro.Actor/ProtocolActor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ public sealed class ProtocolActor : IProtocolActor
});

private readonly TimeSpan _shutdownTimeout;
private readonly ActorShutdownPolicy _shutdownPolicy;

private int _disposedFlag;

Expand Down Expand Up @@ -280,9 +281,23 @@ public ProtocolActor(ActorExecutionMode mode = ActorExecutionMode.DedicatedThrea
{
}

/// <summary>
/// Creates an actor with an explicitly selected shutdown policy. The original two-parameter
/// constructor continues to use <see cref="ActorShutdownPolicy.DrainDueTimers"/>.
/// </summary>
/// <param name="mode">Execution context for the loop (FR-RAW-024).</param>
/// <param name="synchronizationContext">Required for SynchronizationContext mode; null otherwise.</param>
/// <param name="shutdownPolicy">Whether pending due timers are executed or discarded at shutdown.</param>
/// <exception cref="ArgumentOutOfRangeException">The shutdown policy is not defined.</exception>
public ProtocolActor(ActorExecutionMode mode, SynchronizationContext? synchronizationContext,
ActorShutdownPolicy shutdownPolicy)
: this(mode, synchronizationContext, timeSource: null, shutdownTimeout: null, shutdownPolicy: shutdownPolicy)
{
}

// Test seam. Kept internal (and as a separate overload rather than extra optional
// parameters on the public constructor, which would change that constructor's compiled
// signature) so the published surface stays exactly as it was: substituting the tick source
// signature) so the original constructor signature stays as it was: substituting the tick source
// is what makes timer behaviour testable without sleeping, and shortening the shutdown
// timeout is what makes the "Dispose gave up" path testable without a five-second wait.
internal ProtocolActor(
Expand All @@ -291,8 +306,12 @@ internal ProtocolActor(
ITimeSource? timeSource,
TimeSpan? shutdownTimeout,
Action<ProtocolActor>? beforeLoopStart = null,
Action<ProtocolActor>? beforeStopRequested = null)
Action<ProtocolActor>? beforeStopRequested = null,
ActorShutdownPolicy shutdownPolicy = ActorShutdownPolicy.DrainDueTimers)
{
if (shutdownPolicy != ActorShutdownPolicy.DrainDueTimers && shutdownPolicy != ActorShutdownPolicy.DiscardPendingTimers)
throw new ArgumentOutOfRangeException(nameof(shutdownPolicy));
_shutdownPolicy = shutdownPolicy;
_beforeStopRequested = beforeStopRequested;
if (mode == ActorExecutionMode.SynchronizationContext)
{
Expand Down Expand Up @@ -779,7 +798,8 @@ private async Task RunLoopAsync()
// from that point on), but run everything already queued at the moment of Dispose to
// completion -- so a caller awaiting PostAsync exactly when Dispose happens still gets a
// real result/exception instead of a task that hangs forever. Never waits for or fires
// not-yet-due timers; those are simply discarded.
// not-yet-due timers; those are simply discarded. The opt-in DiscardPendingTimers policy
// also discards due timers instead of calling FireDueTimers after draining the mailbox.
private void FinalDrain()
{
// Unlike the live loop, this drains to genuine emptiness: the batching in DrainMailbox
Expand All @@ -793,7 +813,15 @@ private void FinalDrain()
DrainPendingTimerInserts();
}

FireDueTimers();
if (_shutdownPolicy == ActorShutdownPolicy.DiscardPendingTimers)
{
foreach (var entry in _timers) Retire(entry);
_timers.Clear();
}
else
{
FireDueTimers();
}
}

private int NextWaitTimeoutMilliseconds()
Expand Down Expand Up @@ -900,13 +928,23 @@ private void FireDueTimers()
if (_mailbox.TryPeek(out var older) && older.PostedAt < entry.DueTimestamp)
break;

_timers.RemoveAt(0);
// Selecting a callback and accepting disposal have one ordering point. Do not
// hold this gate while invoking user code or marshaling to a context: callbacks
// may dispose this actor or wait for a thread that calls Post/Schedule.
bool cancelled;
lock (_disposeGate)
{
if (_shutdownPolicy == ActorShutdownPolicy.DiscardPendingTimers && Volatile.Read(ref _disposedFlag) != 0)
break;
_timers.RemoveAt(0);
cancelled = entry.Retire();
}

// Retire() reports whether this entry had been cancelled, and does so atomically
// with taking it out of circulation -- so a Dispose racing us either wins (we skip
// the callback) or loses and finds nothing left to cancel, matching Schedule's
// documented "a callback already in flight may still complete".
if (!entry.Retire())
if (!cancelled)
RunSafely(entry.Callback);
else
Interlocked.Decrement(ref _cancelledTimerCount);
Expand Down Expand Up @@ -1067,7 +1105,7 @@ private int ToTimeoutMilliseconds(long remainingTicks)

// Loop-thread-only view of the pending timer list, for tests that need to prove cancelled
// entries are actually reclaimed rather than merely never fired. Reading _timers.Count is
// only safe from the loop, hence the accessor rather than exposing the list.
// only safe from the loop (or after joining it), hence the accessor rather than exposing the list.
internal int PendingTimerCount => _timers.Count;

// OnDispatchFailure is null for plain Post() items (BackgroundExceptionOccurred is their
Expand Down
17 changes: 16 additions & 1 deletion src/CanKit.Pro.Actor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,22 @@ using var timeout = actor.Schedule(TimeSpan.FromMilliseconds(150), () => channel
Disposing an actor stops it from accepting new work (`Post`/`Schedule` throw
`ObjectDisposedException`) but runs whatever was already queued to completion first, so a caller
awaiting `PostAsync` right as `Dispose` happens still gets a real result instead of hanging.
Not-yet-due `Schedule` callbacks are discarded, not fired. `Dispose` waits up to five seconds for
The original constructor keeps `ActorShutdownPolicy.DrainDueTimers`: due callbacks still run;
not-yet-due `Schedule` callbacks are discarded. To finish accepted mailbox work while discarding
all pending timers, including due ones, explicitly select the additive policy:

```csharp
using var actor = new ProtocolActor(ActorExecutionMode.DedicatedThread, null,
ActorShutdownPolicy.DiscardPendingTimers);
```

Timer selection and disposal are ordered under the same gate. A callback already selected before
disposal may still complete, including a callback waiting to marshal through a synchronization
context. The gate is released before invoking or marshaling user code. This policy does not cancel
accepted mailbox requests or complete tasks that depend on discarded timers: callers must still
link their own lifetime cancellation. Neither policy permits rearming after disposal.

`Dispose` waits up to five seconds for
the loop to actually finish; if a callback is still running when that elapses it returns anyway
(so `Dispose` never becomes the thing that hangs) and reports a `TimeoutException` through
`BackgroundExceptionOccurred` — a silent give-up would leave the caller unable to tell a clean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ namespace CanKit.Pro.Actor
ThreadPool = 1,
SynchronizationContext = 2,
}
public enum ActorShutdownPolicy
{
DrainDueTimers = 0,
DiscardPendingTimers = 1,
}
public interface IProtocolActor : System.IDisposable
{
event System.EventHandler<System.Exception> BackgroundExceptionOccurred;
Expand All @@ -17,6 +22,7 @@ namespace CanKit.Pro.Actor
public sealed class ProtocolActor : CanKit.Pro.Actor.IProtocolActor, System.IDisposable
{
public ProtocolActor(CanKit.Pro.Actor.ActorExecutionMode mode = 0, System.Threading.SynchronizationContext? synchronizationContext = null) { }
public ProtocolActor(CanKit.Pro.Actor.ActorExecutionMode mode, System.Threading.SynchronizationContext? synchronizationContext, CanKit.Pro.Actor.ActorShutdownPolicy shutdownPolicy) { }
public bool IsOnCurrentActor { get; }
public event System.EventHandler<System.Exception>? BackgroundExceptionOccurred;
public void Dispose() { }
Expand Down
Loading
Loading