From 2b75bba43c1ca4cc5844c72b196e64a8892b9b75 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 15:27:43 +0100 Subject: [PATCH 1/8] 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 | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index 9dd9914382..ed408cd892 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -101,8 +101,17 @@ public DispatcherJob(Action work, TaskCompletionSource 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) + { + _tcs.TrySetException(ex); + } } } From efcc662d1fbfa458263f2bcb827b64cc68768a7a Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 15:28:02 +0100 Subject: [PATCH 2/8] 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 ed408cd892..79c5fa9d06 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -71,14 +71,14 @@ 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(work, tcs)); return tcs.Task; } 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 b1a0a8a6a89a8bedc6c5b7244cf14afe4e0daad0 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 16:27:51 +0100 Subject: [PATCH 3/8] dispatcher: allow shutdown before the loop starts 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 | 61 ++++++++++++++++++++++++++++ src/Core/UI/Dispatcher.cs | 10 +++-- 2 files changed, 67 insertions(+), 4 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..7ec76f96e3 --- /dev/null +++ b/src/Core.Tests/UI/DispatcherTests.cs @@ -0,0 +1,61 @@ +using System; +using System.Threading; +using GitCredentialManager.UI; +using Xunit; + +namespace GitCredentialManager.Tests.UI; + +public class DispatcherTests +{ + private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(10); + + [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 + // 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/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index 79c5fa9d06..dd69046eb0 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -164,9 +164,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; @@ -184,13 +184,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 30d5c5fbe87858051f3132b1b15a4880d1277eb1 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 16:28:07 +0100 Subject: [PATCH 4/8] 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. Add overloads that take task-returning work and unwrap it, so the returned task tracks the work to completion. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- .../Entra/EntraAuthentication.PublicClient.cs | 6 ++---- src/Core/UI/Dispatcher.cs | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) 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 dd69046eb0..f4854e1dc2 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -83,6 +83,26 @@ public Task InvokeAsync(Func work) return tcs.Task; } + /// + /// 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 tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + _queue.AddJob(new DispatcherJob(work, tcs)); + return tcs.Task.Unwrap(); + } + + /// + public Task InvokeAsync(Func> work) + { + var tcs = new TaskCompletionSource>(TaskCreationOptions.RunContinuationsAsynchronously); + _queue.AddJob(new DispatcherJob>(work, tcs)); + return tcs.Task.Unwrap(); + } + private interface IDispatcherJob { void Execute(CancellationToken ct); From aee34ba94c7a4fe43f7915277b1f682168aff7fd Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 16:28:37 +0100 Subject: [PATCH 5/8] 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, and since the hand-over completes before any job is dispatched, every job is guaranteed to run with NSApplication already up. There is no escalation call for a caller to forget and no ordering left to get wrong. Invocations that need neither UI nor the broker still pay nothing: the thread parks on the job queue 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 itself 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 Signed-off-by: Matthew John Cheetham --- src/Core.Tests/UI/DispatcherTests.cs | 388 ++++++++++++++++++++++++++- src/Core/UI/AvaloniaMainLoop.cs | 60 +++++ src/Core/UI/AvaloniaUi.cs | 61 +---- src/Core/UI/Dispatcher.cs | 238 ++++++++++++++-- src/Core/UI/IMainLoop.cs | 35 +++ 5 files changed, 702 insertions(+), 80 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 7ec76f96e3..2fb5937c6c 100644 --- a/src/Core.Tests/UI/DispatcherTests.cs +++ b/src/Core.Tests/UI/DispatcherTests.cs @@ -1,5 +1,7 @@ using System; +using System.Collections.Concurrent; using System.Threading; +using System.Threading.Tasks; using GitCredentialManager.UI; using Xunit; @@ -10,42 +12,354 @@ public class DispatcherTests private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(10); [Fact] - public void Dispatcher_Shutdown_BeforeRunIsReached_RunReturns() + 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 shut down in the - // window that the application thread can genuinely hit in Program.Main. - Thread thread = StartDispatcherThread(initialized, () => mayRun.Wait(Timeout)); + // 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_Shutdown_NoWorkPosted_RunReturns() + public void Dispatcher_Shutdown_NoWorkPosted_NeverStartsMainLoop() { + var mainLoop = new FakeMainLoop(); var initialized = new ManualResetEventSlim(); - Thread thread = StartDispatcherThread(initialized); + 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); } - private static Thread StartDispatcherThread(ManualResetEventSlim initialized, Action beforeRun = null) + [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_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); + } + + [Fact] + public void Dispatcher_Shutdown_WorkStillPending_AbandonsIt() + { + var mainLoop = new FakeMainLoop { PumpPostedWork = false }; + var initialized = new ManualResetEventSlim(); + + 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); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task Dispatcher_InvokeAsync_MainLoopFails_FaultsPendingAndFutureJobs(bool failInitialize) + { + var failure = new InvalidOperationException("The main loop failed."); + Action fail = () => throw failure; + var mainLoop = new FakeMainLoop + { + BeforeInitialize = failInitialize ? fail : null, + BeforeRun = failInitialize ? null : fail, + }; + using var initialized = new ManualResetEventSlim(); + using var mayRun = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized, () => mayRun.Wait(Timeout)); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + bool workRan = false; + Task[] tasks = + { + dispatcher.InvokeAsync(_ => { workRan = true; }), + dispatcher.InvokeAsync(_ => { workRan = true; return 42; }), + dispatcher.InvokeAsync(_ => { workRan = true; return Task.CompletedTask; }), + dispatcher.InvokeAsync(_ => { workRan = true; return Task.FromResult(42); }), + }; + mayRun.Set(); + + foreach (Task task in tasks) + { + 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 + { + mayRun.Set(); + dispatcher.Shutdown(); + Assert.True(thread.Join(Timeout)); + } + } + + [Fact] + public async Task Dispatcher_InvokeAsync_PostFailsDuringHandoff_FaultsAllJobs() + { + var failure = new InvalidOperationException("Posting to the main loop failed."); + int postCount = 0; + var mainLoop = new FakeMainLoop + { + BeforePost = () => + { + if (Interlocked.Increment(ref postCount) == 2) + { + throw failure; + } + }, + }; + using var initialized = new ManualResetEventSlim(); + using var mayRun = new ManualResetEventSlim(); + + Thread thread = StartDispatcherThread(mainLoop, initialized, () => mayRun.Wait(Timeout)); + Assert.True(initialized.Wait(Timeout)); + Dispatcher dispatcher = Dispatcher.MainThread; + + try + { + bool workRan = false; + Task[] tasks = + { + dispatcher.InvokeAsync(_ => { workRan = true; }), + dispatcher.InvokeAsync(_ => { workRan = true; }), + dispatcher.InvokeAsync(_ => { workRan = true; }), + }; + mayRun.Set(); + + // Cover work already posted, the rejected post, and work not yet posted. + foreach (Task task in tasks) + { + 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); + Assert.False(mainLoop.Ran); + Assert.Equal(2, postCount); + } + finally + { + mayRun.Set(); + 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 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(); @@ -58,4 +372,62 @@ 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 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) + { + BeforePost?.Invoke(); + _work.Add(work); + } + + public void Run(CancellationToken ct) + { + Ran = true; + BeforeRun?.Invoke(); + try + { + if (PumpPostedWork) + { + foreach (Action work in _work.GetConsumingEnumerable(ct)) + { + work(); + AfterWork?.Invoke(); + } + } + else + { + ct.WaitHandle.WaitOne(); + ct.ThrowIfCancellationRequested(); + } + } + catch (OperationCanceledException) + { + RunWasCancelled = true; + } + } + } } 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 f4854e1dc2..bc0516a95e 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -5,9 +5,27 @@ 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. Because the hand-over completes + /// before any job can be dispatched, work posted here is guaranteed to run with the + /// main loop - and therefore NSApplication - already running. + /// + /// public class Dispatcher { - private readonly DispatcherJobQueue _queue = new(); + private readonly DispatcherJobQueue _queue; private readonly Thread _thread; public static Dispatcher MainThread { get; private set; } @@ -15,14 +33,17 @@ 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(mainLoop); } public void Run() @@ -33,6 +54,17 @@ public void Run() _queue.Run(); } + /// + /// Stop the dispatcher and release the main thread, causing to return. + /// + /// + /// Work that has been posted but has not yet completed is abandoned, and the tasks + /// returned for it never complete. 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. @@ -88,6 +120,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 tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); @@ -106,6 +143,8 @@ public Task InvokeAsync(Func> private interface IDispatcherJob { void Execute(CancellationToken ct); + + void Fail(Exception ex); } private class DispatcherJob : IDispatcherJob @@ -133,6 +172,8 @@ public void Execute(CancellationToken ct) _tcs.TrySetException(ex); } } + + public void Fail(Exception ex) => _tcs?.TrySetException(ex); } private class DispatcherJob : IDispatcherJob @@ -158,12 +199,16 @@ public void Execute(CancellationToken ct) _tcs.TrySetException(ex); } } + + public void Fail(Exception ex) => _tcs?.TrySetException(ex); } private class DispatcherJobQueue { private readonly Queue _queue = new(); + private readonly HashSet _outstandingJobs = new(); private readonly CancellationTokenSource _cts = new(); + private readonly IMainLoop _mainLoop; private enum State { @@ -175,6 +220,14 @@ private enum State private State _state = State.NotStarted; + private bool _isMainLoopRunning; + private Exception _mainLoopFault; + + public DispatcherJobQueue(IMainLoop mainLoop) + { + _mainLoop = mainLoop; + } + public void Run() { lock (_queue) @@ -192,10 +245,16 @@ public void Run() _state = State.Started; } - while (TryTake(out IDispatcherJob job)) + // Park cheaply until the main thread is actually needed. An invocation that + // never shows UI and never talks to the macOS broker must not pay to start + // the main loop. + if (!WaitForWork()) { - job.Execute(_cts.Token); + // We were shut down before any work arrived. + return; } + + RunMainLoop(); } public void Shutdown() @@ -214,13 +273,19 @@ public void Shutdown() // 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); + Monitor.PulseAll(_queue); } + + // Cancel outside of the queue lock: this runs the main loop's own + // cancellation callbacks, which take its locks. + _cts.Cancel(); } public void AddJob(IDispatcherJob job) { + bool post = false; + Exception fault; + lock (_queue) { switch (_state) @@ -231,33 +296,172 @@ public void AddJob(IDispatcherJob job) throw new InvalidOperationException("Dispatcher has shut down."); } - _queue.Enqueue(job); - Monitor.Pulse(_queue); + fault = _mainLoopFault; + if (fault is null) + { + _outstandingJobs.Add(job); + if (_isMainLoopRunning) + { + // Our own loop no longer pumps the thread, so queuing here would + // strand the job; hand it to the main loop instead. + post = true; + } + else + { + _queue.Enqueue(job); + Monitor.Pulse(_queue); + } + } + } + + // Complete outside of the queue lock; the main loop takes its own locks. + if (fault is not null) + { + job.Fail(fault); + } + else if (post) + { + try + { + PostToMainLoop(job); + } + catch (Exception ex) + { + FailAllJobs(ex); + } } } - 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() + { + try + { + // 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(); + + IDispatcherJob[] pending; + lock (_queue) + { + // Hand over and drain in a single atomic step so that no job can be + // enqueued into a queue that will never be pumped again. Outstanding + // jobs stay tracked until their callbacks finish. + _isMainLoopRunning = true; + pending = _queue.ToArray(); + _queue.Clear(); + } + + // Queue.ToArray returns items in dequeue order, so FIFO is preserved. + // These cannot run until the main loop below is pumping, which is exactly + // the guarantee callers rely on: on macOS the MSAL broker requires a + // running NSApplication, which only exists from that point onwards. + foreach (IDispatcherJob job in pending) + { + PostToMainLoop(job); + } + + // 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 (_queue) + { + // A posting failure can invalidate callbacks already in the platform queue. + if (_mainLoopFault is not null) + { + return; + } + } + + try + { + job.Execute(_cts.Token); + } + finally + { + lock (_queue) + { + _outstandingJobs.Remove(job); + } + } + }); + + private void FailAllJobs(Exception ex) + { + IDispatcherJob[] pending; + lock (_queue) + { + if (_mainLoopFault is not null) + { + return; + } + + _mainLoopFault = ex; + _isMainLoopRunning = false; + pending = new IDispatcherJob[_outstandingJobs.Count]; + _outstandingJobs.CopyTo(pending); + _outstandingJobs.Clear(); + _queue.Clear(); + } + + foreach (IDispatcherJob job in pending) + { + job.Fail(ex); + } + } + + /// + /// Block until a job is queued, or until shutdown. + /// + /// True if there is work to do, false if the dispatcher is shutting down. + private bool WaitForWork() { lock (_queue) { while (_queue.Count == 0) { - // 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) + // Only check for stopping state when the queue is empty so that any + // remaining jobs still get to run. AddJob rejects new jobs once we + // are stopping. + if (_state is State.Stopping or State.Stopped) { - job = null; return false; } Monitor.Wait(_queue); } - job = _queue.Dequeue(); return true; } } + + private void WaitForShutdown() + { + lock (_queue) + { + while (_state is not (State.Stopping or State.Stopped)) + { + Monitor.Wait(_queue); + } + } + } } } } 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 ad42569d1f611f9713015b9edce7ed14a19c3c80 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Wed, 16 Sep 2026 16:28:47 +0100 Subject: [PATCH 6/8] 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 7d7acdf8ba14313c60d2d4b67e99299ba56c0628 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 12:18:18 +0100 Subject: [PATCH 7/8] dispatcher: record when the loop has 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 | 31 ++++++++++++++++++++++++++++ src/Core/UI/Dispatcher.cs | 30 +++++++++++++++++++-------- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/Core.Tests/UI/DispatcherTests.cs b/src/Core.Tests/UI/DispatcherTests.cs index 2fb5937c6c..b1ca188b33 100644 --- a/src/Core.Tests/UI/DispatcherTests.cs +++ b/src/Core.Tests/UI/DispatcherTests.cs @@ -31,6 +31,37 @@ public void Dispatcher_Shutdown_BeforeRunIsReached_RunReturnsWithoutStartingMain 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() { diff --git a/src/Core/UI/Dispatcher.cs b/src/Core/UI/Dispatcher.cs index bc0516a95e..f16ee74b8d 100644 --- a/src/Core/UI/Dispatcher.cs +++ b/src/Core/UI/Dispatcher.cs @@ -236,25 +236,37 @@ 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; } - // Park cheaply until the main thread is actually needed. An invocation that - // never shows UI and never talks to the macOS broker must not pay to start - // the main loop. - if (!WaitForWork()) + try { - // We were shut down before any work arrived. - return; - } + // Park cheaply until the main thread is actually needed. An invocation + // that never shows UI and never talks to the macOS broker must not pay + // to start the main loop. + if (!WaitForWork()) + { + // We were shut down before any work arrived. + return; + } - RunMainLoop(); + RunMainLoop(); + } + finally + { + lock (_queue) + { + _state = State.Stopped; + } + } } public void Shutdown() From db35b229dabb74e722e92ee6f672411589782ef8 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 17 Sep 2026 12:35:39 +0100 Subject: [PATCH 8/8] 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 the hand-over flips a flag and drains the queue under one lock, why two collections track work, 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 what moves where during the hand-over. Include the rules a caller needs to follow, since the cost of getting them wrong is a hang rather than an obvious failure. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham --- docs/architecture.md | 6 ++ docs/dispatcher.md | 228 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 234 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..a3bde9984d --- /dev/null +++ b/docs/dispatcher.md @@ -0,0 +1,228 @@ +# 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. + +It also extends somewhere less obvious. 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 on the job queue
no UI framework started + App->>Main: InvokeAsync(work) - the first job + Note over Main: IMainLoop.Initialize()
hand over and drain the queue
IMainLoop.Run(token) owns the thread + 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. + +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 + +All of these are guarded by the lock taken on `_queue`. + +Name|Purpose +-|- +`_queue`|Jobs accepted before the main loop is running. Drained into the main loop at hand-over. Also serves as the monitor for parking and for every state change. +`_outstandingJobs`|Every accepted job, from acceptance until its callback finishes. This is what makes a main loop failure recoverable - see [Failure handling](#failure-handling). +`_isMainLoopRunning`|Whether `AddJob` should post to the main loop rather than enqueue. +`_mainLoopFault`|The first fault seen, if any. Once set, the dispatcher is permanently unusable. +`_state`|See [States](#states). + +The main loop has its own separate queue - Avalonia's dispatcher queue - which +the dispatcher can only add to, via `IMainLoop.Post`. + +## 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. + +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 unwrap the nested +task, so the result always tracks the work to completion. + +> **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. + +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 (_queue)"} + C -->|"state is Stopping or Stopped"| D["throw InvalidOperationException"] + C -->|"_mainLoopFault is set"| E["job.Fail(fault)"] + C -->|"main loop not running"| F["_queue.Enqueue(job)
Monitor.Pulse(_queue)"] + C -->|"main loop running"| G["IMainLoop.Post(job)"] +``` + +The three accepting paths also add the job to `_outstandingJobs`. Completing 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. + +## Hand-over + +The hand-over is the delicate part. It must not leave a job sitting in a queue +that nobody will ever pump again. + +```mermaid +flowchart TD + A["Run() - state is Started"] --> B["WaitForWork()
parked until the first job arrives"] + B --> C["RunMainLoop()"] + C --> D["IMainLoop.Initialize()
the loop queue now exists,
but nothing is pumping it yet"] + D --> E["lock (_queue)
_isMainLoopRunning = true
pending = _queue.ToArray()
_queue.Clear()"] + E --> F["post each pending job
dequeue order, so FIFO is preserved"] + F --> G["IMainLoop.Run(token)
owns the thread until shutdown"] + G --> H["only now do the posted jobs run"] +``` + +Flipping `_isMainLoopRunning` and draining `_queue` under a single lock closes +the window in which a concurrent `AddJob` could enqueue into a queue that has +already been drained. + + |before hand-over|after hand-over +-|-|- +`_queue`|`[j1] [j2] [j3]`|empty +`_outstandingJobs`|`{j1, j2, j3}`|`{j1, j2, j3}` - unchanged +main loop queue|not started|`[j1] [j2] [j3]` + +Jobs stay in `_outstandingJobs` across the hand-over. They are removed only once +their callback has actually run. + +Note the ordering guarantee this buys. A job cannot run until `IMainLoop.Run` is +pumping, and on macOS that 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. + +## 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` - which covers +work still in `_queue` *and* work already handed to the main loop - records the +fault, and fails any later arrivals immediately. It is idempotent: the first +fault wins, so concurrent failures report a single, consistent cause. + +Two further details matter: + +- 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. + +## Shutdown + +`Shutdown()` may be called from any thread. It moves to `Stopping`, wakes +anything parked, and 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.** Its tasks never complete. This +is deliberate: `Shutdown()` is called only once the application has finished +everything it cares about, so anything still in flight is fire-and-forget by +definition. Waiting for it 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]. +- **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. +- **Never wait on a dispatcher task from inside a job.** The continuation needs + the very loop that the job itself is occupying. +- `CheckAccess()` and `VerifyAccess()` report whether the calling thread is the + dispatcher thread, which is useful for avoiding a needless round trip. + +## Testing + +`IMainLoop` is an internal seam with a single production implementation, +`AvaloniaMainLoop`. An internal `Dispatcher.Initialize(IMainLoop)` overload lets +tests substitute a fake, so hand-over, failure, and shutdown behaviour can be +exercised without a real UI framework and without a real thread 1. See +[`DispatcherTests`][dispatcher-tests]. + +[dispatcher-tests]: ../src/Core.Tests/UI/DispatcherTests.cs +[entra-public-client]: ../src/Core/Authentication/Entra/EntraAuthentication.PublicClient.cs