Repository navigation
Fixes #5680. Begin() without Run() leaves MainLoopSyncContext installed, and Post() then queues continuations nothing drains - #5722
Conversation
Pump posted callbacks for Begin-only sessions while preserving safe thread-pool fallback and run-loop state. Restore synchronization context consistently for `Post` and `Send` while maintaining exception and blocking semantics. Add coverage for await continuations, UI events, thread affinity, and session cleanup.
BDisp
changed the title
Prevent posted work from starting nested rendering while inline output is settling or `LayoutAndDraw` is active. Add coverage ensuring captured await continuations remain pending until rendering completes and dispatches are explicitly drained.
Pass the current test cancellation token to four `Task.Run` continuations in `BeginEndTests.cs` while preserving existing toolkit continuation behavior.
Add tests verifying IsRunLoopRunning is false after initialization, remains true across nested run loops, and becomes false only after the final runnable exits.
Update `IApplication.Begin` and multitasking documentation to explain that Begin prepares a session without starting the continuous event loop. Document limited async continuation support during the first active Begin-only session and clarify that `Run` remains required for ongoing input and UI processing.
Add tests verifying callback exceptions propagate through `Send` and pending synchronous sends complete without deadlocking when the run loop stops. Include ANSI application setup, thread coordination, timeouts, assertions, and resource cleanup.
Assert timer processing starts on the worker thread and the callback is subsequently marshaled to the application’s main thread.
Cover `IsRunLoopRunning` reset after normal stop requests and exception-driven exits, including active-state verification during iteration.
Clarify queued dispatch ownership during session end: active sessions retain only owner work, while final teardown releases remaining callbacks to the thread pool. Add a 25-iteration concurrency test verifying SynchronizationContext.Send executes at most once, completes after shutdown, and leaves no queued dispatches.
…multole posted callbacks.
|
Hi @BDisp, thanks for working on this. I want to make sure I understand the design decision correctly. In the issue, we discussed that Post could fire on a worker thread, so running RunIteration there could end up processing input and drawing the UI off-thread — and had a few different ideas of how to handle that. This PR implements the RunIteration-from-Post approach. I had my LLM trace through PumpPostedWork and confirmed it does run the full iteration (including the Iteration event, layout/draw, and timers) on the posting thread, blocking it for ~40ms per frame. That's the thread-affinity problem we were trying to avoid. A few other questions:
I may be misreading the code or the intent. But I think we should revisit whether pumping on the posting thread is the right path. |
|
Thanks @harder. It was indeed a different approach than before, but with the same consequence. The iteration never actually happens in UIThread and it's not in accordance with the thread-safe protocol. |
context, so a Begin without Run left a context nothing drained. Posts from await continuations were queued and never ran. Move installation and restoration into Run: - Run saves the caller's context, installs the app's before Begin, and restores it in a finally, only if the app's context is still current. A nested Run saves the app's own context, so only the outermost Run restores the caller's. - Begin and End no longer read or write SynchronizationContext.Current. - Remove _callerSynchronizationContext, which was shared mutable state written on the Begin thread and read on whichever thread called End, and drop its use in ResetState. - Init creates the MainLoopSyncContext inside the _dispatchLock block, before Initialized = true, so a reader that sees Initialized also sees the context. Work posted from other threads still runs only on the UI thread, when it iterates. Leftover callbacks are still released to the thread pool when the final session ends (tui-cs#5636). Docs: update IApplication.Begin remarks, application.md and multitasking.md to say Begin starts no loop and installs no context, and to use Invoke to reach the UI thread after an await. Tests: add SyncContextLifecycleTests covering Begin without Run, Run with one runnable and nested runnables, an existing caller context, and restoration after an exception. Update tests that relied on Begin installing the context.
Pressing Esc while "Cancelable Load Items" was running ended the session without canceling the load. The await continuation then resumed on a thread-pool thread after shutdown and set the button text on a disposed view, throwing ObjectDisposedException (DEBUG_IDISPOSABLE). - Capture the app, button and list in locals at the start of CallLoadItemsAsync instead of reading fields after the await, which a later run could reassign. - Marshal the post-await UI updates through InvokeAsync bound to the session owner, as MethodAsync already does, and catch OperationCanceledException and NotInitializedException for the case where the session ended or the app was disposed first. - Create the CancellationTokenSource only when a load starts. The Cancel click now cancels the running load instead of replacing the source. - Pass the token to LoadItemsAsync and Task.Delay so a canceled load stops promptly. - Cancel the in-flight load when Run returns, since Esc and Quit end the session without pressing the button.
|
Hi @harder, thanks for tracing it, you read the old code correctly. Pumping on the posting thread was the wrong approach. Posts can come from any thread, so it ran input, layout and drawing off the UI thread, and it also had no gate against concurrent pumps. I've dropped it and followed the direction you suggested in the issue, so the PR now takes a different approach. What changed
Your questions
Behavior and docs Without I also found a related problem in the |
|
PR Review: #5722 – Fixes #5680. Begin() without Run() leaves MainLoopSyncContext installed, and Post() then queues continuations nothing drains Status: Draft (as of the latest visible state). Author (BDisp) previously marked an earlier approach as futile after review feedback and switched to a cleaner design. Problem
Solution (final direction in the PR)Move ambient context ownership entirely into
Strengths
Concerns / Suggestions
VerdictApprove once the PR is marked ready and the final commits are confirmed to contain only the “context owned by The design is the right one, the tests look comprehensive, and the docs make the contract explicit. The earlier dead-end of pumping from worker-thread |
…d must not sit on the toolkit queue forever; it falls back to the thread pool (tui-cs#5636 / tui-cs#5680).
|
Hi @BDisp, thanks for revisiting this. It's looking good. The revised approach addresses the earlier concerns. Having Run own the synchronization context, while Begin and End leave the caller’s context alone, looks correct. The worker-thread pump is gone, and the nested-run and exception-restoration behavior looks sound. I have a few smaller follow-ups from an AI code review:
These are focused cleanup requests; I don’t think another architectural change is needed. |
Fixes
Summary
PostandSend.Changes
ApplicationImpl.Dispatchto process posted callbacks correctly during Begin/End lifecycle.ApplicationImpl.Runto preserve expected state transitions.MainLoopSyncContextbehavior for context restoration and dispatch semantics.ApplicationImplBeginEndTestsfor await continuations, UI event thread affinity, invoked actions, and cleanup afterEnd.Testing
ApplicationTests.BeginEnd.ApplicationImplBeginEndTests.Await_After_Begin_Always_ResumesPull Request checklist:
CTRL-K-Dto automatically reformat your files before committing.dotnet testbefore commit///style comments)