Add Trace2 instrumentation to Entra authentication - #2450
Draft
mjcheetham wants to merge 10 commits into
Draft
mjcheetham wants to merge 10 commits into
mjcheetham wants to merge 10 commits into
Conversation
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 <mjcheetham@outlook.com>
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 <mjcheetham@outlook.com>
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 <mjcheetham@outlook.com>
Passing an async lambda to InvokeAsync bound to the plain Func<T> 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 <mjcheetham@outlook.com>
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 <mjcheetham@outlook.com>
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 <mjcheetham@outlook.com>
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 <mjcheetham@outlook.com>
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 <mjcheetham@outlook.com>
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 <mjcheetham@outlook.com>
Entra has the most branching of any authentication path in GCM: broker or not, silent or interactive, and within interactive one of three modes - each selected by some combination of user setting, stored preference, platform support and runtime availability. When someone reports that authentication did something unexpected, the answer is almost always one of those decisions, and none of them left a trace that could be correlated with timings. Assisted-by: Claude Opus 5 Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requires #2448 be merged first!
Add Trace2-based instrumentation around all aspects of Entra authentication.
Also now ensure that the
AppMainthread is captured as a 'trace2' thread region - previously we didn't do since this we didn't attribute any work to the real main thread (we attributed Avalonia UI init to the first caller), but now we correctly attribute things like UI work to the main thread. This means we cannot 'lie' else Trace2 events do not get parented to the correct region/thread spans.