From fe2dd3fe8709af0813ec5c4cc3297e1930f65cf9 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 18:29:36 +0100 Subject: [PATCH 01/12] dispatcher: fold the untyped job into the generic one The untyped DispatcherJob and the generic DispatcherJob differ only in that one has no result to hand back. Keeping both means every change to how a job runs - how it completes, how it fails, what context it runs in - has to be made and reviewed twice, then kept in step by hand. Express the untyped case as a job with a null result instead. Callers awaiting the untyped overload still get a plain Task, so nothing outside the dispatcher can tell the difference. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- src/Core/UI/Dispatcher.cs | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index 9dd9914382..7906b26ffe 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -72,10 +72,11 @@ public void Post(Action work) public Task InvokeAsync(Action work) { var tcs = new TaskCompletionSource(); - _queue.AddJob(new DispatcherJob(work, tcs)); + _queue.AddJob(new DispatcherJob(ct => { work(ct); return null; }, tcs)); return tcs.Task; } + /// public Task InvokeAsync(Func work) { var tcs = new TaskCompletionSource(); @@ -88,24 +89,6 @@ private interface IDispatcherJob void Execute(CancellationToken ct); } - private class DispatcherJob : IDispatcherJob - { - private readonly Action _work; - private readonly TaskCompletionSource _tcs; - - public DispatcherJob(Action work, TaskCompletionSource tcs) - { - _work = work; - _tcs = tcs; - } - - public void Execute(CancellationToken ct) - { - _work(ct); - _tcs?.SetResult(null); - } - } - private class DispatcherJob : IDispatcherJob { private readonly Func _work; From 9f71a279ed4d0a5c78ad98dba63dc64bc9fe4c9b Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 18:30:12 +0100 Subject: [PATCH 02/12] dispatcher: complete jobs that throw A dispatcher job that threw left its TaskCompletionSource uncompleted, so a caller awaiting InvokeAsync waited forever. The exception then unwound the queue loop and tore down the dispatcher thread with it, so no further main thread work could run either. Complete the task with the exception instead, so failures surface where the work was requested rather than on whichever loop happens to be pumping the dispatcher thread. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- src/Core/UI/Dispatcher.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index 7906b26ffe..90f5d283cf 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -102,8 +102,17 @@ public DispatcherJob(Func work, TaskCompletionSource public void Execute(CancellationToken ct) { - TResult result = _work(ct); - _tcs?.SetResult(result); + try + { + TResult result = _work(ct); + _tcs?.TrySetResult(result); + } + catch (Exception ex) when (_tcs is not null) + { + // Marshal the failure back to the caller rather than letting it escape + // on to whichever loop is currently pumping the dispatcher thread. + _tcs.TrySetException(ex); + } } } From d180821255fdb09ec456d969ac4b403061b214d1 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 15:28:02 +0100 Subject: [PATCH 03/12] dispatcher: run continuations off the job thread TaskCompletionSource runs its continuations synchronously by default, so a caller awaiting InvokeAsync resumes inline on the dispatcher thread, inside the loop that is meant to be draining the job queue. Whatever the caller does next - including blocking - delays every other job posted to the main thread. Ask for asynchronous continuations so the dispatcher thread returns to pumping as soon as the job itself is done. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- src/Core/UI/Dispatcher.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index 90f5d283cf..97745e920a 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -71,7 +71,7 @@ public void Post(Action work) /// Work to be run. public Task InvokeAsync(Action work) { - var tcs = new TaskCompletionSource(); + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); _queue.AddJob(new DispatcherJob(ct => { work(ct); return null; }, tcs)); return tcs.Task; } @@ -79,7 +79,7 @@ public Task InvokeAsync(Action work) /// public Task InvokeAsync(Func work) { - var tcs = new TaskCompletionSource(); + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); _queue.AddJob(new DispatcherJob(work, tcs)); return tcs.Task; } From f5d22be1f45d1a51850bdd14c6181323c29c49bb Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 16:28:07 +0100 Subject: [PATCH 04/12] dispatcher: add overloads for asynchronous work Passing an async lambda to InvokeAsync bound to the plain Func overload with T inferred as Task, so the returned task completed when the work first yielded rather than when it finished. The broker call site had to notice that and await twice to get the real result. Anyone who missed it got a task that completed early. Give task-returning work explicit overloads and jobs that own their final completion sources. The caller then observes the whole operation, including its original exceptions and cancellation information. The final task must also retain the asynchronous-continuation policy of synchronous jobs. Unwrapping an outer task does not carry that policy to the inner task or the proxy returned to the caller, so completing async work could otherwise run unrelated application code inline on the dispatcher thread. Keep completion handling with the job rather than constructing separate success and failure callbacks at each public entry point. Synchronous and asynchronous jobs share the execution and exception boundary, while each variant completes the caller's task only when its own work is done. Assisted-by: Claude Opus 5 Assisted-by: GPT-6 Astra Signed-off-by: Matthew John Cheetham --- src/Core.Tests/UI/DispatcherTests.cs | 231 ++++++++++++++++++ .../Entra/EntraAuthentication.PublicClient.cs | 6 +- src/Core/UI/Dispatcher.cs | 141 +++++++++-- 3 files changed, 354 insertions(+), 24 deletions(-) create mode 100644 src/Core.Tests/UI/DispatcherTests.cs diff --git a/src/Core.Tests/UI/DispatcherTests.cs b/src/Core.Tests/UI/DispatcherTests.cs new file mode 100644 index 0000000000..e4d5ac4c1c --- /dev/null +++ b/src/Core.Tests/UI/DispatcherTests.cs @@ -0,0 +1,231 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using GitCredentialManager.UI; +using Xunit; + +namespace GitCredentialManager.Tests.UI; + +public class DispatcherTests +{ + private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(10); + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Dispatcher_InvokeAsync_AsyncWork_DoesNotInlineCallerContinuations(bool returnsValue) + { + using var initialized = new ManualResetEventSlim(); + Thread thread = StartDispatcherThread(initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + // Deliberately allow the work's own task to run continuations inline. + var work = new TaskCompletionSource(); + Task task = returnsValue + ? dispatcher.InvokeAsync(_ => work.Task) + : dispatcher.InvokeAsync(_ => (Task)work.Task); + Assert.Equal(TaskCreationOptions.RunContinuationsAsynchronously, + task.CreationOptions & TaskCreationOptions.RunContinuationsAsynchronously); + Task continuation = task.ContinueWith( + _ => dispatcher.CheckAccess(), + CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); + + await dispatcher.InvokeAsync(_ => work.SetResult(42)).WaitAsync(Timeout); + + Assert.False(await continuation.WaitAsync(Timeout)); + } + finally + { + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Theory] + [InlineData(false, false, TaskStatus.RanToCompletion)] + [InlineData(false, true, TaskStatus.RanToCompletion)] + [InlineData(true, false, TaskStatus.RanToCompletion)] + [InlineData(true, true, TaskStatus.RanToCompletion)] + [InlineData(false, false, TaskStatus.Faulted)] + [InlineData(false, true, TaskStatus.Faulted)] + [InlineData(true, false, TaskStatus.Faulted)] + [InlineData(true, true, TaskStatus.Faulted)] + [InlineData(false, false, TaskStatus.Canceled)] + [InlineData(false, true, TaskStatus.Canceled)] + [InlineData(true, false, TaskStatus.Canceled)] + [InlineData(true, true, TaskStatus.Canceled)] + public async Task Dispatcher_InvokeAsync_AsyncWork_PreservesTaskOutcome( + bool returnsValue, bool alreadyCompleted, TaskStatus outcome) + { + using var initialized = new ManualResetEventSlim(); + using var cancellation = new CancellationTokenSource(); + cancellation.Cancel(); + + Thread thread = StartDispatcherThread(initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + var work = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + Exception[] failures = + { + new InvalidOperationException("First work failure."), + new ArgumentException("Second work failure."), + }; + + void CompleteWork() + { + switch (outcome) + { + case TaskStatus.RanToCompletion: + work.SetResult(42); + break; + case TaskStatus.Faulted: + work.SetException(failures); + break; + case TaskStatus.Canceled: + work.SetCanceled(cancellation.Token); + break; + default: + throw new ArgumentOutOfRangeException(nameof(outcome)); + } + } + + if (alreadyCompleted) + { + CompleteWork(); + } + + Task task = returnsValue + ? dispatcher.InvokeAsync(_ => work.Task) + : dispatcher.InvokeAsync(_ => (Task)work.Task); + + if (!alreadyCompleted) + { + Assert.False(task.IsCompleted); + CompleteWork(); + } + + switch (outcome) + { + case TaskStatus.RanToCompletion: + await task.WaitAsync(Timeout); + if (returnsValue) + { + Assert.Equal(42, await (Task)task); + } + break; + case TaskStatus.Faulted: + Assert.Same(failures[0], + await Assert.ThrowsAsync(() => task.WaitAsync(Timeout))); + Assert.Equal(failures, task.Exception.InnerExceptions); + break; + case TaskStatus.Canceled: + OperationCanceledException error = + await Assert.ThrowsAnyAsync(() => task.WaitAsync(Timeout)); + Assert.Equal(cancellation.Token, error.CancellationToken); + Assert.True(task.IsCanceled); + break; + } + } + finally + { + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Dispatcher_InvokeAsync_DelegateThrows_FaultsTaskWithoutFailingLoop(bool throwsCancellation) + { + using var initialized = new ManualResetEventSlim(); + Thread thread = StartDispatcherThread(initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + Exception failure = throwsCancellation + ? new OperationCanceledException() + : new InvalidOperationException("The delegate failed."); + void ActionWork(CancellationToken ct) => throw failure; + int ValueWork(CancellationToken ct) => throw failure; + Task AsyncWork(CancellationToken ct) => throw failure; + Task AsyncValueWork(CancellationToken ct) => throw failure; + + Task[] tasks = + { + dispatcher.InvokeAsync(ActionWork), + dispatcher.InvokeAsync(ValueWork), + dispatcher.InvokeAsync(AsyncWork), + dispatcher.InvokeAsync(AsyncValueWork), + }; + + foreach (Task task in tasks) + { + Assert.Same(failure, await Record.ExceptionAsync(() => task.WaitAsync(Timeout))); + Assert.True(task.IsFaulted); + } + + Assert.Equal(42, await dispatcher.InvokeAsync(_ => 42).WaitAsync(Timeout)); + } + finally + { + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Dispatcher_InvokeAsync_AsyncWorkReturnsNull_CancelsTask(bool returnsValue) + { + using var initialized = new ManualResetEventSlim(); + Thread thread = StartDispatcherThread(initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + Task Work(CancellationToken ct) => null; + Task task = returnsValue + ? dispatcher.InvokeAsync(Work) + : dispatcher.InvokeAsync(ct => (Task)Work(ct)); + + await Assert.ThrowsAnyAsync(() => task.WaitAsync(Timeout)); + Assert.True(task.IsCanceled); + } + finally + { + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + private static Thread StartDispatcherThread(ManualResetEventSlim initialized, Action beforeRun = null) + { + // The dispatcher binds to the thread that initializes it and must be run from + // that same thread, so both have to happen here rather than in the test. + var thread = new Thread(() => + { + Dispatcher.Initialize(); + initialized.Set(); + beforeRun?.Invoke(); + Dispatcher.MainThread.Run(); + }) + { + IsBackground = true, + Name = nameof(StartDispatcherThread), + }; + + thread.Start(); + return thread; + } +} diff --git a/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs b/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs index 87d8f74c61..009d2c31cd 100644 --- a/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs +++ b/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs @@ -261,12 +261,10 @@ await UseDefaultAccountAsync(result.Account.Username, ct)) if (isMainThreadRequired && !Dispatcher.MainThread.CheckAccess()) { Context.Trace.WriteLine("Dispatching interactive broker authentication to main thread..."); - Task mainThreadTask = await Dispatcher.MainThread.InvokeAsync(async _ => - await app.AcquireTokenInteractive(scopes) + return await Dispatcher.MainThread.InvokeAsync( + async _ => await app.AcquireTokenInteractive(scopes) .ExecuteAsync(ct) ); - - return await mainThreadTask; } // Run the auth on the current thread diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index 97745e920a..b2b1197db1 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -65,54 +65,155 @@ public void Post(Action work) } /// - /// Execute work to be run on the thread associated with this dispatcher and wait - /// synchronously until the work is complete. + /// Execute synchronous work on the thread associated with this dispatcher and + /// return a task that completes when the work is done. /// /// Work to be run. public Task InvokeAsync(Action work) { - var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - _queue.AddJob(new DispatcherJob(ct => { work(ct); return null; }, tcs)); - return tcs.Task; + var job = new DispatcherJob(ct => { work(ct); return null; }); + _queue.AddJob(job); + return job.Completion; } /// public Task InvokeAsync(Func work) { - var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - _queue.AddJob(new DispatcherJob(work, tcs)); - return tcs.Task; + var job = new DispatcherJob(work); + _queue.AddJob(job); + return job.Completion; + } + + /// + /// Execute asynchronous work on the thread associated with this dispatcher. + /// + /// Work to be run. + /// A task that completes when the work completes, not when it first yields. + public Task InvokeAsync(Func work) + { + var job = new AsyncDispatcherJob(work); + _queue.AddJob(job); + return job.Completion; + } + + /// + public Task InvokeAsync(Func> work) + { + var job = new AsyncDispatcherJob(work); + _queue.AddJob(job); + return job.Completion; } private interface IDispatcherJob { + Task Completion { get; } + void Execute(CancellationToken ct); + + void Fail(Exception ex); + } + + private abstract class DispatcherJob : IDispatcherJob + { + public abstract Task Completion { get; } + + public void Execute(CancellationToken ct) + { + try + { + ExecuteCore(ct); + } + catch (Exception ex) + { + // Marshal the failure back to the caller rather than letting it escape + // on to whichever loop is currently pumping the dispatcher thread. + Fail(ex); + } + } + + public abstract void Fail(Exception ex); + + protected abstract void ExecuteCore(CancellationToken ct); } - private class DispatcherJob : IDispatcherJob + private sealed class DispatcherJob : DispatcherJob { private readonly Func _work; - private readonly TaskCompletionSource _tcs; + private readonly TaskCompletionSource _tcs = + new(TaskCreationOptions.RunContinuationsAsynchronously); - public DispatcherJob(Func work, TaskCompletionSource tcs) + public override Task Completion => _tcs.Task; + + public DispatcherJob(Func work) { _work = work; - _tcs = tcs; } - public void Execute(CancellationToken ct) + protected override void ExecuteCore(CancellationToken ct) => _tcs.TrySetResult(_work(ct)); + + public override void Fail(Exception ex) => _tcs.TrySetException(ex); + } + + private sealed class AsyncDispatcherJob : DispatcherJob + { + private readonly Func _work; + private readonly TaskCompletionSource _tcs = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public override Task Completion => _tcs.Task; + + public AsyncDispatcherJob(Func work) { - try + _work = work; + } + + protected override void ExecuteCore(CancellationToken ct) => _ = CompleteAsync(_work(ct)); + + public override void Fail(Exception ex) => _tcs.TrySetException(ex); + + private async Task CompleteAsync(Task task) + { + if (task is null) { - TResult result = _work(ct); - _tcs?.TrySetResult(result); + // Preserve Unwrap's treatment of a missing inner task. + _tcs.TrySetCanceled(); + return; } - catch (Exception ex) when (_tcs is not null) + + // Observe completion without throwing, then forward the original outcome intact. + await task.ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); + _tcs.TrySetFromTask(task); + } + } + + private sealed class AsyncDispatcherJob : DispatcherJob + { + private readonly Func> _work; + private readonly TaskCompletionSource _tcs = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public override Task Completion => _tcs.Task; + + public AsyncDispatcherJob(Func> work) + { + _work = work; + } + + protected override void ExecuteCore(CancellationToken ct) => _ = CompleteAsync(_work(ct)); + + public override void Fail(Exception ex) => _tcs.TrySetException(ex); + + private async Task CompleteAsync(Task task) + { + if (task is null) { - // Marshal the failure back to the caller rather than letting it escape - // on to whichever loop is currently pumping the dispatcher thread. - _tcs.TrySetException(ex); + _tcs.TrySetCanceled(); + return; } + + // SuppressThrowing is only supported by the non-generic Task awaiter. + await ((Task)task).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); + _tcs.TrySetFromTask(task); } } From aee8739324bc5d3e0153adca27689cb0ac751d73 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 16:27:51 +0100 Subject: [PATCH 05/12] dispatcher: allow shutdown before Run is reached Program.Main starts the application thread and only then runs the dispatcher, so the application thread can reach shutdown before the main thread has reached Run. Shutdown treated that as misuse and threw, which would have surfaced as an unhandled exception on the application thread for an invocation that did nothing wrong - just one that finished unusually quickly. Accept it instead, and have Run return immediately when it finds the dispatcher already stopping. Neither thread has to win the race. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- src/Core.Tests/UI/DispatcherTests.cs | 30 ++++++++++++++++++++++++++++ src/Core/UI/Dispatcher.cs | 10 ++++++---- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/Core.Tests/UI/DispatcherTests.cs b/src/Core.Tests/UI/DispatcherTests.cs index e4d5ac4c1c..17a5852fc2 100644 --- a/src/Core.Tests/UI/DispatcherTests.cs +++ b/src/Core.Tests/UI/DispatcherTests.cs @@ -209,6 +209,36 @@ public async Task Dispatcher_InvokeAsync_AsyncWorkReturnsNull_CancelsTask(bool r } } + [Fact] + public void Dispatcher_Shutdown_BeforeRunIsReached_RunReturns() + { + var initialized = new ManualResetEventSlim(); + var mayRun = new ManualResetEventSlim(); + + // Hold the dispatcher thread between Initialize and Run so we shut down in the + // window that the application thread can genuinely hit in Program.Main. + Thread thread = StartDispatcherThread(initialized, () => mayRun.Wait(Timeout)); + Assert.True(initialized.Wait(Timeout)); + + Dispatcher.MainThread.Shutdown(); + mayRun.Set(); + + Assert.True(thread.Join(Timeout)); + } + + [Fact] + public void Dispatcher_Shutdown_NoWorkPosted_RunReturns() + { + var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(initialized); + Assert.True(initialized.Wait(Timeout)); + + Dispatcher.MainThread.Shutdown(); + + Assert.True(thread.Join(Timeout)); + } + private static Thread StartDispatcherThread(ManualResetEventSlim initialized, Action beforeRun = null) { // The dispatcher binds to the thread that initializes it and must be run from diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index b2b1197db1..94970b1f85 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -241,9 +241,9 @@ public void Run() case State.Started: throw new InvalidOperationException("Dispatcher has already started."); case State.Stopping: - throw new InvalidOperationException("Dispatcher is shutting down."); case State.Stopped: - throw new InvalidOperationException("Dispatcher has shut down."); + // Shut down before we got here, so there is nothing left to run. + return; } _state = State.Started; @@ -261,13 +261,15 @@ public void Shutdown() { switch (_state) { - case State.NotStarted: - throw new InvalidOperationException("Dispatcher is not running."); case State.Stopping: throw new InvalidOperationException("Dispatcher is already shutting down."); case State.Stopped: throw new InvalidOperationException("Dispatcher has already shut down."); } + + // Shutting down before Run() has been reached is legitimate: the + // application thread can finish before the main thread gets there. + // Run() sees this and returns without starting anything. _state = State.Stopping; _cts.Cancel(); Monitor.Pulse(_queue); From 559e0a515186ee1af4f058bf8d6044926c1977bf Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 18:31:22 +0100 Subject: [PATCH 06/12] dispatcher: distinguish stopping from stopped The state machine declared a Stopped state that nothing ever entered, so every branch handling it was unreachable and the dispatcher could not tell "shut down before Run was reached" from "Run has already returned". Those need to be told apart. Run tolerates the first because the application thread can legitimately finish before the main thread gets that far, but the second is a caller trying to reuse a dispatcher whose thread has already been released, and used to be accepted in silence. Enter Stopped when Run is about to return, and reject running again. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- src/Core.Tests/UI/DispatcherTests.cs | 30 ++++++++++++++++++++++++++++ src/Core/UI/Dispatcher.cs | 18 ++++++++++++++--- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/Core.Tests/UI/DispatcherTests.cs b/src/Core.Tests/UI/DispatcherTests.cs index 17a5852fc2..49463372ef 100644 --- a/src/Core.Tests/UI/DispatcherTests.cs +++ b/src/Core.Tests/UI/DispatcherTests.cs @@ -239,6 +239,36 @@ public void Dispatcher_Shutdown_NoWorkPosted_RunReturns() Assert.True(thread.Join(Timeout)); } + [Fact] + public void Dispatcher_Run_AfterRunHasReturned_Throws() + { + var initialized = new ManualResetEventSlim(); + Exception secondRun = null; + + // Run must be called from the dispatcher thread, so the second call has to be + // made there too rather than from the test thread. + var thread = new Thread(() => + { + Dispatcher.Initialize(); + initialized.Set(); + Dispatcher.MainThread.Run(); + secondRun = Record.Exception(() => Dispatcher.MainThread.Run()); + }) + { + IsBackground = true, + Name = nameof(Dispatcher_Run_AfterRunHasReturned_Throws), + }; + thread.Start(); + + Assert.True(initialized.Wait(Timeout)); + Dispatcher.MainThread.Shutdown(); + Assert.True(thread.Join(Timeout)); + + // Running again once the thread has been released is a programming error, and is + // distinct from the tolerated race where shutdown beats Run to the dispatcher. + Assert.IsType(secondRun); + } + private static Thread StartDispatcherThread(ManualResetEventSlim initialized, Action beforeRun = null) { // The dispatcher binds to the thread that initializes it and must be run from diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index 94970b1f85..f5d2850fec 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -240,18 +240,30 @@ public void Run() { case State.Started: throw new InvalidOperationException("Dispatcher has already started."); - case State.Stopping: case State.Stopped: + throw new InvalidOperationException("Dispatcher has shut down."); + case State.Stopping: // Shut down before we got here, so there is nothing left to run. + _state = State.Stopped; return; } _state = State.Started; } - while (TryTake(out IDispatcherJob job)) + try + { + while (TryTake(out IDispatcherJob job)) + { + job.Execute(_cts.Token); + } + } + finally { - job.Execute(_cts.Token); + lock (_queue) + { + _state = State.Stopped; + } } } From f5d51a69adcd625ce0e91d3f8fea7621a4f90c64 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 18:33:06 +0100 Subject: [PATCH 07/12] ui: start the main loop just-in-time Showing UI and using the macOS MSAL broker both need the process entry thread, for different reasons: macOS requires UI controls to be created there, and the broker requires a running NSApplication. Avalonia supplied the former by running its main loop as a dispatcher job, which never returns. Anything posted afterwards queued up behind it and never ran, so main thread work that followed showing a window deadlocked. MSAL makes the ordering matter a second time. It decides once per process whether it is a "console app" by testing whether NSApplication is running, and caches that answer for the lifetime of the process. Without NSApplication it requires the interactive broker call to run on managed thread 1 and then seizes that thread with its own polling loop, which cannot coexist with Avalonia's. With NSApplication running it requires neither. So whichever of the two ran first silently decided whether the second could work at all. Move the main loop into the dispatcher and start it lazily, on the first job posted. Needing the main thread now implies a running main loop: there is no escalation call for a caller to forget, and no ordering left to get wrong. Posting is what waits, rather than the work. A caller blocks until the main loop is up and then hands its own job straight to it, so the dispatcher never holds work that nothing is pumping, and every job is guaranteed to start with NSApplication already running. The first caller therefore pays to initialise Avalonia, which is fair enough - it is the one that asked for the main thread. Posting from the dispatcher thread before the loop exists is rejected rather than left to deadlock, since that thread is the only one that could release the wait. Invocations that need neither UI nor the broker still pay nothing: the thread parks until somebody asks for it, and shuts down again without ever initialising Avalonia. Shutting down abandons work that is still outstanding, which is worth being deliberate about. Draining instead would look tidier but deadlocks the obvious case: a window shown without anyone awaiting it stays open, so nothing would ever complete the work being waited for. The Avalonia bootstrap moves behind IMainLoop so that the dispatcher carries no UI framework dependency and the hand-over stays testable with a fake. The existing shutdown tests are moved on to that fake so they can also assert that the fast path never starts the loop, and tests are added for the hand-over, for posting from the wrong thread, and for abandoned work. One consequence of the move is that the avn_init trace region is now recorded on the main thread rather than on the calling thread, since initialisation no longer has a caller to attribute it to. Assisted-by: Claude Opus 5 Assisted-by: GPT-6 Astra Signed-off-by: Matthew John Cheetham --- src/Core.Tests/UI/DispatcherTests.cs | 883 +++++++++++++++++++++++++-- src/Core/UI/AvaloniaMainLoop.cs | 60 ++ src/Core/UI/AvaloniaUi.cs | 61 +- src/Core/UI/Dispatcher.cs | 279 +++++++-- src/Core/UI/IMainLoop.cs | 35 ++ 5 files changed, 1185 insertions(+), 133 deletions(-) create mode 100644 src/Core/UI/AvaloniaMainLoop.cs create mode 100644 src/Core/UI/IMainLoop.cs diff --git a/src/Core.Tests/UI/DispatcherTests.cs b/src/Core.Tests/UI/DispatcherTests.cs index 49463372ef..362723b487 100644 --- a/src/Core.Tests/UI/DispatcherTests.cs +++ b/src/Core.Tests/UI/DispatcherTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Threading; using System.Threading.Tasks; using GitCredentialManager.UI; @@ -10,13 +11,155 @@ public class DispatcherTests { private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(10); + [Fact] + public void Dispatcher_Shutdown_BeforeRunIsReached_RunReturnsWithoutStartingMainLoop() + { + var mainLoop = new FakeMainLoop(); + var initialized = new ManualResetEventSlim(); + var mayRun = new ManualResetEventSlim(); + + // Hold the dispatcher thread between Initialize and Run so we can shut down in + // the window that the application thread can genuinely hit in Program.Main. + Thread thread = StartDispatcherThread(mainLoop, initialized, () => mayRun.Wait(Timeout)); + Assert.True(initialized.Wait(Timeout)); + + Dispatcher.MainThread.Shutdown(); + mayRun.Set(); + + Assert.True(thread.Join(Timeout)); + Assert.False(mainLoop.Initialized); + Assert.False(mainLoop.Ran); + } + + [Fact] + public void Dispatcher_Run_AfterRunHasReturned_Throws() + { + var mainLoop = new FakeMainLoop(); + var initialized = new ManualResetEventSlim(); + Exception secondRun = null; + + // Run must be called from the dispatcher thread, so the second call has to be + // made there too rather than from the test thread. + var thread = new Thread(() => + { + Dispatcher.Initialize(mainLoop); + initialized.Set(); + Dispatcher.MainThread.Run(); + secondRun = Record.Exception(() => Dispatcher.MainThread.Run()); + }) + { + IsBackground = true, + Name = nameof(Dispatcher_Run_AfterRunHasReturned_Throws), + }; + thread.Start(); + + Assert.True(initialized.Wait(Timeout)); + Dispatcher.MainThread.Shutdown(); + Assert.True(thread.Join(Timeout)); + + // Running again once the thread has been released is a programming error, and is + // distinct from the tolerated race where shutdown beats Run to the dispatcher. + Assert.IsType(secondRun); + } + + [Fact] + public void Dispatcher_Shutdown_NoWorkPosted_NeverStartsMainLoop() + { + var mainLoop = new FakeMainLoop(); + var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + + Dispatcher.MainThread.Shutdown(); + + Assert.True(thread.Join(Timeout)); + Assert.False(mainLoop.Initialized); + Assert.False(mainLoop.Ran); + } + + [Fact] + public async Task Dispatcher_InvokeAsync_FirstJob_StartsMainLoopAndRunsWork() + { + var mainLoop = new FakeMainLoop(); + var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + Task task = dispatcher.InvokeAsync(_ => 42); + + Assert.Equal(42, await task.WaitAsync(Timeout)); + Assert.True(mainLoop.Initialized); + Assert.True(mainLoop.Ran); + + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + + [Fact] + public async Task Dispatcher_InvokeAsync_BeforeRunIsReached_PreservesWorkRequest() + { + var mainLoop = new FakeMainLoop(); + using var initialized = new ManualResetEventSlim(); + using var mayRun = new ManualResetEventSlim(); + using var posting = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized, () => mayRun.Wait()); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + Thread postingThread = null; + bool workRan = false; + Task submission = Submit(() => + { + postingThread = Thread.CurrentThread; + posting.Set(); + return dispatcher.InvokeAsync(_ => { workRan = true; }); + }); + + Assert.True(posting.Wait(Timeout)); + + // Let the caller block in AddJob before Run can reach its wait, so the + // work request has to survive being signalled before anybody waits on it. + Assert.True(SpinWait.SpinUntil( + () => (postingThread.ThreadState & ThreadState.WaitSleepJoin) != 0, Timeout)); + Assert.False(submission.IsCompleted); + Assert.False(mainLoop.Initialized); + + mayRun.Set(); + await (await submission.WaitAsync(Timeout)).WaitAsync(Timeout); + + Assert.True(workRan); + Assert.True(mainLoop.Ran); + } + finally + { + mayRun.Set(); + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + [Theory] [InlineData(false)] [InlineData(true)] public async Task Dispatcher_InvokeAsync_AsyncWork_DoesNotInlineCallerContinuations(bool returnsValue) { + using var callbackReturned = new ManualResetEventSlim(); + var mainLoop = new FakeMainLoop + { + AfterWork = () => callbackReturned.Set(), + // Make the outer task complete before InvokeAsync returns. Unwrap can + // then return the inner task directly, losing the outer task's options. + AfterPost = () => Assert.True(callbackReturned.Wait(Timeout)), + }; using var initialized = new ManualResetEventSlim(); - Thread thread = StartDispatcherThread(initialized); + + Thread thread = StartDispatcherThread(mainLoop, initialized); Assert.True(initialized.Wait(Timeout)); Dispatcher dispatcher = Dispatcher.MainThread; @@ -27,8 +170,6 @@ public async Task Dispatcher_InvokeAsync_AsyncWork_DoesNotInlineCallerContinuati Task task = returnsValue ? dispatcher.InvokeAsync(_ => work.Task) : dispatcher.InvokeAsync(_ => (Task)work.Task); - Assert.Equal(TaskCreationOptions.RunContinuationsAsynchronously, - task.CreationOptions & TaskCreationOptions.RunContinuationsAsynchronously); Task continuation = task.ContinueWith( _ => dispatcher.CheckAccess(), CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); @@ -60,11 +201,13 @@ public async Task Dispatcher_InvokeAsync_AsyncWork_DoesNotInlineCallerContinuati public async Task Dispatcher_InvokeAsync_AsyncWork_PreservesTaskOutcome( bool returnsValue, bool alreadyCompleted, TaskStatus outcome) { + using var callbackReturned = new ManualResetEventSlim(); + var mainLoop = new FakeMainLoop { AfterWork = () => callbackReturned.Set() }; using var initialized = new ManualResetEventSlim(); using var cancellation = new CancellationTokenSource(); cancellation.Cancel(); - Thread thread = StartDispatcherThread(initialized); + Thread thread = StartDispatcherThread(mainLoop, initialized); Assert.True(initialized.Wait(Timeout)); Dispatcher dispatcher = Dispatcher.MainThread; @@ -103,6 +246,7 @@ void CompleteWork() Task task = returnsValue ? dispatcher.InvokeAsync(_ => work.Task) : dispatcher.InvokeAsync(_ => (Task)work.Task); + Assert.True(callbackReturned.Wait(Timeout)); if (!alreadyCompleted) { @@ -144,8 +288,10 @@ void CompleteWork() [InlineData(true)] public async Task Dispatcher_InvokeAsync_DelegateThrows_FaultsTaskWithoutFailingLoop(bool throwsCancellation) { + var mainLoop = new FakeMainLoop(); using var initialized = new ManualResetEventSlim(); - Thread thread = StartDispatcherThread(initialized); + + Thread thread = StartDispatcherThread(mainLoop, initialized); Assert.True(initialized.Wait(Timeout)); Dispatcher dispatcher = Dispatcher.MainThread; @@ -187,8 +333,10 @@ public async Task Dispatcher_InvokeAsync_DelegateThrows_FaultsTaskWithoutFailing [InlineData(true)] public async Task Dispatcher_InvokeAsync_AsyncWorkReturnsNull_CancelsTask(bool returnsValue) { + var mainLoop = new FakeMainLoop(); using var initialized = new ManualResetEventSlim(); - Thread thread = StartDispatcherThread(initialized); + + Thread thread = StartDispatcherThread(mainLoop, initialized); Assert.True(initialized.Wait(Timeout)); Dispatcher dispatcher = Dispatcher.MainThread; @@ -210,72 +358,660 @@ public async Task Dispatcher_InvokeAsync_AsyncWorkReturnsNull_CancelsTask(bool r } [Fact] - public void Dispatcher_Shutdown_BeforeRunIsReached_RunReturns() + public async Task Dispatcher_InvokeAsync_FirstJob_BlocksUntilMainLoopIsInitialized() { - var initialized = new ManualResetEventSlim(); - var mayRun = new ManualResetEventSlim(); + using var mayInitialize = new ManualResetEventSlim(); + var mainLoop = new FakeMainLoop + { + BeforeInitialize = () => Assert.True(mayInitialize.Wait(Timeout)), + }; + using var initialized = new ManualResetEventSlim(); - // Hold the dispatcher thread between Initialize and Run so we shut down in the - // window that the application thread can genuinely hit in Program.Main. - Thread thread = StartDispatcherThread(initialized, () => mayRun.Wait(Timeout)); + Thread thread = StartDispatcherThread(mainLoop, initialized); Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; - Dispatcher.MainThread.Shutdown(); - mayRun.Set(); + Task submission = Submit(() => dispatcher.InvokeAsync(_ => { })); + // Callers hand their own work to the main loop so that it runs with their ambient + // state, which means they cannot return until the loop is there to take it. + Task delay = Task.Delay(TimeSpan.FromMilliseconds(250)); + Assert.Same(delay, await Task.WhenAny(submission, delay)); + + mayInitialize.Set(); + await (await submission.WaitAsync(Timeout)).WaitAsync(Timeout); + + dispatcher.Shutdown(); Assert.True(thread.Join(Timeout)); } [Fact] - public void Dispatcher_Shutdown_NoWorkPosted_RunReturns() + public async Task Dispatcher_InvokeAsync_BeforeMainLoopPumps_AcceptsButDoesNotRunWork() { - var initialized = new ManualResetEventSlim(); + using var reachedRun = new ManualResetEventSlim(); + using var mayPump = new ManualResetEventSlim(); + var mainLoop = new FakeMainLoop + { + BeforeRun = () => + { + reachedRun.Set(); + Assert.True(mayPump.Wait(Timeout)); + }, + }; + using var initialized = new ManualResetEventSlim(); - Thread thread = StartDispatcherThread(initialized); + Thread thread = StartDispatcherThread(mainLoop, initialized); Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + bool workRan = false; + Task task = await Submit(() => dispatcher.InvokeAsync(_ => { workRan = true; })).WaitAsync(Timeout); + Assert.True(reachedRun.Wait(Timeout)); + Assert.True(mainLoop.Initialized); + Assert.False(task.IsCompleted); + Assert.False(workRan); + + mayPump.Set(); + await task.WaitAsync(Timeout); + Assert.True(workRan); + } + finally + { + mayPump.Set(); + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Fact] + public async Task Dispatcher_InvokeAsync_ConcurrentFirstJobs_ReleasesAllCallers() + { + using var initializing = new ManualResetEventSlim(); + using var mayInitialize = new ManualResetEventSlim(); + int postCount = 0; + var mainLoop = new FakeMainLoop + { + BeforeInitialize = () => + { + initializing.Set(); + Assert.True(mayInitialize.Wait(Timeout)); + }, + BeforePost = () => Interlocked.Increment(ref postCount), + }; + using var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + int workCount = 0; + Task[] submissions = + { + Submit(() => dispatcher.InvokeAsync(_ => { Interlocked.Increment(ref workCount); })), + Submit(() => dispatcher.InvokeAsync(_ => { Interlocked.Increment(ref workCount); return 42; })), + Submit(() => dispatcher.InvokeAsync(_ => { Interlocked.Increment(ref workCount); return Task.CompletedTask; })), + Submit(() => dispatcher.InvokeAsync(_ => { Interlocked.Increment(ref workCount); return Task.FromResult(42); })), + }; + + Assert.True(initializing.Wait(Timeout)); + Assert.All(submissions, submission => Assert.False(submission.IsCompleted)); + Assert.Equal(0, Volatile.Read(ref postCount)); + + mayInitialize.Set(); + foreach (Task submission in submissions) + { + await (await submission.WaitAsync(Timeout)).WaitAsync(Timeout); + } + + Assert.Equal(4, workCount); + Assert.Equal(4, postCount); + + // The open gate also admits late callers and posts from the dispatcher + // thread itself, without another signal or a reset. + Task subsequent = await Submit(() => dispatcher.InvokeAsync(_ => + dispatcher.InvokeAsync(_ => { Interlocked.Increment(ref workCount); }))).WaitAsync(Timeout); + await subsequent.WaitAsync(Timeout); + + Assert.Equal(5, workCount); + Assert.Equal(6, postCount); + } + finally + { + mayInitialize.Set(); + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Fact] + public void Dispatcher_InvokeAsync_FromDispatcherThreadBeforeMainLoop_Throws() + { + var mainLoop = new FakeMainLoop(); + using var initialized = new ManualResetEventSlim(); + using var posted = new ManualResetEventSlim(); + Exception error = null; + + // Waiting here would be waiting on the only thread that can start the main loop. + Thread thread = StartDispatcherThread(mainLoop, initialized, () => + { + error = Record.Exception(() => Dispatcher.MainThread.Post(_ => { })); + posted.Set(); + }); + + Assert.True(initialized.Wait(Timeout)); + Assert.True(posted.Wait(Timeout)); + Assert.IsType(error); Dispatcher.MainThread.Shutdown(); + Assert.True(thread.Join(Timeout)); + Assert.False(mainLoop.Initialized); + } + + [Fact] + public async Task Dispatcher_Shutdown_MainLoopRunning_StopsItByCancellation() + { + var mainLoop = new FakeMainLoop(); + var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + await dispatcher.InvokeAsync(_ => { }).WaitAsync(Timeout); + dispatcher.Shutdown(); + + // Cancelling the token handed to Run is the only shutdown signal the loop gets. Assert.True(thread.Join(Timeout)); + Assert.True(mainLoop.RunWasCancelled); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Dispatcher_Shutdown_WhileInitializing_ReleasesWaitingCallers(bool failInitialize) + { + using var initializing = new ManualResetEventSlim(); + using var mayInitialize = new ManualResetEventSlim(); + int postCount = 0; + var mainLoop = new FakeMainLoop + { + BeforeInitialize = () => + { + initializing.Set(); + Assert.True(mayInitialize.Wait(Timeout)); + if (failInitialize) + { + throw new InvalidOperationException("Initialization failed after shutdown."); + } + }, + BeforePost = () => Interlocked.Increment(ref postCount), + }; + using var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + bool shutdownRequested = false; + + try + { + Task[] submissions = + { + Submit(() => dispatcher.InvokeAsync(_ => { })), + Submit(() => dispatcher.InvokeAsync(_ => 42)), + Submit(() => dispatcher.InvokeAsync(_ => Task.CompletedTask)), + Submit(() => dispatcher.InvokeAsync(_ => Task.FromResult(42))), + }; + + Assert.True(initializing.Wait(Timeout)); + dispatcher.Shutdown(); + shutdownRequested = true; + + // Shutdown must release callers before Initialize returns, without letting + // any of their work reach a loop that is not ready to accept it. + foreach (Task submission in submissions) + { + InvalidOperationException error = await Assert.ThrowsAsync( + () => submission.WaitAsync(Timeout)); + Assert.Equal("Dispatcher is shutting down.", error.Message); + } + + Assert.False(mainLoop.Initialized); + Assert.False(mainLoop.Ran); + Assert.Equal(0, Volatile.Read(ref postCount)); + + mayInitialize.Set(); + Assert.True(thread.Join(Timeout)); + Assert.Equal(!failInitialize, mainLoop.RunWasCancelled); + + // A failure after shutdown must not wait for another shutdown signal. + InvalidOperationException stopped = await Assert.ThrowsAsync( + () => Submit(() => dispatcher.InvokeAsync(_ => { })).WaitAsync(Timeout)); + Assert.Equal("Dispatcher has shut down.", stopped.Message); + Assert.Equal(0, postCount); + } + finally + { + if (!shutdownRequested) + { + dispatcher.Shutdown(); + } + + mayInitialize.Set(); + Assert.True(thread.Join(Timeout)); + } } [Fact] - public void Dispatcher_Run_AfterRunHasReturned_Throws() + public void Dispatcher_Shutdown_WorkStillPending_AbandonsIt() { + var mainLoop = new FakeMainLoop { PumpPostedWork = false }; var initialized = new ManualResetEventSlim(); - Exception secondRun = null; - // Run must be called from the dispatcher thread, so the second call has to be - // made there too rather than from the test thread. - var thread = new Thread(() => + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + Task task = dispatcher.InvokeAsync(_ => { }); + Assert.True(SpinWait.SpinUntil(() => mainLoop.Ran, Timeout)); + + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + + // Outstanding work is dropped rather than drained. Draining instead would hang + // shutdown whenever a window is left open with nobody awaiting it. + Assert.False(task.IsCompleted); + } + + [Fact] + public void Dispatcher_Shutdown_AfterAsyncWorkYields_AbandonsIt() + { + using var callbackReturned = new ManualResetEventSlim(); + var mainLoop = new FakeMainLoop { AfterWork = () => callbackReturned.Set() }; + using var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + Task task; + + try { - Dispatcher.Initialize(); - initialized.Set(); - Dispatcher.MainThread.Run(); - secondRun = Record.Exception(() => Dispatcher.MainThread.Run()); - }) + var pending = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + task = dispatcher.InvokeAsync(async _ => { await pending.Task; }); + Assert.True(callbackReturned.Wait(Timeout)); + Assert.False(task.IsCompleted); + } + finally { - IsBackground = true, - Name = nameof(Dispatcher_Run_AfterRunHasReturned_Throws), + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + + Assert.False(task.IsCompleted); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task Dispatcher_InvokeAsync_MainLoopFails_FaultsPendingAndFutureJobs(bool failInitialize) + { + var failure = new InvalidOperationException("The main loop failed."); + using var reachedFailure = new ManualResetEventSlim(); + using var mayFail = new ManualResetEventSlim(); + Action fail = () => + { + reachedFailure.Set(); + Assert.True(mayFail.Wait(Timeout)); + throw failure; }; - thread.Start(); + var mainLoop = new FakeMainLoop + { + BeforeInitialize = failInitialize ? fail : null, + BeforeRun = failInitialize ? null : fail, + }; + using var initialized = new ManualResetEventSlim(); + Thread thread = StartDispatcherThread(mainLoop, initialized); Assert.True(initialized.Wait(Timeout)); - Dispatcher.MainThread.Shutdown(); - Assert.True(thread.Join(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; - // Running again once the thread has been released is a programming error, and is - // distinct from the tolerated race where shutdown beats Run to the dispatcher. - Assert.IsType(secondRun); + try + { + bool workRan = false; + + // Submitting blocks until the main loop is running, so it cannot be done from + // the thread that has to release it. + Task[] submissions = + { + Submit(() => dispatcher.InvokeAsync(_ => { workRan = true; })), + Submit(() => dispatcher.InvokeAsync(_ => { workRan = true; return 42; })), + Submit(() => dispatcher.InvokeAsync(_ => { workRan = true; return Task.CompletedTask; })), + Submit(() => dispatcher.InvokeAsync(_ => { workRan = true; return Task.FromResult(42); })), + }; + + // Getting as far as the failure proves the dispatcher thread has seen the + // work, so at least one caller is waiting on the loop it is about to fail. + Assert.True(reachedFailure.Wait(Timeout)); + mayFail.Set(); + + foreach (Task submission in submissions) + { + Task task = await submission.WaitAsync(Timeout); + Assert.Same(failure, + await Assert.ThrowsAsync(() => task.WaitAsync(Timeout))); + } + + Task future = dispatcher.InvokeAsync(_ => { workRan = true; }); + Assert.Same(failure, + await Assert.ThrowsAsync(() => future.WaitAsync(Timeout))); + Assert.False(workRan); + } + finally + { + mayFail.Set(); + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } } - private static Thread StartDispatcherThread(ManualResetEventSlim initialized, Action beforeRun = null) + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public async Task Dispatcher_InvokeAsync_FailureAfterAsyncWorkYields_FaultsRunningJob( + bool returnsValue, bool failPost) + { + var failure = new InvalidOperationException("The main loop failed after work yielded."); + using var callbackReturned = new ManualResetEventSlim(); + using var mayFail = new ManualResetEventSlim(); + var resumeWork = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var workResumed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + int postCount = 0; + var mainLoop = new FakeMainLoop + { + BeforePost = () => + { + if (Interlocked.Increment(ref postCount) == 2 && failPost) + { + throw failure; + } + }, + AfterWork = () => + { + callbackReturned.Set(); + if (!failPost) + { + Assert.True(mayFail.Wait(Timeout)); + throw failure; + } + }, + }; + using var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + async Task Work(CancellationToken ct) + { + await resumeWork.Task; + Assert.True(dispatcher.CheckAccess()); + workResumed.SetResult(); + return 42; + } + + Task task = returnsValue + ? dispatcher.InvokeAsync(Work) + : dispatcher.InvokeAsync(ct => (Task)Work(ct)); + Assert.True(callbackReturned.Wait(Timeout)); + Assert.False(task.IsCompleted); + + if (failPost) + { + Task rejected = dispatcher.InvokeAsync(_ => { }); + Assert.Same(failure, + await Assert.ThrowsAsync(() => rejected.WaitAsync(Timeout))); + } + else + { + mayFail.Set(); + } + + Assert.Same(failure, + await Assert.ThrowsAsync(() => task.WaitAsync(Timeout))); + + Task future = dispatcher.InvokeAsync(_ => Task.CompletedTask); + Assert.Same(failure, + await Assert.ThrowsAsync(() => future.WaitAsync(Timeout))); + + if (failPost) + { + // A posting failure leaves this fake loop pumping. Finishing the + // already-running operation must not overwrite its reported failure. + resumeWork.SetResult(); + await workResumed.Task.WaitAsync(Timeout); + Assert.Same(failure, + await Assert.ThrowsAsync(() => task.WaitAsync(Timeout))); + } + } + finally + { + mayFail.Set(); + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Fact] + public async Task Dispatcher_InvokeAsync_PostFails_FaultsPostedAndFutureJobs() + { + var failure = new InvalidOperationException("Posting to the main loop failed."); + int postCount = 0; + var mainLoop = new FakeMainLoop + { + // Nothing may run: the jobs have to fault rather than complete. + PumpPostedWork = false, + BeforePost = () => + { + if (Interlocked.Increment(ref postCount) == 2) + { + throw failure; + } + }, + }; + using var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + bool workRan = false; + + // Submit one at a time so the failing post is reliably the second, covering + // work already posted, the rejected post, and work submitted afterwards. + Task posted = await Submit(() => dispatcher.InvokeAsync(_ => { workRan = true; })).WaitAsync(Timeout); + Task rejected = await Submit(() => dispatcher.InvokeAsync(_ => { workRan = true; })).WaitAsync(Timeout); + Task future = await Submit(() => dispatcher.InvokeAsync(_ => { workRan = true; })).WaitAsync(Timeout); + + foreach (Task task in new[] { posted, rejected, future }) + { + Assert.Same(failure, + await Assert.ThrowsAsync(() => task.WaitAsync(Timeout))); + } + + Assert.False(workRan); + Assert.Equal(2, postCount); + } + finally + { + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Fact] + public async Task Dispatcher_InvokeAsync_PostFailsWhileRunning_SkipsFaultedCallbacks() + { + var failure = new InvalidOperationException("Posting to the running main loop failed."); + using var initialized = new ManualResetEventSlim(); + using var running = new ManualResetEventSlim(); + using var mayPump = new ManualResetEventSlim(); + using var callbackPumped = new ManualResetEventSlim(); + int postCount = 0; + var mainLoop = new FakeMainLoop + { + BeforePost = () => + { + if (Interlocked.Increment(ref postCount) == 2) + { + throw failure; + } + }, + BeforeRun = () => + { + running.Set(); + Assert.True(mayPump.Wait(Timeout)); + }, + AfterWork = () => callbackPumped.Set(), + }; + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + bool workRan = false; + Task pending = dispatcher.InvokeAsync(_ => { workRan = true; }); + Assert.True(running.Wait(Timeout)); + + // Bound the submission itself: a posting caller must not wait for shutdown + // or throw synchronously instead of returning the faulted task. + Task submission = Task.Factory.StartNew( + () => dispatcher.InvokeAsync(_ => { workRan = true; }), + CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); + Task rejected = await submission.WaitAsync(Timeout); + + Assert.Same(failure, + await Assert.ThrowsAsync(() => pending.WaitAsync(Timeout))); + Assert.Same(failure, + await Assert.ThrowsAsync(() => rejected.WaitAsync(Timeout))); + + Task future = dispatcher.InvokeAsync(_ => { workRan = true; }); + Assert.Same(failure, + await Assert.ThrowsAsync(() => future.WaitAsync(Timeout))); + + mayPump.Set(); + Assert.True(callbackPumped.Wait(Timeout)); + Assert.False(workRan); + } + finally + { + mayPump.Set(); + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Fact] + public async Task Dispatcher_InvokeAsync_ConcurrentPostsFail_PreservesFirstFailure() + { + var firstFailure = new InvalidOperationException("The first post failed."); + var secondFailure = new InvalidOperationException("The second post failed."); + using var initialized = new ManualResetEventSlim(); + using var running = new ManualResetEventSlim(); + using var firstPosting = new ManualResetEventSlim(); + using var secondPosting = new ManualResetEventSlim(); + using var mayFailFirst = new ManualResetEventSlim(); + using var mayFailSecond = new ManualResetEventSlim(); + int postCount = 0; + var mainLoop = new FakeMainLoop + { + PumpPostedWork = false, + BeforeRun = () => running.Set(), + BeforePost = () => + { + switch (Interlocked.Increment(ref postCount)) + { + case 2: + firstPosting.Set(); + Assert.True(mayFailFirst.Wait(Timeout)); + throw firstFailure; + case 3: + secondPosting.Set(); + Assert.True(mayFailSecond.Wait(Timeout)); + throw secondFailure; + } + }, + }; + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + Task pending = dispatcher.InvokeAsync(_ => { }); + Assert.True(running.Wait(Timeout)); + + Task firstSubmission = Task.Factory.StartNew( + () => dispatcher.InvokeAsync(_ => { }), + CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); + Assert.True(firstPosting.Wait(Timeout)); + + Task secondSubmission = Task.Factory.StartNew( + () => dispatcher.InvokeAsync(_ => { }), + CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); + Assert.True(secondPosting.Wait(Timeout)); + + mayFailFirst.Set(); + Task first = await firstSubmission.WaitAsync(Timeout); + Assert.Same(firstFailure, + await Assert.ThrowsAsync(() => pending.WaitAsync(Timeout))); + Assert.Same(firstFailure, + await Assert.ThrowsAsync(() => first.WaitAsync(Timeout))); + + mayFailSecond.Set(); + Task second = await secondSubmission.WaitAsync(Timeout); + Assert.Same(firstFailure, + await Assert.ThrowsAsync(() => second.WaitAsync(Timeout))); + + Task future = dispatcher.InvokeAsync(_ => { }); + Assert.Same(firstFailure, + await Assert.ThrowsAsync(() => future.WaitAsync(Timeout))); + } + finally + { + mayFailFirst.Set(); + mayFailSecond.Set(); + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + private static Task Submit(Func submission) => + Task.Factory.StartNew( + submission, CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); + + private static Thread StartDispatcherThread( + IMainLoop mainLoop, ManualResetEventSlim initialized, Action beforeRun = null) { // The dispatcher binds to the thread that initializes it and must be run from // that same thread, so both have to happen here rather than in the test. var thread = new Thread(() => { - Dispatcher.Initialize(); + Dispatcher.Initialize(mainLoop); initialized.Set(); beforeRun?.Invoke(); Dispatcher.MainThread.Run(); @@ -288,4 +1024,77 @@ private static Thread StartDispatcherThread(ManualResetEventSlim initialized, Ac thread.Start(); return thread; } + + private sealed class FakeMainLoop : IMainLoop + { + private readonly BlockingCollection _work = new(); + + public Action BeforeInitialize { get; init; } + public Action BeforePost { get; init; } + public Action AfterPost { get; init; } + public Action BeforeRun { get; init; } + public Action AfterWork { get; init; } + + public bool Initialized { get; private set; } + public bool Ran { get; private set; } + public bool RunWasCancelled { get; private set; } + + /// + /// False to accept posted work but never run it, modelling a loop that is shut + /// down while work is still outstanding. + /// + public bool PumpPostedWork { get; init; } = true; + + public void Initialize() + { + BeforeInitialize?.Invoke(); + Initialized = true; + } + + public void Post(Action work) + { + Assert.True(Initialized); + BeforePost?.Invoke(); + _work.Add(work); + AfterPost?.Invoke(); + } + + public void Run(CancellationToken ct) + { + Ran = true; + BeforeRun?.Invoke(); + SynchronizationContext previous = SynchronizationContext.Current; + SynchronizationContext.SetSynchronizationContext(new MainLoopSynchronizationContext(this)); + try + { + if (PumpPostedWork) + { + foreach (Action work in _work.GetConsumingEnumerable(ct)) + { + work(); + AfterWork?.Invoke(); + } + } + else + { + ct.WaitHandle.WaitOne(); + ct.ThrowIfCancellationRequested(); + } + } + catch (OperationCanceledException) + { + RunWasCancelled = true; + } + finally + { + SynchronizationContext.SetSynchronizationContext(previous); + } + } + + private sealed class MainLoopSynchronizationContext(FakeMainLoop mainLoop) : SynchronizationContext + { + public override void Post(SendOrPostCallback callback, object state) => + mainLoop.Post(() => callback(state)); + } + } } diff --git a/src/Core/UI/AvaloniaMainLoop.cs b/src/Core/UI/AvaloniaMainLoop.cs new file mode 100644 index 0000000000..4ec3e1e275 --- /dev/null +++ b/src/Core/UI/AvaloniaMainLoop.cs @@ -0,0 +1,60 @@ +using System; +using System.Threading; +using Avalonia; +using Avalonia.Threading; +using AvnDispatcher = Avalonia.Threading.Dispatcher; + +namespace GitCredentialManager.UI +{ + /// + /// The Avalonia application main loop. + /// + internal class AvaloniaMainLoop : IMainLoop + { + private static bool _win32SoftwareRendering; + private static bool _isStarted; + + /// + /// Configure the Avalonia application. + /// + /// True to enable software rendering on Windows, false otherwise. + /// The application has already been started. + public static void Configure(bool win32SoftwareRendering) + { + if (_isStarted) + { + throw new InvalidOperationException( + "Avalonia must be configured before the application is started."); + } + + _win32SoftwareRendering = win32SoftwareRendering; + } + + public void Initialize() + { + _isStarted = true; + + using (Trace2.StartRegion("ui", "avn_init")) + { + AppBuilder appBuilder = AppBuilder.Configure(); + + // Set custom rendering options and modes if required + if (PlatformUtils.IsWindows() && _win32SoftwareRendering) + { + Trace2.WriteData("ui", "win32/software_rendering", "true"); + appBuilder.With(new Win32PlatformOptions + { RenderingMode = new[] { Win32RenderingMode.Software } }); + } + + appBuilder + .UsePlatformDetect() + .LogToTrace() + .SetupWithoutStarting(); + } + } + + public void Post(Action work) => AvnDispatcher.UIThread.Post(work, DispatcherPriority.Send); + + public void Run(CancellationToken ct) => AvnDispatcher.UIThread.MainLoop(ct); + } +} diff --git a/src/Core/UI/AvaloniaUi.cs b/src/Core/UI/AvaloniaUi.cs index 361780f051..bdf88421cb 100644 --- a/src/Core/UI/AvaloniaUi.cs +++ b/src/Core/UI/AvaloniaUi.cs @@ -3,7 +3,6 @@ using System.Threading.Tasks; using Avalonia; using Avalonia.Controls; -using Avalonia.Threading; using GitCredentialManager.Interop.Windows.Native; using GitCredentialManager.UI.Controls; using GitCredentialManager.UI.ViewModels; @@ -13,9 +12,6 @@ namespace GitCredentialManager.UI { public static class AvaloniaUi { - private static bool _isAppStarted; - private static bool _win32SoftwareRendering; - /// /// Configure the Avalonia application. /// @@ -25,12 +21,7 @@ public static class AvaloniaUi /// public static void Initialize(bool win32SoftwareRendering) { - if (_isAppStarted) - { - throw new InvalidOperationException("Setup must be called before the Avalonia application is started."); - } - - _win32SoftwareRendering = win32SoftwareRendering; + AvaloniaMainLoop.Configure(win32SoftwareRendering); } public static Task ShowViewAsync(Func viewFunc, WindowViewModel viewModel, IntPtr parentHandle, CancellationToken ct) => @@ -53,51 +44,11 @@ public static Task ShowWindowAsync(object dataContext, IntPtr parentHan public static Task ShowWindowAsync(Func windowFunc, object dataContext, IntPtr parentHandle, CancellationToken ct) { - if (!_isAppStarted) - { - _isAppStarted = true; - - var appInitialized = new ManualResetEventSlim(); - - // Keep the trace region to outside the dispatcher's lambda so we can attribute the - // UI init cost to the caller's thread, rather than the main thread. - using (Trace2.StartRegion("ui", "avn_init")) - { - // Fire and forget the Avalonia app main loop over to our dispatcher (running on the main/entry thread). - // This action only returns on our dispatcher shutdown. - Dispatcher.MainThread.Post(appCancelToken => - { - var appBuilder = AppBuilder.Configure(); - - // Set custom rendering options and modes if required - if (PlatformUtils.IsWindows() && _win32SoftwareRendering) - { - Trace2.WriteData("ui", "win32/software_rendering", "true"); - appBuilder.With(new Win32PlatformOptions - { RenderingMode = new[] { Win32RenderingMode.Software } }); - } - - appBuilder - .UsePlatformDetect() - .LogToTrace() - .SetupWithoutStarting(); - - appInitialized.Set(); - - // Run the application loop (only exit when the dispatcher is shutting down) - AvnDispatcher.UIThread.MainLoop(appCancelToken); - }); - - // Wait for the action posted above to be dequeued from the dispatcher's job queue - // and for the Avalonia framework (and their dispatcher) to be initialized. - appInitialized.Wait(); - } - } - - // Post the window action to the Avalonia dispatcher (which should be running) - return AvnDispatcher.UIThread.InvokeAsync( - () => ShowWindowInternal(windowFunc, dataContext, parentHandle, ct), - DispatcherPriority.Send + // The dispatcher owns the main thread and starts the Avalonia application + // just-in-time, so by the time this job runs we are on the Avalonia UI thread + // with its main loop already pumping. + return Dispatcher.MainThread.InvokeAsync( + _ => ShowWindowInternal(windowFunc, dataContext, parentHandle, ct) ); } diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index f5d2850fec..99be5059f5 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -5,9 +5,29 @@ namespace GitCredentialManager.UI { + /// + /// Owns the process entry thread (the "main thread") and runs work posted to it. + /// + /// + /// + /// Some platform APIs must be used from the process entry thread: macOS requires UI + /// controls to be created there, and the macOS MSAL broker requires a running + /// NSApplication. Both of those need a platform main loop, which is expensive to + /// start and most GCM invocations never need. + /// + /// + /// The dispatcher therefore parks the main thread cheaply until the first job is + /// posted, and only then starts the main loop just-in-time and hands the thread over + /// to it for the remaining lifetime of the process. Posting blocks until the main + /// loop can accept work, and that work only runs once the loop is pumping. + /// + /// public class Dispatcher { - private readonly DispatcherJobQueue _queue = new(); + // The queue of work to run on the dispatcher thread; the queue owns a loop that pumps work. + private readonly DispatcherJobQueue _queue; + + // The thread that owns this dispatcher; work posted to the dispatcher must run on this thread. private readonly Thread _thread; public static Dispatcher MainThread { get; private set; } @@ -15,27 +35,41 @@ public class Dispatcher /// /// Initialize the dispatcher associated to the current thread. See . /// - public static void Initialize() + public static void Initialize() => Initialize(new AvaloniaMainLoop()); + + internal static void Initialize(IMainLoop mainLoop) { - MainThread = new Dispatcher(Thread.CurrentThread); + MainThread = new Dispatcher(Thread.CurrentThread, mainLoop); } - private Dispatcher(Thread thread) + private Dispatcher(Thread thread, IMainLoop mainLoop) { _thread = thread; + _queue = new DispatcherJobQueue(this, mainLoop); } public void Run() { - // Should only run the dispatcher job queue from the thread that - // created the dispatcher. + // Should only run the dispatcher job queue from the thread that created the dispatcher. VerifyAccess(); _queue.Run(); } + /// + /// Stop the dispatcher and release the main thread, causing to return. + /// + /// + /// Shutdown does not drain outstanding work. Tasks for work that still needs the + /// main loop may never complete once it stops; already-started asynchronous work + /// may still finish independently. Callers must therefore only shut down once all + /// work they care about has finished. This is why the application thread shuts the + /// dispatcher down after running to completion, rather than the other way around: + /// waiting for outstanding work instead would hang whenever a window is still open + /// with nobody left to close it. + /// public void Shutdown() { - // Can shutdown the dispatcher from any thread. + // Can shut down the dispatcher from any thread. _queue.Shutdown(); } @@ -45,7 +79,7 @@ public void Shutdown() /// Ensure the calling thread is the thread associated with this dispatcher. /// /// - /// The calling thread does not have access this dispatcher. + /// The calling thread does not have access to this dispatcher. /// public void VerifyAccess() { @@ -59,6 +93,10 @@ public void VerifyAccess() /// Post work to be run on the thread associated with this dispatcher. /// /// Work to be run. + /// + /// The first call to the dispatcher starts the main loop, and blocks until it can + /// accept posted work. + /// public void Post(Action work) { Task _ = InvokeAsync(work); @@ -69,6 +107,10 @@ public void Post(Action work) /// return a task that completes when the work is done. /// /// Work to be run. + /// + /// The first call to the dispatcher starts the main loop, and blocks until it can + /// accept posted work. + /// public Task InvokeAsync(Action work) { var job = new DispatcherJob(ct => { work(ct); return null; }); @@ -89,6 +131,11 @@ public Task InvokeAsync(Func work) /// /// Work to be run. /// A task that completes when the work completes, not when it first yields. + /// + /// The work starts on the dispatcher thread, and because the main loop installs a + /// synchronization context its continuations resume there too, unless the work + /// opts out with . + /// public Task InvokeAsync(Func work) { var job = new AsyncDispatcherJob(work); @@ -219,9 +266,15 @@ private async Task CompleteAsync(Task task) private class DispatcherJobQueue { - private readonly Queue _queue = new(); + private readonly Dispatcher _owner; + private readonly IMainLoop _mainLoop; + private readonly Lock _lock = new(); + private readonly HashSet _outstandingJobs = new(); private readonly CancellationTokenSource _cts = new(); + private readonly ManualResetEventSlim _workRequested = new(false, spinCount: 0); + private readonly ManualResetEventSlim _mainLoopReady = new(false); + private enum State { NotStarted, @@ -232,9 +285,17 @@ private enum State private State _state = State.NotStarted; + private Exception _mainLoopFault; + + public DispatcherJobQueue(Dispatcher owner, IMainLoop mainLoop) + { + _owner = owner; + _mainLoop = mainLoop; + } + public void Run() { - lock (_queue) + lock (_lock) { switch (_state) { @@ -253,23 +314,31 @@ public void Run() try { - while (TryTake(out IDispatcherJob job)) + // Park cheaply until the main thread is actually needed. An invocation + // that never needs the main thread must not pay to start the main loop. + if (!WaitForWork()) { - job.Execute(_cts.Token); + // We were shut down before any work arrived! + return; } + + RunMainLoop(); } finally { - lock (_queue) + lock (_lock) { _state = State.Stopped; + + // No waiting caller can have its work accepted now. + _mainLoopReady.Set(); } } } public void Shutdown() { - lock (_queue) + lock (_lock) { switch (_state) { @@ -279,54 +348,182 @@ public void Shutdown() throw new InvalidOperationException("Dispatcher has already shut down."); } - // Shutting down before Run() has been reached is legitimate: the - // application thread can finish before the main thread gets there. + // Shutting down before Run() has been reached is legitimate. + // The application thread can finish before the main thread gets there. // Run() sees this and returns without starting anything. _state = State.Stopping; - _cts.Cancel(); - Monitor.Pulse(_queue); + _workRequested.Set(); + _mainLoopReady.Set(); } + + // Cancel outside the lock. This runs the main loop's own cancellation + // callbacks, which take its locks. + _cts.Cancel(); } public void AddJob(IDispatcherJob job) { - lock (_queue) + Exception fault; + + lock (_lock) { - switch (_state) + ThrowIfShuttingDown(); + + if (!_mainLoopReady.IsSet && _owner.CheckAccess()) { - case State.Stopping: - throw new InvalidOperationException("Dispatcher is shutting down."); - case State.Stopped: - throw new InvalidOperationException("Dispatcher has shut down."); + // We would be waiting on the only thread that can release us. + throw new InvalidOperationException( + "Cannot post work from the dispatcher thread before the main loop is running."); } - _queue.Enqueue(job); - Monitor.Pulse(_queue); + // Needing the main thread is what starts the main loop. + _workRequested.Set(); + } + + // Wait for the main loop to be ready to accept work before we post the job. + _mainLoopReady.Wait(); + + lock (_lock) + { + // The 'ready' gate also opens on failure and shutdown, not just success. + fault = _mainLoopFault; + if (fault is null) + { + ThrowIfShuttingDown(); + _outstandingJobs.Add(job); + } + } + + // Complete outside the lock; the main loop takes its own locks. + if (fault is not null) + { + job.Fail(fault); + return; + } + + // Async work remains faultable after its initial callback returns. + // Cleanup must not need the main loop, which may have failed. + _ = RemoveWhenCompletedAsync(job); + + try + { + PostToMainLoop(job); + } + catch (Exception ex) + { + FailAllJobs(ex); + } + } + + private async Task RemoveWhenCompletedAsync(IDispatcherJob job) + { + // Observe faults without rethrowing them from this bookkeeping task. + await job.Completion.ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); + + lock (_lock) + { + _outstandingJobs.Remove(job); } } - private bool TryTake(out IDispatcherJob job) + /// + /// Start the platform main loop and hand the dispatcher thread over to it. + /// Runs on the dispatcher thread and does not return until shutdown. + /// + private void RunMainLoop() { - lock (_queue) + try { - while (_queue.Count == 0) + // Initialize the main loop on this thread. Once this returns its own + // dispatcher exists and accepts posted work, even though the main loop + // is not running yet. + _mainLoop.Initialize(); + + // Release the callers waiting to post. Their work still cannot run + // until the main loop below is pumping (which we start below), but + // after init the main loop can begin accepting and queuing work. + _mainLoopReady.Set(); + + // Owns the dispatcher thread until shutdown. + _mainLoop.Run(_cts.Token); + } + catch (Exception ex) + { + // The main loop is unusable, so no main thread work can ever run. + // Fail outstanding and future jobs so callers see the error instead of + // hanging, then keep this thread parked: the application thread still + // needs to unwind and shut us down so the process exits cleanly. + FailAllJobs(ex); + WaitForShutdown(); + } + } + + private void PostToMainLoop(IDispatcherJob job) => _mainLoop.Post(() => + { + lock (_lock) + { + // A posting failure can invalidate callbacks already in the platform queue. + if (_mainLoopFault is not null) { - // Only check for stopping state when the queue is empty - // to allow remaining jobs to drain. We check for the stopping - // state in AddJob to ensure no more jobs can be added. - if (_state == State.Stopping) - { - job = null; - return false; - } - - Monitor.Wait(_queue); + return; } + } - job = _queue.Dequeue(); - return true; + job.Execute(_cts.Token); + }); + + private void FailAllJobs(Exception ex) + { + IDispatcherJob[] pending; + lock (_lock) + { + if (_mainLoopFault is not null) + { + return; + } + + _mainLoopFault = ex; + pending = new IDispatcherJob[_outstandingJobs.Count]; + _outstandingJobs.CopyTo(pending); + _outstandingJobs.Clear(); + + // Release anyone waiting to post; they will see the fault instead. + _mainLoopReady.Set(); + } + + foreach (IDispatcherJob job in pending) + { + job.Fail(ex); + } + } + + private void ThrowIfShuttingDown() + { + switch (_state) + { + case State.Stopping: + throw new InvalidOperationException("Dispatcher is shutting down."); + case State.Stopped: + throw new InvalidOperationException("Dispatcher has shut down."); } } + + /// + /// Block until the main thread is needed, or until shutdown. + /// + /// True if there is work to do, false if the dispatcher is shutting down. + private bool WaitForWork() + { + _workRequested.Wait(); + + lock (_lock) + { + return _state is not (State.Stopping or State.Stopped); + } + } + + // Only the failure path materializes the cancellation token's wait handle. + private void WaitForShutdown() => _cts.Token.WaitHandle.WaitOne(); } } } diff --git a/src/Core/UI/IMainLoop.cs b/src/Core/UI/IMainLoop.cs new file mode 100644 index 0000000000..7cec10d94c --- /dev/null +++ b/src/Core/UI/IMainLoop.cs @@ -0,0 +1,35 @@ +using System; +using System.Threading; + +namespace GitCredentialManager.UI +{ + /// + /// A platform main loop that owns the thread. + /// + /// + /// All members except are called on the dispatcher thread. + /// + internal interface IMainLoop + { + /// + /// Initialize the main loop on the calling thread, but do not start running it. + /// + /// + /// Once this returns, must accept work even though + /// has not been called yet. + /// + void Initialize(); + + /// + /// Post work to the main loop's job queue. Callable from any thread. + /// + /// Work to be run. + void Post(Action work); + + /// + /// Run the main loop, returning only once is cancelled. + /// + /// Token signalling that the main loop should exit. + void Run(CancellationToken ct); + } +} From b2c43a69c8135241e76ca3e9e38e483bcafe9531 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 16:28:47 +0100 Subject: [PATCH 08/12] entra: explain the macOS broker thread requirement The comment claimed the broker needs the main thread "to display UI", which sends anyone reading it looking for a window parenting problem. The real constraint is that the macOS broker needs a running NSApplication, and that MSAL decides once per process whether it has one and caches that answer. Without NSApplication it requires interactive calls to run on managed thread 1 and then takes that thread over with its own polling loop. Record why dispatching is what avoids that, why the silent attempts above deliberately do not dispatch, and which MSAL version the reasoning was checked against, since an upgrade could move the decision to another code path. The variable carrying the platform check is dropped: it read as though the main thread were a hard requirement of the call, when it is really how we guarantee NSApplication is up. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- .../Entra/EntraAuthentication.PublicClient.cs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs b/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs index 009d2c31cd..06ece660d5 100644 --- a/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs +++ b/src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs @@ -255,10 +255,18 @@ await UseDefaultAccountAsync(result.Account.Username, ct)) Context.Trace.WriteLine("Using broker for interactive authentication..."); - // On some platforms the broker requires the use of the main thread to display UI. - // If we are on some other thread, we need to dispatch the interactive auth call to the main thread. - bool isMainThreadRequired = PlatformUtils.IsMacOS(); - if (isMainThreadRequired && !Dispatcher.MainThread.CheckAccess()) + // The macOS broker requires a running NSApplication. MSAL decides once per process + // whether it has one and caches the answer: with NSApplication running it delegates + // threading to the broker, but without it MSAL demands that interactive calls run on + // managed thread 1 and then seizes that thread with its own polling loop - which + // cannot coexist with our main loop. + // + // Running this on the dispatcher avoids that entirely: it owns the entry thread and + // has started the main loop, and therefore NSApplication, by the time our work runs. + // Note that only interactive calls decide the mode, so the silent attempts above must + // stay off the dispatcher, or they would pay to start Avalonia for nothing. + // Verified against MSAL 4.85.2; re-check DesktopOsHelper.IsMacConsoleApp on upgrade. + if (PlatformUtils.IsMacOS() && !Dispatcher.MainThread.CheckAccess()) { Context.Trace.WriteLine("Dispatching interactive broker authentication to main thread..."); return await Dispatcher.MainThread.InvokeAsync( @@ -267,7 +275,7 @@ await UseDefaultAccountAsync(result.Account.Username, ct)) ); } - // Run the auth on the current thread + // Already on the main thread, or on a platform whose broker does not care return await app.AcquireTokenInteractive(scopes) .ExecuteAsync(ct); } From c421b2244c43c4eeeb1f7fafa4afa1d80203a30f Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 13:04:57 +0100 Subject: [PATCH 09/12] trace2: give the application thread its own context The application thread deliberately ran without a Trace2 thread scope so that its events were attributed to "main". That was accurate enough when it was the only thread doing anything: the main thread merely served queued work, and attributing that work to the thread that asked for it was more useful than naming the thread it happened to run on. That is no longer true. The main thread now starts and runs the platform main loop, and emits events of its own while doing so. Both threads reported as "main", leaving no way to tell the two apart in a trace. Give the application thread its own context so they can be. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- src/git-credential-manager/Program.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/git-credential-manager/Program.cs b/src/git-credential-manager/Program.cs index 5d8a2a83b7..d68084e317 100644 --- a/src/git-credential-manager/Program.cs +++ b/src/git-credential-manager/Program.cs @@ -43,11 +43,10 @@ private static void AppMain(object o) { string[] args = (string[])o; - // Do NOT start a Trace2 thread scope for the 'AppMain' thread so that all traces are attributed - // to the 'main' thread. We do not gain anything accurately attributing things to this secondary - // thread that actually runs the majority of the application. - // The existence of this AppMain-thread is only to provide Avalonia UI with the actual initial - // thread #1 that some platforms require (namely macOS) for interacting with UI components. + // Give this thread its own Trace2 context. The main thread now emits its own + // events, since it starts and runs the platform main loop, so without this + // both would report as "main" and be impossible to tell apart. + using (Trace2.StartThread(nameof(AppMain))) using (var context = new CommandContext()) using (var app = new Application(context)) { From 32ba1fbfe925a706c33272b387de00d30ac3b7a4 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 18:36:06 +0100 Subject: [PATCH 10/12] trace2: expose the thread context as a handle Trace2 attributes each event to a logical thread, and until now the only way to change that attribution was UseMainContext, which could restore exactly one context - the process one - and had no callers left. What is actually needed is the general case: capture whichever context a thread is currently reporting as, and apply it somewhere else. The dispatcher runs work on behalf of other threads and needs precisely that. Replace it with a matching get/set pair over an opaque Trace2Context handle. The type carries no public surface of its own, so callers can only pass it back to Trace2, and it stays free to grow internal state without becoming API. Setting is a no-op before initialization or for a null handle, so a context captured early can be restored without a guard. StartThread remains the right tool for genuinely new logical threads; this is for work that continues an existing one somewhere else. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- src/Core/Tracing/Trace2.cs | 131 +++++++++++++++++++++---------------- 1 file changed, 74 insertions(+), 57 deletions(-) diff --git a/src/Core/Tracing/Trace2.cs b/src/Core/Tracing/Trace2.cs index 50334bd1ab..20aa87cd60 100644 --- a/src/Core/Tracing/Trace2.cs +++ b/src/Core/Tracing/Trace2.cs @@ -35,12 +35,12 @@ public static class Trace2 private static readonly Lock WritersLock = new(); private static readonly List Writers = new(); - private static readonly AsyncLocal ThreadContext = new(); + private static readonly AsyncLocal ThreadContext = new(); private static bool _initialized; private static DateTimeOffset _applicationStartTime; private static Trace2Settings _settings; - private static Trace2ExecutionContext _mainContext; + private static Trace2Context _mainContext; private static string _sid; private static int _depth; @@ -78,7 +78,7 @@ public static void Initialize( InitializeWriters(); // The main thread context is ambiently created with the process and Trace2 init - _mainContext = new Trace2ExecutionContext(MainThreadName, _applicationStartTime); + _mainContext = new Trace2Context(MainThreadName, _applicationStartTime); ThreadContext.Value = _mainContext; _initialized = true; @@ -122,15 +122,51 @@ internal static int GetProcessDepth(string sid) return count; } - private static void SetContext(Trace2ExecutionContext context) + /// + /// Sets the Trace2 context of the current logical thread. + /// + /// The context to attribute subsequent events to. + /// + /// + /// Use this together with to attribute work to a logical + /// thread other than the one that scheduled it, such as work handed to another thread + /// to run on its behalf. The context is stored in an , so + /// the change is scoped to the current execution context and is undone when that + /// context is left. Callers that need it undone sooner must restore the previous + /// context themselves. + /// + /// + /// Prefer when the work is a genuinely new logical thread + /// rather than a continuation of an existing one. + /// + /// + /// The call is ignored when Trace2 has not been initialized, or when + /// is null, so that a context captured before + /// initialization can be restored without a null check. + /// + /// + public static void SetCurrentContext(Trace2Context context) { + if (!_initialized || context is null) return; + ThreadContext.Value = context; } - private static Trace2ExecutionContext GetCurrentContext() + /// + /// Gets the Trace2 context of the current logical thread. + /// + /// + /// The current context, or null if Trace2 has not been initialized. + /// + /// + /// The returned handle is opaque; pass it to to attribute + /// events elsewhere to this logical thread. + /// + public static Trace2Context GetCurrentContext() { - Trace2ExecutionContext context = ThreadContext.Value; - Debug.Assert(context is not null, "Trace2 event emitted without an execution context."); + Trace2Context context = ThreadContext.Value; + Debug.Assert(context is not null || !_initialized, + "Trace2 event emitted without an execution context."); // Fall back to the main thread context if we are missing one. // This can happen when ExecutionContext flow is suppressed, an unsafe // ThreadPool API is used, or work runs on a manually created thread without @@ -138,14 +174,6 @@ private static Trace2ExecutionContext GetCurrentContext() return context ?? _mainContext; } - public static IDisposable UseMainContext() - { - if (!_initialized) - return NoOpDisposable.Instance; - - return new ContextScope(_mainContext); - } - private static void Start(string appPath, string[] args, string filePath, @@ -545,7 +573,7 @@ public static IDisposable StartRegion( if (!_initialized) return NoOpDisposable.Instance; - Trace2ExecutionContext context = GetCurrentContext(); + Trace2Context context = GetCurrentContext(); return new RegionScope(context, category, label, filePath, lineNumber, message); } @@ -572,7 +600,7 @@ public static void WriteData( value ??= string.Empty; DateTimeOffset now = DateTimeOffset.UtcNow; - Trace2ExecutionContext context = GetCurrentContext(); + Trace2Context context = GetCurrentContext(); DateTimeOffset relativeStart = context.RegionStartTime.Value ?? context.StartTime; WriteMessage(new DataMessage @@ -641,7 +669,7 @@ public static void WriteData( } DateTimeOffset now = DateTimeOffset.UtcNow; - Trace2ExecutionContext context = GetCurrentContext(); + Trace2Context context = GetCurrentContext(); DateTimeOffset relativeStart = context.RegionStartTime.Value ?? context.StartTime; WriteMessage(new DataJsonMessage @@ -1026,44 +1054,12 @@ private class NoOpDisposable : IDisposable public void Dispose(){} } - private class Trace2ExecutionContext( - string threadName, - DateTimeOffset? startTime = null) - { - public AsyncLocal RegionNesting { get; } = new(); - public AsyncLocal RegionStartTime { get; } = new(); - public DateTimeOffset StartTime { get; } = startTime ?? DateTimeOffset.UtcNow; - public string ThreadName { get; } = threadName; - } - - private class ContextScope : DisposableObject - { - private readonly Trace2ExecutionContext _context; - private readonly Trace2ExecutionContext _previousContext; - - public ContextScope(Trace2ExecutionContext context) - { - _context = context; - _previousContext = Trace2.GetCurrentContext(); - Trace2.SetContext(_context); - } - - protected override void ReleaseManagedResources() - { - Debug.Assert( - ReferenceEquals(Trace2.GetCurrentContext(), _context), - "Trace2 contexts must be disposed in LIFO order."); - - Trace2.SetContext(_previousContext); - } - } - private class ThreadScope : DisposableObject { private readonly string _filePath; private readonly int _lineNumber; - private readonly Trace2ExecutionContext _context; - private readonly Trace2ExecutionContext _prevContext; + private readonly Trace2Context _context; + private readonly Trace2Context _prevContext; private readonly DateTimeOffset _startTime; public ThreadScope(string threadName, string filePath, int lineNumber) @@ -1072,8 +1068,8 @@ public ThreadScope(string threadName, string filePath, int lineNumber) _filePath = filePath; _lineNumber = lineNumber; - _context = new Trace2ExecutionContext(threadName); - Trace2.SetContext(_context); + _context = new Trace2Context(threadName); + Trace2.SetCurrentContext(_context); _startTime = Trace2.WriteThreadStart(_context.ThreadName, _filePath, _lineNumber); } @@ -1090,14 +1086,14 @@ protected override void ReleaseManagedResources() ReferenceEquals(Trace2.GetCurrentContext(), _context), "Trace2 threads must be disposed in LIFO order."); - Trace2.SetContext(_prevContext); + Trace2.SetCurrentContext(_prevContext); } } } private class RegionScope : DisposableObject { - private readonly Trace2ExecutionContext _context; + private readonly Trace2Context _context; private readonly string _category; private readonly string _label; private readonly string _filePath; @@ -1108,7 +1104,7 @@ private class RegionScope : DisposableObject private readonly DateTimeOffset _startTime; internal RegionScope( - Trace2ExecutionContext context, + Trace2Context context, string category, string label, string filePath, @@ -1165,3 +1161,24 @@ protected override void ReleaseManagedResources() } } } + +/// +/// An opaque handle to the Trace2 context of a logical thread. +/// +/// +/// Capture one with and apply it with +/// . +/// +public sealed class Trace2Context +{ + internal Trace2Context(string threadName, DateTimeOffset? startTime = null) + { + ThreadName = threadName; + StartTime = startTime ?? DateTimeOffset.UtcNow; + } + + internal AsyncLocal RegionNesting { get; } = new(); + internal AsyncLocal RegionStartTime { get; } = new(); + internal DateTimeOffset StartTime { get; } + internal string ThreadName { get; } +} From d0cc0b44a4f714cfd33491b17e414dd8cfc8e59b Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 18:36:26 +0100 Subject: [PATCH 11/12] dispatcher: run jobs with the caller's ambient state Work handed to the dispatcher runs on the main thread, and picked up whatever ambient state that thread happened to be carrying rather than the caller's. Nothing flowed by AsyncLocal survived the crossing: Activity.Current, and anything else a caller had established around the call, were silently absent inside the job. Neither the platform main loop nor a dispatcher of our own restores it for us. Capture the caller's execution context when the job is created and run the work inside it. A caller that has deliberately suppressed flow captures nothing, and gets the old behaviour. Trace2 has to be carved out of that, because it records which thread work ran on rather than which thread wanted it. Restoring the caller wholesale would have reported main thread work - showing a window, driving the macOS broker - as AppMain, and folded the time it took into whichever region the caller had open. So the dispatcher's own Trace2 context is applied on top, with the caller recorded as data so the link back is not lost. The two interact, which is worth spelling out: the Trace2 switch has to happen inside the restored execution context, since restoring replaces the whole AsyncLocal map and would otherwise shadow a switch made around it. For the same reason the dispatcher's context is captured when it is created rather than read when a job runs - by then the caller's context is in place, and would be the one observed. Assisted-by: Claude Opus 5 Assisted-by: GPT-6 Astra Signed-off-by: Matthew John Cheetham --- src/Core.Tests/UI/DispatcherTests.cs | 73 ++++++++++++++++++++++ src/Core/UI/Dispatcher.cs | 91 +++++++++++++++++++++++++--- 2 files changed, 154 insertions(+), 10 deletions(-) diff --git a/src/Core.Tests/UI/DispatcherTests.cs b/src/Core.Tests/UI/DispatcherTests.cs index 362723b487..0ac5a653c2 100644 --- a/src/Core.Tests/UI/DispatcherTests.cs +++ b/src/Core.Tests/UI/DispatcherTests.cs @@ -144,6 +144,35 @@ public async Task Dispatcher_InvokeAsync_BeforeRunIsReached_PreservesWorkRequest } } + [Fact] + public async Task Dispatcher_InvokeAsync_RunsWorkInThePostingCallersExecutionContext() + { + var mainLoop = new FakeMainLoop(); + var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + // Set the value only once the dispatcher thread exists so that it cannot have + // inherited it; the value can now only reach a job by flowing from the call that + // posted it. + var state = new AsyncLocal(); + state.Value = "caller"; + + // The first job is posted while the main loop is still starting, and the second + // once it is already running: two different paths through AddJob. + string cold = null; + await dispatcher.InvokeAsync(_ => { cold = state.Value; }).WaitAsync(Timeout); + Assert.Equal("caller", cold); + + string warm = await dispatcher.InvokeAsync(_ => state.Value).WaitAsync(Timeout); + Assert.Equal("caller", warm); + + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + [Theory] [InlineData(false)] [InlineData(true)] @@ -283,6 +312,50 @@ void CompleteWork() } } + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Dispatcher_InvokeAsync_AsyncWork_PreservesContextAcrossAwait(bool returnsValue) + { + var mainLoop = new FakeMainLoop(); + using var initialized = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + var state = new AsyncLocal { Value = "caller" }; + + try + { + async Task Work(CancellationToken ct) + { + Assert.True(dispatcher.CheckAccess()); + Assert.Equal("caller", state.Value); + SynchronizationContext context = SynchronizationContext.Current; + Assert.NotNull(context); + + state.Value = "job"; + await Task.Yield(); + + Assert.True(dispatcher.CheckAccess()); + Assert.Same(context, SynchronizationContext.Current); + Assert.Equal("job", state.Value); + return 42; + } + + Task task = returnsValue + ? dispatcher.InvokeAsync(Work) + : dispatcher.InvokeAsync(ct => (Task)Work(ct)); + await task.WaitAsync(Timeout); + Assert.Equal("caller", state.Value); + } + finally + { + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + [Theory] [InlineData(false)] [InlineData(true)] diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index 99be5059f5..b34c1e5198 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -30,6 +30,10 @@ public class Dispatcher // The thread that owns this dispatcher; work posted to the dispatcher must run on this thread. private readonly Thread _thread; + // Work run by this dispatcher is reported against this context rather than + // against whoever posted it, since this is the thread it executes on. + private readonly Trace2Context _traceContext; + public static Dispatcher MainThread { get; private set; } /// @@ -39,12 +43,13 @@ public class Dispatcher internal static void Initialize(IMainLoop mainLoop) { - MainThread = new Dispatcher(Thread.CurrentThread, mainLoop); + MainThread = new Dispatcher(Thread.CurrentThread, Trace2.GetCurrentContext(), mainLoop); } - private Dispatcher(Thread thread, IMainLoop mainLoop) + private Dispatcher(Thread thread, Trace2Context traceContext, IMainLoop mainLoop) { _thread = thread; + _traceContext = traceContext; _queue = new DispatcherJobQueue(this, mainLoop); } @@ -113,7 +118,8 @@ public void Post(Action work) /// public Task InvokeAsync(Action work) { - var job = new DispatcherJob(ct => { work(ct); return null; }); + var job = new DispatcherJob( + ct => { work(ct); return null; }, ExecutionContext.Capture(), _traceContext); _queue.AddJob(job); return job.Completion; } @@ -121,7 +127,7 @@ public Task InvokeAsync(Action work) /// public Task InvokeAsync(Func work) { - var job = new DispatcherJob(work); + var job = new DispatcherJob(work, ExecutionContext.Capture(), _traceContext); _queue.AddJob(job); return job.Completion; } @@ -138,7 +144,7 @@ public Task InvokeAsync(Func work) /// public Task InvokeAsync(Func work) { - var job = new AsyncDispatcherJob(work); + var job = new AsyncDispatcherJob(work, ExecutionContext.Capture(), _traceContext); _queue.AddJob(job); return job.Completion; } @@ -146,7 +152,7 @@ public Task InvokeAsync(Func work) /// public Task InvokeAsync(Func> work) { - var job = new AsyncDispatcherJob(work); + var job = new AsyncDispatcherJob(work, ExecutionContext.Capture(), _traceContext); _queue.AddJob(job); return job.Completion; } @@ -162,13 +168,22 @@ private interface IDispatcherJob private abstract class DispatcherJob : IDispatcherJob { + private readonly ExecutionContext _callerContext; + private readonly Trace2Context _traceContext; + public abstract Task Completion { get; } + protected DispatcherJob(ExecutionContext callerContext, Trace2Context traceContext) + { + _callerContext = callerContext; + _traceContext = traceContext; + } + public void Execute(CancellationToken ct) { try { - ExecuteCore(ct); + RunWork(() => ExecuteCore(ct)); } catch (Exception ex) { @@ -181,6 +196,53 @@ public void Execute(CancellationToken ct) public abstract void Fail(Exception ex); protected abstract void ExecuteCore(CancellationToken ct); + + /// + /// Run the work as the caller that posted it, but reported as the thread that + /// is running it. + /// + private void RunWork(Action work) + { + // Work must observe the ambient state of whoever posted it, so the caller's + // execution context is restored around it; anything flowed by AsyncLocal, + // such as System.Diagnostics.Activity.Current, would otherwise be lost + // crossing to the dispatcher thread. + // + // Trace2 is the exception. It reports which thread work ran on, and this work + // runs on the dispatcher thread, so its context is applied on top - and must + // be applied inside the restored context, since restoring replaces the whole + // AsyncLocal map and would shadow a switch made outside it. + + // Nothing to restore if the caller suppressed flow. + if (_callerContext is null) + { + RunAs(work); + return; + } + + ExecutionContext.Run(_callerContext, state => RunAs((Action)state), work); + } + + private void RunAs(Action work) + { + Trace2Context previous = Trace2.GetCurrentContext(); + Trace2.SetCurrentContext(_traceContext); + try + { + if (!ReferenceEquals(previous, _traceContext)) + { + // Switching context loses which logical thread asked for the work, + // so record it before we run. + Trace2.WriteData("dispatcher", "caller", previous?.ThreadName ?? string.Empty); + } + + work(); + } + finally + { + Trace2.SetCurrentContext(previous); + } + } } private sealed class DispatcherJob : DispatcherJob @@ -191,7 +253,10 @@ private sealed class DispatcherJob : DispatcherJob public override Task Completion => _tcs.Task; - public DispatcherJob(Func work) + public DispatcherJob( + Func work, + ExecutionContext callerContext, + Trace2Context traceContext) : base(callerContext, traceContext) { _work = work; } @@ -209,7 +274,10 @@ private sealed class AsyncDispatcherJob : DispatcherJob public override Task Completion => _tcs.Task; - public AsyncDispatcherJob(Func work) + public AsyncDispatcherJob( + Func work, + ExecutionContext callerContext, + Trace2Context traceContext) : base(callerContext, traceContext) { _work = work; } @@ -241,7 +309,10 @@ private sealed class AsyncDispatcherJob : DispatcherJob public override Task Completion => _tcs.Task; - public AsyncDispatcherJob(Func> work) + public AsyncDispatcherJob( + Func> work, + ExecutionContext callerContext, + Trace2Context traceContext) : base(callerContext, traceContext) { _work = work; } From e4ede935e8dce8a56a4d79cb511ff75cdc888172 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 18:38:45 +0100 Subject: [PATCH 12/12] docs: describe the main thread dispatcher The dispatcher carries a lot of load-bearing subtlety that is hard to recover from the code alone: why the main loop starts on the first job rather than on request, why posting is what blocks rather than the work, why a job is handed to the main loop by its own caller, why two flags gate the rendezvous, and why shutdown abandons outstanding jobs instead of draining them. Write it down, with diagrams for the thread interaction, the state machine, how work is routed, and how the main loop gets started. Document the context handling too, since the execution context and the Trace2 context are deliberately moved in opposite directions and the ordering between them matters. Include the rules a caller needs to follow, since the cost of getting them wrong is a stall or a rejected post rather than an obvious failure. Assisted-by: Claude Opus 5 Assisted-by: GPT-6 Astra Signed-off-by: Matthew John Cheetham --- docs/architecture.md | 6 + docs/dispatcher.md | 328 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 334 insertions(+) create mode 100644 docs/dispatcher.md diff --git a/docs/architecture.md b/docs/architecture.md index ed319051ab..3bd8d66684 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -98,6 +98,11 @@ GCM makes use of the `async`/`await` model of .NET and C# in almost all parts of the codebase where appropriate as usually requests end up going to the network at some point. +Work that must run on the process entry thread - creating UI controls, or +using the macOS identity broker - is marshalled there by the main thread +dispatcher. See the [main thread dispatcher][gcm-dispatcher] documentation for +how that works and the rules for posting to it. + ## Command execution ```text @@ -282,5 +287,6 @@ to the trace object in most places of GCM. [credential-provider]: configuration.md#credentialprovider [issue-113]: https://github.com/git-ecosystem/git-credential-manager/issues/113 [issue-136]: https://github.com/git-ecosystem/git-credential-manager/issues/136 +[gcm-dispatcher]: dispatcher.md [gcm-provider]: environment.md#GCM_PROVIDER [msal]: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet diff --git a/docs/dispatcher.md b/docs/dispatcher.md new file mode 100644 index 0000000000..b9d3512587 --- /dev/null +++ b/docs/dispatcher.md @@ -0,0 +1,328 @@ +# Main thread dispatcher + +## Why it exists + +Some platform APIs may only be used from the thread that started the process - +"thread 1", the *main thread*. On macOS this includes creating any UI control. + +The main-thread requirement also affects the macOS MSAL broker. MSAL decides +**once per process** whether an `NSApplication` is running, and caches that +answer. If one is not running it requires interactive broker calls to be made +on thread 1, and then takes that thread over with its own polling loop - which +cannot coexist with a UI main loop. If one *is* running it requires neither. + +Serving these by simply running GCM on the main thread is not an option: +starting a UI framework costs far more than a typical GCM invocation, and most +invocations never show a window at all. A `get` request served from the +credential store should not pay for a graphical toolkit. + +GCM therefore splits the two roles: + +- **The main thread** hosts the `Dispatcher` and serves work that must run on + thread 1. +- **The `AppMain` thread** runs the application itself - command dispatch, + provider selection, authentication, and everything else. + +The dispatcher keeps the main thread parked cheaply until somebody actually +needs it, and only then starts the platform main loop. + +## Thread layout + +```mermaid +sequenceDiagram + participant Main as main thread + participant App as AppMain thread + + Main->>Main: Dispatcher.Initialize() + Main->>App: start AppMain thread + Main->>Main: Dispatcher.MainThread.Run() + Note over Main: parked, waiting to be needed
no UI framework started + App->>Main: InvokeAsync(work) - the first job + Note over App: blocked until the loop can accept work + Note over Main: IMainLoop.Initialize() + Main-->>App: released + Note over Main: IMainLoop.Run(token) owns the thread + App->>Main: IMainLoop.Post(job) + Main-->>App: job runs and the awaited task completes + Note over App: _exitCode = ...
dispose app and context + App->>Main: Shutdown() - cancels the token + Note over Main: main loop exits
Run() returns + Main->>Main: Trace2.Stop(_exitCode)
Environment.Exit(_exitCode) +``` + +The main loop is started by the *first job posted*, not by an explicit call. +This is deliberate: it makes "work on the main thread implies a running main +loop" true by construction. There is no start-up call for a caller to forget, +and no ordering for a caller to get wrong. + +What waits is the *posting*, not the work. A caller that arrives before the +main loop is initialized blocks inside `AddJob` until it can accept work, and +then hands its own job to the loop. The first caller therefore pays the cost of +starting the UI framework - which is reasonable, since it is the one that +asked for the main thread. + +An invocation that posts no jobs never initialises the UI framework at all. The +main thread parks, `Shutdown()` wakes it, and `Run()` returns. + +## States + +```mermaid +stateDiagram-v2 + [*] --> NotStarted: Initialize() + NotStarted --> Started: Run() + NotStarted --> Stopping: Shutdown() wins the race + Started --> Stopping: Shutdown() + Stopping --> Stopped: Run() returns, or Run() finds
the dispatcher already stopping + Stopped --> [*] +``` + +`NotStarted -> Stopping` is a legitimate race, not misuse. `Program.Main` starts +the `AppMain` thread *before* calling `Run()`, so a fast invocation can finish +and shut down first. `Run()` detects this and returns without starting anything. + +The distinction between `Stopping` and `Stopped` is what allows that tolerance +without also silently accepting a genuine error. Calling `Run()` once the thread +has been released is a programming error, and throws. + +## Data structures + +`_lock`, a `System.Threading.Lock`, guards the lifecycle state, fault, and +outstanding jobs. Two one-shot `ManualResetEventSlim` gates handle waiting, +always outside that lock: + +Name|Purpose +-|- +`_workRequested`|Opened by the first work request or shutdown. Releases the parked dispatcher thread, which checks whether it should initialize the main loop or exit. +`_mainLoopReady`|Opened when initialization succeeds, the main loop fails, or the dispatcher shuts down. Releases callers waiting in `AddJob`, which inspect the outcome under `_lock` before accepting work. +`_outstandingJobs`|Every accepted job, tracked until its completion task settles, including time spent awaiting asynchronous work. This is what makes a main loop failure recoverable - see [Failure handling](#failure-handling). +`_mainLoopFault`|The first fault seen, if any. Once set, the dispatcher is permanently unusable. +`_state`|See [States](#states). + +Neither gate is ever reset. Signals survive until a thread waits, and every +current or future waiter is released once a gate opens. An open gate is a +reason to inspect the state, not a promise that work can run. + +`_workRequested` uses a zero spin count to park the main thread without +spinning while it waits to be needed. `_mainLoopReady` uses the default spin +count, allowing a brief spin before blocking. + +The gates have the dispatcher's lifetime, not just `Run()`'s. They are not +disposed by `Shutdown()` or `Run()`, since posting callers can still be waiting +or signalling as those methods return. Neither gate's `WaitHandle` is used, so +their native wait handles are never materialized. + +The dispatcher deliberately keeps **no queue of its own**. The only queue is the +main loop's - Avalonia's dispatcher queue - which the dispatcher can add to via +`IMainLoop.Post`. A job therefore never sits anywhere that nothing is pumping. + +## Posting work + +`Dispatcher` exposes one fire-and-forget method and four awaitable ones: + +Method|Returns|Completes when +-|-|- +`Post(Action)`|`void`|n/a - the task is discarded +`InvokeAsync(Action)`|`Task`|the delegate returns +`InvokeAsync(Func)`|`Task`|the delegate returns +`InvokeAsync(Func)`|`Task`|the returned task completes +`InvokeAsync(Func>)`|`Task`|the returned task completes + +The `CancellationToken` passed to the delegate is signalled at shutdown. + +Each job owns its work delegate and completion source. `DispatcherJob` +handles synchronous work; `AsyncDispatcherJob` and `AsyncDispatcherJob` +handle task-returning work. Their shared `DispatcherJob` base restores the +execution context, applies Trace2 attribution, and catches synchronous +exceptions from invoking the delegate. + +The last two overloads exist because an `async` delegate returns at its first +yielding `await`. Without them `async _ => ...` binds to `Func` with `T` +inferred as `Task<...>`, and the caller gets back a task that completes when the +work *starts* rather than when it finishes. These overloads forward the work's +task outcome into a completion source owned by the dispatcher. The task +returned to the caller is that completion source's task, not an `Unwrap()` +proxy, so `RunContinuationsAsynchronously` applies to the caller's +continuations as well. Results, exceptions, and cancellation are preserved. + +Each async job's `CompleteAsync` helper awaits the work's task without +capturing the main loop's synchronization context. `SuppressThrowing` lets it +observe completion before forwarding the original outcome with +`TrySetFromTask`, including all exceptions and the cancellation token. +Failures are transferred to the caller's task, not discarded. + +The queue tracks `IDispatcherJob.Completion`, without needing to distinguish +sync and async jobs. An asynchronous job remains in `_outstandingJobs` after +its first yield, so a later main-loop failure can still fault the caller's +task even when the work's own continuation can no longer run. Removal from +the set does not depend on the main loop being available. Cleanup observes +the registered job's completion, including faults, without rethrowing or +reporting them; the caller's task keeps its original outcome. + +> **Note** +> +> `Post` discards the task, so a job that throws has nowhere to report the +> failure. Prefer `InvokeAsync` unless the result genuinely does not matter. +> It is also not quite fire-and-forget: like every method here it blocks until +> the main loop can accept work; only waiting for the work's completion is +> omitted. + +Work always *begins* on the main thread. Because the main loop installs its own +synchronization context, continuations after an `await` resume there too, unless +the delegate opts out with `ConfigureAwait(false)`. + +### Routing + +```mermaid +flowchart TD + A["InvokeAsync(work) / Post(work)"] --> B["AddJob(job)"] + B --> C{"lock (_lock)"} + C -->|"state is Stopping or Stopped"| D["throw InvalidOperationException"] + C -->|"on the dispatcher thread,
startup still pending"| E["throw InvalidOperationException"] + C -->|"otherwise"| F["_workRequested.Set()
release the lock"] + F --> G["_mainLoopReady.Wait()
outside the lock"] + G --> H{"lock (_lock)
inspect the outcome"} + H -->|"fault"| I["job.Fail(fault)
outside the lock"] + H -->|"no fault, but stopping or stopped"| D + H -->|"initialized and accepting work"| J["_outstandingJobs.Add(job)"] + J --> K["IMainLoop.Post(job)
outside the lock"] +``` + +Opening `_workRequested` requests initialization. Waiting on +`_mainLoopReady` lets each caller inspect the outcome under `_lock`: fault +the job, reject shutdown, or accept it into `_outstandingJobs`. A caller that +has already passed the initial lifecycle check observes a recorded fault even +if shutdown follows it; without a fault, it must re-check the lifecycle before +accepting work. + +Failing or posting a job happens *outside* the lock, because both run code that +takes other locks - the main loop's, or the caller's continuation. Gate waits +also happen outside the lock: waiting on an event does not release a held lock. + +The second rejection above is the one worth knowing about. Posting from the +dispatcher thread itself while initialization is pending would block the only +thread that could ever release it, so it is rejected rather than left to +deadlock. Once initialization succeeds the dispatcher thread may post freely. + +## Starting the main loop + +```mermaid +flowchart TD + A["Run() - state is Started"] --> B["WaitForWork()
parked on _workRequested"] + B -->|"shutting down"| X["return without initializing"] + B -->|"work requested"| C["RunMainLoop()"] + C --> D["IMainLoop.Initialize()
the loop queue now exists,
but nothing is pumping it yet"] + D --> E["_mainLoopReady.Set()
waiting callers can post their jobs"] + E --> F["IMainLoop.Run(token)
owns the thread until shutdown"] + F --> G["only now do the posted jobs run"] +``` + +Every job is handed to the main loop by the caller that wanted it, which is why +callers wait for the loop rather than leaving work for the dispatcher thread to +forward. Posting on the caller's own stack keeps the job's [context](#context) +attached to it, and it means there is never a moment where accepted work is +sitting somewhere that nothing pumps. + +On the successful path, `_mainLoopReady` is set only once +`IMainLoop.Initialize` has returned. No job can *run* until `IMainLoop.Run` is +pumping - which on macOS is the point at which `NSApplication` starts. Work +posted to the dispatcher is therefore guaranteed to run with `NSApplication` +already up, so MSAL always sees a GUI application no matter which happens first. + +## Context + +A job runs on the main thread but conceptually belongs to the caller, and the +two want opposite things from the ambient state. + +- **Execution context.** The caller's is captured when the job is created and + restored around the work, so anything flowed by `AsyncLocal` - such as + `Activity.Current` - is still there inside the job. +- **Trace2 context.** Deliberately the other way round. Trace2 records the + thread work *ran on*, so the dispatcher's own context is applied on top of the + restored one, and the caller is recorded as a `dispatcher`/`caller` data event + so the link back is not lost. + +The Trace2 switch has to happen *inside* the restored execution context: +restoring replaces the whole `AsyncLocal` map, so a switch made around it +would simply be shadowed. For the same reason the dispatcher captures its own +Trace2 context when it is constructed rather than reading it when a job runs - +by then the caller's context is in place and would be the one observed. + +## Failure handling + +If the main loop cannot be started, or stops unexpectedly, no main thread work +can ever run. Callers waiting on a job would otherwise wait forever, and GCM +would hang with Git waiting on it. + +`FailAllJobs` therefore faults everything in `_outstandingJobs`, records the +fault, and fails any later arrivals immediately. It is idempotent: the first +fault wins, so concurrent failures report a single, consistent cause. + +Three further details matter: + +- Publishing `_mainLoopFault` and opening `_mainLoopReady` releases callers + blocked in `AddJob`. They see the fault instead of the loop they were waiting + for, so a loop that fails to start cannot strand them. The gate stays open so + later arrivals also see the fault without waiting. +- A callback already sitting in the main loop's queue re-checks `_mainLoopFault` + before executing, so work never runs after its task has been faulted. +- The dispatcher thread then parks until shutdown rather than propagating. The + `AppMain` thread still needs to observe its faulted task, unwind, and shut the + dispatcher down so that the process exits with the right code. Only this + failure-path wait materializes `_cts.Token.WaitHandle`, and it returns + immediately if shutdown already cancelled the token. + +## Shutdown + +`Shutdown()` may be called from any thread. It moves to `Stopping` and opens +both gates, releasing the parked dispatcher thread and any callers waiting to +post. Callers check the outcome rather than treating the signal as successful +initialization. It then cancels the token, outside the lock since cancellation +runs the main loop's own callbacks. Cancelling is the *only* stop signal the +main loop gets; `IMainLoop` has no separate shutdown method. + +**Outstanding work is abandoned, not drained.** Tasks that still depend on the +stopped loop may never complete. Already-started asynchronous work can still +settle independently, for example after `ConfigureAwait(false)` or cooperative +cancellation. `Shutdown()` must therefore be called only once the application +has finished everything it cares about. Draining would hang the common case +of a window shown without anyone awaiting it - there would be nothing left to +close the window, and so nothing to wait for. + +## Rules for contributors + +- **Anything needing the main thread goes through the dispatcher.** Do not reach + for the UI framework's own dispatcher directly. +- **Post only what genuinely needs thread 1.** Posting the first job is what + pays for starting the UI framework. This is why the silent authentication + paths deliberately stay off the dispatcher - see + [`EntraAuthentication.PublicClient.cs`][entra-public-client]. +- **Expect the first post to block.** Starting the main loop happens on the + posting caller's time. Do not post the first job from anywhere that cannot + afford to wait for a UI framework to initialise. +- **Do not block the main thread.** A job that blocks stops the loop pumping, + which stalls every other job, the UI, and any platform work the loop drives. +- **Do not synchronously block on a dispatcher task from inside a job.** + `Wait()`, `.Result`, and `GetAwaiter().GetResult()` can block the loop needed + to finish that work. Asynchronously awaiting nested dispatcher work is + supported; it yields the loop while the task is pending. +- `CheckAccess()` and `VerifyAccess()` report whether the calling thread is the + dispatcher thread, which is useful for avoiding a needless round trip. Before + the main loop is initialized such a round trip is not merely wasteful but + rejected, since the dispatcher thread would be waiting on itself. + +## Testing + +`IMainLoop` is an internal seam with a single production implementation, +`AvaloniaMainLoop`. An internal `Dispatcher.Initialize(IMainLoop)` overload lets +tests substitute a fake, so start-up, failure, and shutdown behaviour can be +exercised without a real UI framework and without a real thread 1. The fake can +also hold `Initialize` or `Run` open on demand, which is what makes the +rendezvous between a posting caller and the starting main loop testable. Its +synchronization context queues asynchronous continuations back to the fake +loop, so failures after a job yields can be exercised too. Tests cover signals +preceding waits, concurrent posters, asynchronous completion, and shutdown +while initialization is still blocked, whether it later succeeds or fails. See +[`DispatcherTests`][dispatcher-tests]. + +[dispatcher-tests]: ../src/Core.Tests/UI/DispatcherTests.cs +[entra-public-client]: ../src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs