Fixes #5652. Execute timeout callbacks outside the queue lock - #5655
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Per-token cancellation bookkeeping can cancel newly added occurrences while allowing pre-existing active occurrences to reschedule.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves timeout callbacks outside the queue lock while preserving serialized execution and cancellation behavior.
Changes:
- Adds a reentrant runner gate and pending-run handoff.
- Tracks active/cancelled repeating timeouts.
- Adds concurrency and synchronization-context regression tests.
File summaries
| File | Description |
|---|---|
TimedEvents.cs |
Refactors timer execution and cancellation locking. |
TimedEventsTests.cs |
Adds concurrency and deadlock regression coverage. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Pending follow-up callback exceptions can be silently discarded.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Terminal.Gui/App/Timeout/TimedEvents.cs:133
- When a pending follow-up pass also throws after an earlier callback failed,
??=silently discards the later exception even though that timeout has already been dequeued. No caller can observe or retry that failure; accumulate subsequent failures (for example, in anAggregateException) before rethrowing.
catch (Exception ex)
{
error ??= System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture (ex);
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The core timer scheduler now relies on intricate cross-thread handoff and reentrant cancellation behavior requiring final human review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Address review nits on the timer cancellation rework: - Add a deterministic test for the documented contract that Remove returns true for an occurrence that has already been dequeued and is executing, and neither waits for nor interrupts that callback. This was documented on ITimedEvents.Remove but not covered by a test. - Note that ActiveTimeoutState is a mutable struct in a Dictionary, so every mutation must be written back before the queue lock is released. - Document ActiveTimeoutOccurrence and ActiveTimeoutState. - Note that Remove intentionally scans the whole queue, because the same Timeout instance can be queued more than once. - Correct the class-level thread-safety docs. The blanket "Thread-safe for concurrent access" claim was inaccurate: Timeouts returns the live queue rather than a snapshot and is not synchronized. - Replace the orphaned <inheritdoc/> on CheckTimers with real docs. CheckTimers is not declared on ITimedEvents, so it inherited nothing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Prevent zero-delay repeating timeouts from starving the main loop by limiting each pass to the number of callbacks due at its start. Validate Timeout inputs, return a synchronized queue snapshot, and report the collision-adjusted Added timestamp with deterministic regression coverage.
There was a problem hiding this comment.
🟡 Changes recommended
Immediate rescheduling can let one repeating timeout consume the pass budget and indefinitely starve other due timers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1169
- This AI-test marker omits the required separator. The accepted forms are documented in
.claude/rules/testing-patterns.md:15-22.
This issue also appears in the following locations of the same file:
- line 1178
- line 1189
- line 1205
- line 1220
Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1178
- This AI-test marker omits the required separator. The accepted forms are documented in
.claude/rules/testing-patterns.md:15-22.
// Claude Opus 5
Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1189
- This AI-test marker omits the required separator. The accepted forms are documented in
.claude/rules/testing-patterns.md:15-22.
// Claude Opus 5
Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1205
- This AI-test marker omits the required separator. The accepted forms are documented in
.claude/rules/testing-patterns.md:15-22.
// Claude Opus 5
Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1220
- This AI-test marker omits the required separator. The accepted forms are documented in
.claude/rules/testing-patterns.md:15-22.
// Claude Opus 5
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Assign each queued timeout occurrence a monotonic identity and limit a timer pass to occurrences that were already queued and due when it began. Add a deterministic zero-delay repeater plus peer regression and correct the AI test markers noted by review.
There was a problem hiding this comment.
🔵 Needs a closer look
The intricate concurrency and cancellation state machine warrants final human review despite strong regression coverage.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
CompleteTimeout runs from the finally that follows a timeout callback, and it raised Added. That made an Added subscriber the only user-code call site reachable from that finally, so a throwing subscriber could replace an exception raised by the callback itself. The masking was not reachable in practice: a throwing callback leaves repeat false, so CompleteTimeout took its early-return path, which only reads the active-state entry, decrements it, and either removes it or assigns over an existing key. None of that allocates, so none of it could throw. The guard was incidental rather than designed, though, and any future change that let repeat be true on an error path would have turned it into real masking. Return the reschedule result from CompleteTimeout instead and raise Added from the timer loop once the callback has returned normally. Bookkeeping and rescheduling stay in the finally, still under the queue lock, so the ordering Remove depends on is unchanged, and Added is still raised after that lock is released. Document the resulting contract: an Added handler exception ends the timer pass while leaving the timeout scheduled, so a handler that throws on every reschedule ends every subsequent pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
One claimed exception-ordering regression test never exercises its Added handler.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
A throwing timeout callback cannot also request rescheduling because the callback result is never assigned. The Added event is therefore unreachable on that path, so the exception-masking regression did not exercise the behavior it claimed. Remove the unsupported regression and restore the simpler occurrence-completion flow. Keep the separate coverage for the reachable case where an Added handler throws after a repeating timeout has been rescheduled.
There was a problem hiding this comment.
🟡 Changes recommended
Timer selection can degrade quadratically with many immediately repeating timeouts, and wrapper documentation remains inconsistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Terminal.Gui/App/Timeout/TimedEvents.cs:407
- This rescans deferred occurrences from index 0 after every callback. With N same-time zero-span repeaters, each reschedule reuses a low queue key but receives an ID above the cutoff, so the next lookup skips 1, then 2, … deferred entries—making one pass O(N²) (1,000 repeaters require about 501,500 scans). This can stall the UI timer pass; capture the eligible
(key, occurrenceId)entries once per pass, or otherwise keep a cursor/index that does not repeatedly traverse deferred entries.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Align IApplication.RemoveTimeout with the ITimedEvents.Remove contract. A true result can mean cancellation was recorded for an executing callback, which continues running but cannot reschedule.
There was a problem hiding this comment.
🟡 Changes recommended
Repeat rescheduling still evaluates a virtual property while holding the timeout queue lock, permitting lock-inversion deadlocks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Read a repeating Timeout Span and the current time before reentering the timeout queue lock, then revalidate the occurrence generation and StopAll epoch before enqueueing. Preserve active-state cleanup if either extension point throws. Apply the same lock boundary to GetTimeout and all time-provider reads. Add deterministic regressions for blocking and throwing Span overrides plus blocking time providers.
There was a problem hiding this comment.
🟡 Changes recommended
RunTimers now unnecessarily reads potentially blocking user-provided time when no timeouts are queued.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The zero-delay repeater regression can hang indefinitely when the behavior it tests regresses.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Tests/UnitTestsParallelizable/Application/Timeouts/TimedEventsTests.cs:1530
- If the zero-delay repeater regresses to the previous unbounded pass, the runner retains the queue lock and this synchronous
Removeblocks forever, so the regression test hangs instead of failing within its timeout. Stop the loop by replacing the callback first, wait for the runner with the existing bound, and then clean up the queued occurrence.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Addressed the latest Copilot review summary in 8de9e59. The zero-delay pass-bound regression now uses an atomic stop signal before waiting for the runner, so a regression terminates and fails its bounded-return assertion instead of blocking in Remove. I applied the same hardening to the adjacent zero-delay peer-starvation regression and preserved its assertion that the repeating occurrence remains queued. TimedEventsTests passes 40/40 locally. |
Summary
IApplication.Invokecan enqueue work from other threads. BecauseInvokeis implemented asTimedEvents.Add (TimeSpan.Zero, …), every cross-threadInvoke,Post, andSendpreviously blocked for the full duration of every timer callback.RunTimerscall returns, a later pass handles remaining due timers, and callback exceptions propagate directly and end the current pass.Timeoutreference as one cancellation token:Removecancels all queued and active occurrences ordered before it without interrupting an executing callback. Removal generations and aStopAllepoch keep later additions unaffected without allocating per callback.RemoveandGetTimeout. Both previously usedSortedList.IndexOfValue, which returns only the first match, so removing aTimeoutthat had been queued more than once left the remaining occurrences scheduled and still firing.Removeduring execution was previously a silent no-op, and the callback rescheduled itself regardless.Timeouts, validateAdd(Timeout)inputs at the call site, and report the collision-adjusted queue key throughAdded.TimedEvents.Addedafter releasing the queue lock. A concurrent runner may execute a due timeout before itsAddedhandler runs.Timeout.Spangetters and injected time providers outside the queue lock. Repeating occurrences remain active during these reads and revalidate their removal generation andStopAllepoch before rescheduling. Empty timer passes return before reading the provider.<inheritdoc/>onCheckTimers, which is not declared onITimedEventsand therefore inherited nothing.IApplication.InvokeandMainLoopSyncContext.Post/Sendpaths introduced by Fixes #5636 - Scope MainLoopSyncContext to running sessions (await-before-RunAsync deadlock) #5641.Sendenqueues without the timeout-queue lock but still waits synchronously for main-loop execution by design.Addedhandler exception ends the timer pass while leaving the timeout scheduled.SkillView consumer reproduction
SkillView 0.3 exposed the lock cycle in a real nested-modal workflow. A timeout-delivered UI callback opened the cleanup modal, then the removal worker tried to report its terminal progress through
IApplication.Invoke. The captured stacks were:The worker had completed two removal validations with refusals, but it could not enqueue the completion callback while the outer timer callback retained the queue lock. The modal therefore stayed on
removing...; pressing Escape changed it tocanceling removal..., but cancellation could not finish and the rest of the UI remained unavailable.CrossThread_Invoke_Executes_During_Nested_Run_Started_From_Timeoutnow covers this combined condition and verifies that the queued callback executes while the nested dialog is active.Testing
CS0419inViewBase/View.Drawing.csandCS1574×2 inViews/DropDownList.cs. None are in files this PR touches, and the change introduces no new warnings.TimedEventsTests: 40 passedVerified behaviors
Beyond the assertions in the suite, these were confirmed with throwaway probes:
TimeSpan.Zerotimeout runs exactly once per pass. Before the pass bound, it re-ran indefinitely — measured at ~17.4M invocations in 3 seconds withRunTimersnever returning, which starves drawing, input, and shutdown.Addedsubscriber on the reschedule path leaves the active-state map empty and the queue and occurrence-ID map consistent; the timeout stays scheduled and later passes keep running it.Timeout.Spanoverrides and time-provider reads no longer block concurrent timeout removal or addition; cancellation during a repeat-interval read still prevents rescheduling.Review notes
Draft pending final review. The normal local GitVersion step hung in
GitVersion.MsBuildon one machine; the equivalent build withDisableGitVersionTask=truepassed, as does a plaindotnet build. Solution builds emit only the pre-existing compiler warnings listed above plus sandbox-related NuGet audit-cache warnings (NU1900).To pull down this PR locally:
git remote add copilot https://github.com/harder/Terminal.Gui.git
git fetch copilot fix/5652-timed-events-locking
git checkout copilot/fix/5652-timed-events-locking