diff --git a/CHANGELOG.md b/CHANGELOG.md index 2257e67..54b5856 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2 ## [Unreleased] +### Fixed +- `SimulatedTimeProvider`: rescheduling a queued timer with `ITimer.Change` no longer corrupts the timer queue. Moving the earliest timer later, or stopping it with `Infinite`, hid every other due timer from `Advance` until that timer was reached, so `CancellationTokenSource.CancelAfter`, `TryReset` and `PeriodicTimer.Period` on a provider-made timer could stall unrelated delays and timeouts. Queue entries now carry an immutable `(due, id)` priority and a schedule version, and stale entries are discarded. +- `SimulatedTimeProvider`: a timer disposed or rescheduled by an earlier callback of the same `Advance` no longer fires for that occurrence. A one-shot timer is spent when its callback runs rather than when `Advance` finds it due. If a callback throws, the one-shot timers that `Advance` found due and did not reach are spent, as before. +- `SimulatedTimeProvider`: disposed timers and superseded schedules no longer accumulate in the timer queue behind a long-lived timer. The queue is rebuilt from its live entries whenever stale ones outnumber them, so it holds at most about twice the live timers, and a disposed or spent timer releases its callback and state. + ## [1.4.0] - 2026-05-16 ### Fixed diff --git a/property-tests/SimulatedTimeProviderProperties.fs b/property-tests/SimulatedTimeProviderProperties.fs index b818d1b..238669f 100644 --- a/property-tests/SimulatedTimeProviderProperties.fs +++ b/property-tests/SimulatedTimeProviderProperties.fs @@ -192,3 +192,65 @@ let ``GetElapsedTime matches advanced time`` (advanceMs: uint16) = // Allow small rounding differences abs (elapsed.TotalMilliseconds - expectedElapsed.TotalMilliseconds) < 0.01 + +/// Reference model for the timer queue: each advance fires every live timer that is due, once, in (due, id) order; +/// a periodic timer is rescheduled one period after the advanced-to time; a disposed timer, or a one-shot timer +/// that has fired, refuses Change. A due of None never comes. The queue never holds more than twice the most +/// timers live at once, however many have been changed or disposed. +type private ModelTimer = { Id: int; mutable Due: int64 option; mutable Period: int64; mutable Done: bool } + +/// Property: under any sequence of create, change, dispose and advance, timers fire as the reference model says +[] +let ``Timers fire as a reference model says under change and dispose`` (ops: (byte * byte * byte) list) = + let timeProvider = new SimulatedTimeProvider() + let timers = ResizeArray() + let model = ResizeArray() + let fired = ResizeArray() + let mutable now = 0L + let mutable mostLive = 0 + let mutable agrees = true + + let dueOf (b: byte) = if b % 8uy = 7uy then None else Some (int64 (b % 50uy)) + let periodOf (b: byte) = if b % 3uy = 0uy then int64 (b % 20uy) + 1L else 0L + let dueSpan due = + match due with + | Some ms -> TimeSpan.FromMilliseconds(float ms) + | None -> System.Threading.Timeout.InfiniteTimeSpan + let periodSpan period = + if period = 0L then System.Threading.Timeout.InfiniteTimeSpan else TimeSpan.FromMilliseconds(float period) + + for (kind, a, b) in List.truncate 60 ops do + match kind % 4uy with + | 0uy -> + let id = model.Count + let due, period = dueOf a, periodOf b + model.Add({ Id = id; Due = due |> Option.map ((+) now); Period = period; Done = false }) + timers.Add(timeProvider.CreateTimer((fun state -> fired.Add(unbox state)), box id, dueSpan due, periodSpan period)) + mostLive <- max mostLive (model |> Seq.filter (fun timer -> not timer.Done) |> Seq.length) + | 1uy when model.Count > 0 -> + let timer = model[int a % model.Count] + let due, period = dueOf b, periodOf (a ^^^ b) + let expected = not timer.Done + if expected then + timer.Due <- due |> Option.map ((+) now) + timer.Period <- period + agrees <- agrees && timers[timer.Id].Change(dueSpan due, periodSpan period) = expected + | 2uy when model.Count > 0 -> + let timer = model[int a % model.Count] + timer.Done <- true + timers[timer.Id].Dispose() + | _ -> + let by = int64 (a % 30uy) + now <- now + by + let due = + model + |> Seq.filter (fun timer -> not timer.Done && timer.Due |> Option.exists (fun d -> d <= now)) + |> Seq.sortBy (fun timer -> timer.Due.Value, timer.Id) + |> Seq.toList + for timer in due do + if timer.Period > 0L then timer.Due <- Some (now + timer.Period) else timer.Done <- true + fired.Clear() + timeProvider.Advance(TimeSpan.FromMilliseconds(float by)) + agrees <- agrees && List.ofSeq fired = (due |> List.map (fun timer -> timer.Id)) + + agrees && timeProvider.Statistics.MaxQueueLength <= 2L * int64 mostLive diff --git a/src/SimulatedTimeProvider.cs b/src/SimulatedTimeProvider.cs index bbe444a..e2fb03f 100644 --- a/src/SimulatedTimeProvider.cs +++ b/src/SimulatedTimeProvider.cs @@ -20,7 +20,15 @@ public sealed class SimulatedTimeProvider : TimeProvider private long _schedulerTicks; private long _nextId; - private readonly PriorityQueue _queue; + + // Each entry carries the timer's schedule version and an immutable (due, id) priority, so the heap order always + // holds. Change supersedes a timer's entry and Dispose abandons it, leaving the stale entry in place: Advance + // discards stale entries that reach the head, and the queue is rebuilt from live entries whenever stale ones + // outnumber them, so it never holds more than about twice the live timers. + private readonly PriorityQueue<(ScheduledTimer Timer, long Version), (long DueAtTicks, long Id)> _queue; + + // Timers whose current entry is in the queue; every other queued entry is stale. + private int _queued; /// /// Gets lightweight counters that can be used to observe scheduling behavior during simulation. @@ -38,7 +46,7 @@ public SimulatedTimeProvider(DateTimeOffset? startTime = null, TimeZoneInfo? loc _localTimeZone = localTimeZone ?? TimeZoneInfo.Utc; _schedulerTicks = 0; - _queue = new PriorityQueue(new ScheduledTimerComparer()); + _queue = new PriorityQueue<(ScheduledTimer Timer, long Version), (long DueAtTicks, long Id)>(); } /// @@ -135,48 +143,41 @@ public void Advance(TimeSpan by) Statistics.RecordAdvance(by); - List<(TimerCallback Callback, object? State)>? due = null; + List<(ScheduledTimer Timer, long Version)>? due = null; lock (_gate) { _utcNow = _utcNow.Add(by); _schedulerTicks += by.Ticks; - while (_queue.TryPeek(out var timer, out _)) + while (_queue.TryPeek(out var entry, out var priority)) { - if (timer.IsDisposed) + if (IsStale(entry)) { _queue.Dequeue(); continue; } - if (timer.DueAtTicks > _schedulerTicks) + if (priority.DueAtTicks > _schedulerTicks) { break; } _queue.Dequeue(); - - if (timer.IsDisposed) - { - continue; - } - + Unqueue(entry.Timer); due ??= []; - due.Add((timer.Callback, timer.State)); + due.Add(entry); // Periodic timers: coalesce on jump; schedule next occurrence from "now". - if (timer.PeriodTicks > 0) + if (entry.Timer.PeriodTicks > 0) { - timer.DueAtTicks = _schedulerTicks + timer.PeriodTicks; - _queue.Enqueue(timer, timer); + entry.Timer.DueAtTicks = _schedulerTicks + entry.Timer.PeriodTicks; + Enqueue(entry.Timer); Statistics.RecordPeriodicReschedule(_queue.Count); } - else - { - timer.MarkDisposed(); - } } + + CompactIfMostlyStale(); } if (due is null) @@ -184,11 +185,53 @@ public void Advance(TimeSpan by) return; } - // Fire callbacks outside lock. - foreach (var (callback, state) in due) + // Fire callbacks outside lock. A callback may dispose or reschedule a timer that this advance has already + // found due; such a timer does not fire for this occurrence. + var next = 0; + try { - Statistics.RecordCallbackFired(); - callback(state); + for (; next < due.Count; next++) + { + var (timer, version) = due[next]; + TimerCallback? callback; + object? state; + lock (_gate) + { + if (timer.IsDisposed || timer.Version != version) + { + continue; + } + + callback = timer.Callback; + state = timer.State; + if (timer.PeriodTicks == 0) + { + timer.MarkDisposed(); + } + } + + Statistics.RecordCallbackFired(); + callback!(state); + } + } + finally + { + if (next < due.Count) + { + // A callback threw. The one-shot timers this advance found due and did not reach are spent, as they + // were when Advance marked them on collection; periodic ones keep their next occurrence. + lock (_gate) + { + for (var i = next + 1; i < due.Count; i++) + { + var (timer, version) = due[i]; + if (timer.PeriodTicks == 0 && !timer.IsDisposed && timer.Version == version) + { + timer.MarkDisposed(); + } + } + } + } } } @@ -221,35 +264,54 @@ public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSp var periodTicks = period == Timeout.InfiniteTimeSpan ? 0 : period.Ticks; var timer = new ScheduledTimer(this, id, callback, state, dueTicks, periodTicks); - _queue.Enqueue(timer, timer); + Enqueue(timer); Statistics.RecordTimerCreated(_queue.Count); return timer; } } - private void Reschedule(ScheduledTimer timer) + // Queues the timer's current schedule. Callers hold _gate. + private void Enqueue(ScheduledTimer timer) + { + _queue.Enqueue((timer, timer.Version), (timer.DueAtTicks, timer.Id)); + timer.Queued = true; + _queued++; + } + + // The timer's queued entry has been dequeued, superseded or abandoned. Callers hold _gate. + private void Unqueue(ScheduledTimer timer) { - if (timer.IsDisposed) + if (timer.Queued) { - return; + timer.Queued = false; + _queued--; } - - _queue.Enqueue(timer, timer); - Statistics.RecordQueueEnqueue(_queue.Count); } - private sealed class ScheduledTimerComparer : IComparer + // An entry is current only while its timer is queued at the entry's version. Callers hold _gate. + private static bool IsStale((ScheduledTimer Timer, long Version) entry) => + !entry.Timer.Queued || entry.Version != entry.Timer.Version; + + // Rebuilds the queue from its current entries once stale ones outnumber them, in O(n): each stale entry is + // dropped once, so the rebuilds cost O(1) per change or disposal amortized. Callers hold _gate. + private void CompactIfMostlyStale() { - public int Compare(ScheduledTimer? x, ScheduledTimer? y) + if (_queue.Count - _queued <= _queued) { - if (ReferenceEquals(x, y)) return 0; - if (x is null) return -1; - if (y is null) return 1; + return; + } - var cmp = x.DueAtTicks.CompareTo(y.DueAtTicks); - if (cmp != 0) return cmp; - return x.Id.CompareTo(y.Id); + var current = new List<((ScheduledTimer Timer, long Version) Entry, (long DueAtTicks, long Id) Priority)>(_queued); + foreach (var item in _queue.UnorderedItems) + { + if (!IsStale(item.Element)) + { + current.Add(item); + } } + + _queue.Clear(); + _queue.EnqueueRange(current); } private sealed class ScheduledTimer : ITimer @@ -278,13 +340,19 @@ public ScheduledTimer( /// public long Id { get; } /// - /// Gets the delegate to invoke when the timer fires. + /// Gets the delegate to invoke when the timer fires, or null once the timer is disposed or spent. + /// + public TimerCallback? Callback { get; private set; } + /// + /// Gets the state object passed to the timer callback, released with . /// - public TimerCallback Callback { get; } + public object? State { get; private set; } + /// - /// Gets the state object passed to the timer callback. + /// Gets or sets whether the timer's current entry is in the owner's queue. Read and written under the + /// owner's lock. /// - public object? State { get; } + public bool Queued { get; set; } /// /// Gets or sets the due time of the timer, in scheduler ticks. @@ -295,6 +363,12 @@ public ScheduledTimer( /// public long PeriodTicks { get; set; } + /// + /// Gets the version of the timer's schedule, which increments. A queued entry with an + /// older version is stale. Read and written under the owner's lock. + /// + public long Version { get; private set; } + /// /// Gets a value indicating whether this timer has been disposed. /// @@ -332,8 +406,13 @@ public bool Change(TimeSpan dueTime, TimeSpan period) DueAtTicks = dueTicks; PeriodTicks = period == Timeout.InfiniteTimeSpan ? 0 : period.Ticks; + // Never reorder a queued entry: supersede it, and queue the new schedule. + _owner.Unqueue(this); + Version++; _owner.Statistics.RecordTimerChange(); - _owner.Reschedule(this); + _owner.CompactIfMostlyStale(); + _owner.Enqueue(this); + _owner.Statistics.RecordQueueEnqueue(_owner._queue.Count); return true; } } @@ -343,10 +422,19 @@ public bool Change(TimeSpan dueTime, TimeSpan period) /// public void Dispose() { - if (Interlocked.Exchange(ref _disposed, 1) == 0) + lock (_owner._gate) { - _owner.Statistics.RecordTimerDisposed(); + if (Interlocked.Exchange(ref _disposed, 1) != 0) + { + return; + } + + Release(); + _owner.Unqueue(this); + _owner.CompactIfMostlyStale(); } + + _owner.Statistics.RecordTimerDisposed(); } /// @@ -358,12 +446,22 @@ public ValueTask DisposeAsync() return ValueTask.CompletedTask; } + // A one-shot timer that has fired, or been passed over by a throwing callback, is spent. Callers hold the + // owner's lock; the timer's entry is already out of the queue. internal void MarkDisposed() { if (Interlocked.Exchange(ref _disposed, 1) == 0) { + Release(); _owner.Statistics.RecordTimerDisposed(); } } + + // A disposed or spent timer drops its callback and state, so an entry still queued for it pins neither. + private void Release() + { + Callback = null; + State = null; + } } } diff --git a/tests/SimulatedTimeProviderTests.cs b/tests/SimulatedTimeProviderTests.cs index 153dee4..0357e8c 100644 --- a/tests/SimulatedTimeProviderTests.cs +++ b/tests/SimulatedTimeProviderTests.cs @@ -1,3 +1,4 @@ +using System.Runtime.CompilerServices; using Xunit; using Clockworks.Instrumentation; @@ -323,4 +324,192 @@ public void Timer_callback_can_make_another_timer_due_and_it_fires_on_next_advan tp.Advance(TimeSpan.Zero); Assert.Equal([1, 2], list); } + + [Fact] + public void Changing_the_earliest_timer_later_does_not_hide_a_due_timer() + { + var tp = SimulatedTimeProvider.FromEpoch(); + var fired = new List(); + + using var a = tp.CreateTimer(_ => fired.Add("a"), state: null, dueTime: TimeSpan.FromSeconds(10), period: Timeout.InfiniteTimeSpan); + using var b = tp.CreateTimer(_ => fired.Add("b"), state: null, dueTime: TimeSpan.FromSeconds(20), period: Timeout.InfiniteTimeSpan); + + Assert.True(a.Change(TimeSpan.FromSeconds(25), Timeout.InfiniteTimeSpan)); + + tp.Advance(TimeSpan.FromSeconds(20)); + Assert.Equal(["b"], fired); + + tp.Advance(TimeSpan.FromSeconds(5)); + Assert.Equal(["b", "a"], fired); + } + + [Fact] + public void Stopping_the_earliest_timer_does_not_block_later_timers() + { + var tp = SimulatedTimeProvider.FromEpoch(); + var fired = new List(); + + using var a = tp.CreateTimer(_ => fired.Add("a"), state: null, dueTime: TimeSpan.FromSeconds(10), period: Timeout.InfiniteTimeSpan); + using var b = tp.CreateTimer(_ => fired.Add("b"), state: null, dueTime: TimeSpan.FromSeconds(20), period: Timeout.InfiniteTimeSpan); + + Assert.True(a.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan)); + + tp.Advance(TimeSpan.FromDays(1)); + Assert.Equal(["b"], fired); + } + + [Fact] + public void CancelAfter_on_a_provider_source_does_not_hide_a_due_delay() + { + var tp = SimulatedTimeProvider.FromEpoch(); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(1), tp); + var delay = Task.Delay(TimeSpan.FromSeconds(2), tp); + + // CancelAfter reschedules the source's timer through ITimer.Change. + cts.CancelAfter(TimeSpan.FromSeconds(5)); + + tp.Advance(TimeSpan.FromSeconds(3)); + Assert.True(delay.IsCompletedSuccessfully); + Assert.False(cts.IsCancellationRequested); + + tp.Advance(TimeSpan.FromSeconds(2)); + Assert.True(cts.IsCancellationRequested); + } + + [Fact] + public void Timer_disposed_by_an_earlier_callback_in_the_same_advance_does_not_fire() + { + var tp = SimulatedTimeProvider.FromEpoch(); + var fired = new List(); + + ITimer? second = null; + using var first = tp.CreateTimer(_ => + { + fired.Add("first"); + second!.Dispose(); + }, state: null, dueTime: TimeSpan.FromSeconds(1), period: Timeout.InfiniteTimeSpan); + second = tp.CreateTimer(_ => fired.Add("second"), state: null, dueTime: TimeSpan.FromSeconds(2), period: Timeout.InfiniteTimeSpan); + + tp.Advance(TimeSpan.FromSeconds(2)); + + Assert.Equal(["first"], fired); + } + + [Fact] + public void Timer_rescheduled_by_an_earlier_callback_in_the_same_advance_fires_at_its_new_due_time() + { + var tp = SimulatedTimeProvider.FromEpoch(); + var fired = new List(); + + ITimer? second = null; + using var first = tp.CreateTimer(_ => + { + fired.Add("first"); + Assert.True(second!.Change(TimeSpan.FromSeconds(10), Timeout.InfiniteTimeSpan)); + }, state: null, dueTime: TimeSpan.FromSeconds(1), period: Timeout.InfiniteTimeSpan); + using var secondTimer = second = tp.CreateTimer(_ => fired.Add("second"), state: null, dueTime: TimeSpan.FromSeconds(2), period: Timeout.InfiniteTimeSpan); + + tp.Advance(TimeSpan.FromSeconds(2)); + Assert.Equal(["first"], fired); + + tp.Advance(TimeSpan.FromSeconds(10)); + Assert.Equal(["first", "second"], fired); + } + + [Theory] + [InlineData("disabled")] + [InlineData("far-future")] + public void Rescheduled_timers_do_not_accumulate_behind_a_live_periodic_timer(string firstSchedule) + { + var tp = SimulatedTimeProvider.FromEpoch(); + using var heartbeat = tp.CreateTimer(static _ => { }, state: null, dueTime: TimeSpan.FromSeconds(1), period: TimeSpan.FromSeconds(1)); + + var payloads = ArmFireAndDispose(tp, 1_000, firstSchedule == "disabled" ? Timeout.InfiniteTimeSpan : TimeSpan.FromDays(1)); + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + + // At most the heartbeat and one armed timer are live at a time, so the queue never needs more than + // twice that, and no fired, disposed timer keeps its state reachable. + Assert.InRange(tp.Statistics.MaxQueueLength, 1, 4); + Assert.DoesNotContain(payloads, payload => payload.IsAlive); + GC.KeepAlive(heartbeat); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static WeakReference[] ArmFireAndDispose(SimulatedTimeProvider tp, int count, TimeSpan firstDueTime) + { + var payloads = new WeakReference[count]; + for (var i = 0; i < count; i++) + { + var payload = new byte[1024]; + payloads[i] = new WeakReference(payload); + using var timer = tp.CreateTimer(static _ => { }, payload, firstDueTime, Timeout.InfiniteTimeSpan); + Assert.True(timer.Change(TimeSpan.FromSeconds(1), Timeout.InfiniteTimeSpan)); + tp.Advance(TimeSpan.FromSeconds(1)); + } + + return payloads; + } + + [Theory] + [InlineData("disposed")] + [InlineData("fired")] + public void A_finished_timer_releases_its_state_while_a_stale_entry_for_it_is_queued(string ending) + { + var tp = SimulatedTimeProvider.FromEpoch(); + var live = new ITimer[4]; + for (var i = 0; i < live.Length; i++) + { + live[i] = tp.CreateTimer(static _ => { }, state: null, dueTime: TimeSpan.FromHours(1), period: Timeout.InfiniteTimeSpan); + } + + // Four live timers outnumber the one stale entry, and it is due after them, so the queue keeps it. + var payload = ArmAndFinish(tp, ending); + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + + Assert.False(payload.IsAlive); + GC.KeepAlive(live); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static WeakReference ArmAndFinish(SimulatedTimeProvider tp, string ending) + { + var payload = new byte[1024]; + var timer = tp.CreateTimer(static _ => { }, payload, TimeSpan.FromHours(2), Timeout.InfiniteTimeSpan); + if (ending == "disposed") + { + timer.Dispose(); + } + else + { + Assert.True(timer.Change(TimeSpan.FromMilliseconds(1), Timeout.InfiniteTimeSpan)); + tp.Advance(TimeSpan.FromMilliseconds(1)); + } + + return new WeakReference(payload); + } + + [Fact] + public void A_throwing_callback_leaves_the_other_due_one_shot_timers_of_its_advance_spent() + { + var tp = SimulatedTimeProvider.FromEpoch(); + var fired = new List(); + + using var throwing = tp.CreateTimer(static _ => throw new InvalidOperationException("callback failed"), state: null, dueTime: TimeSpan.FromSeconds(1), period: Timeout.InfiniteTimeSpan); + using var later = tp.CreateTimer(_ => fired.Add("later"), state: null, dueTime: TimeSpan.FromSeconds(2), period: Timeout.InfiniteTimeSpan); + using var periodic = tp.CreateTimer(_ => fired.Add("periodic"), state: null, dueTime: TimeSpan.FromSeconds(2), period: TimeSpan.FromSeconds(5)); + + Assert.Throws(() => tp.Advance(TimeSpan.FromSeconds(2))); + + // As in 1.4.0, a one-shot timer this advance found due is spent even though the exception kept its + // callback from running: it cannot be rescheduled and never fires. A periodic timer keeps its next + // occurrence. + Assert.False(later.Change(TimeSpan.FromSeconds(1), Timeout.InfiniteTimeSpan)); + tp.Advance(TimeSpan.FromSeconds(5)); + Assert.Equal(["periodic"], fired); + } }