Synchronize async view rendering#5791
Open
stefanhaller wants to merge 17 commits into
Open
Conversation
The closures that render static content to a main view (newStringTask and friends) ran on the ViewBufferManager's task goroutine, calling SetViewContent/SetOrigin/ResetViewOrigin directly on the view. Those touch view state (the line buffer, hover cells, the origin) that the UI thread concurrently reads and mutates while laying out and drawing, so they raced it -- e.g. a string task's SetContent clearing the view's lines while the UI thread's CopyContent read them, or its SetOrigin racing the layout's OriginY read. Bounce the whole closure onto the UI thread instead, so the view is only touched there. The bounce blocks (OnUIThreadAndWaitBackground) so the task still completes only once the content has actually been rendered, which the integration-test idle detection relies on; the background variant keeps it from counting towards the app being busy, matching the existing treatment of view rendering as work that must not block a repo switch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a task renders different content to a view (a new task key), the view's scroll origin is reset to the top via onNewKey. That ran on the task's own goroutine, racing the UI thread, which reads the origin (OriginY) while laying out and drawing the view -- the single largest source of view-render data races. Give ViewBufferManager a bounce primitive (onUIThread) that runs a function on the UI thread and waits for it, and reset the origin through it. This is the first use of the primitive; subsequent commits route the rest of the task's view mutations through it too, so that the view is only ever touched on the UI thread. It runs as background work (OnUIThreadAndWaitBackground) so rendering doesn't count towards the app being busy, matching how the render's gocui task is already created. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The readLines channel, by which a running task is told to read more lines as the user scrolls, is swapped out as tasks start and finish. It was a plain field written from the task goroutines (when a task starts, ends, or is replaced) and read from the UI thread in ReadLines/ ReadToEnd, so those accesses raced -- a longstanding data race (and a plausible cause of the occasional "main view stops updating" hang, since a torn read there could drop a scroll's read request). Make the field an atomic.Pointer and give the running task a captured local copy of the channel for its own send/receive, so the field itself is only ever loaded/stored atomically. No lock is involved, so there's nothing to untangle later. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The throttle flag is set from the goroutine that watches a task for being stopped, and read when the next task starts up -- two different goroutines, so the plain bool field was a data race. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A command task streams its output into a view from its own goroutine. To track soft-wraps (so cursor-positioning escapes from a pager land on the right line) the write path read the view's live InnerWidth, and the pty setup read its InnerSize -- both off the UI thread, racing the UI thread mutating the view's dimensions during layout. Capture the width on the UI thread instead and hand it to the task: the escape interpreter keeps a screenColMax it reads from, seeded in NewView and refreshed per render via View.SetContentWidth (called from newCmdTask/newPtyTask before the task's goroutine starts), and the pty size is computed in the after-layout callback rather than in the task's start func. The view's dimensions stay UI-thread-only; the task uses the snapshot rather than reading them live. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A view's line buffer, its viewLines/tainted flags, and its hover state are all written from the command-task goroutine (under writeMutex) as it renders. But three accessors reached that same state from the UI thread without the lock: SetView and the GUI-resize path cleared a view's lines directly, viewsToRedrawContentOnly read the tainted flag, and Buffer read the line buffer. Each raced a rendering task. Guard them with writeMutex, matching the view's other buffer accessors. These are reads/clears of state writeMutex already protects, not new callers of it -- the view's geometry stays outside the mutex. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a command task reaches EOF it runs onEndOfInput, which reads the view's line height (and thus its dimensions) to decide whether to scroll, sets the view's origin, and flushes stale cells. Reading the dimensions and setting the origin are UI-thread-only, but this ran on the task's own goroutine, racing the UI thread. Bounce onEndOfInput onto the UI thread, as we already do for the new-task origin reset. It's once per render, so it doesn't add the per-line UI-thread churn that streaming the content would. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Raising a popup or menu pushes a context and mutates the popup views, so it must happen on the UI thread. But it can be triggered from a worker goroutine — for example a WithWaitingStatus handler that hits a merge conflict and calls PromptForConflictHandling, or a worker that shows a confirmation — where it raced the UI thread's layout and draw code. Bounce the creation onto the UI thread at the one point where the popup and menu producers are injected into the popup handler, so every caller stays oblivious to the threading. For a caller that is already on the UI thread this adds no delay: the main loop drains the enqueued closure in the same event-processing cycle, before it draws. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PopupMutex guarded CurrentPopupOpts against a popup being created on a worker goroutine while the UI thread deactivated it, or reset it on a repo switch. Now that popup and menu creation is bounced onto the UI thread, every access to CurrentPopupOpts — create, deactivate, and the reset-on-switch (which already runs on the UI thread) — happens on the one goroutine, so the mutex protects nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test harness enqueued ErrQuit after a test finished, waited for the program to go idle, then slept a fixed second and declared "gocui should have already exited" if it hadn't. That fixed grace is fragile: under the race detector the shutdown legitimately takes longer than a second, so nearly every test failed with that message even though nothing was wrong. Wait for the main loop to actually return instead. gocui now closes a loopExited channel when MainLoop exits, and the harness blocks on it; the existing 40s watchdog still fails a test whose loop genuinely never quits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The custom-patch git operations (move/pull/delete patch, and their rebase continuations) run on worker goroutines and call PatchBuilder.Reset when they've consumed the patch, clearing To and the fileInfoMap. Meanwhile the UI thread reads that state every layout — the options bar and the mode indicator both call Active() — so the reset raced the render. Add a mutex. The map's entries are only ever touched on the UI thread, so the lock only has to serialize the To field and the fileInfoMap pointer: readers snapshot the pointer under the lock and iterate the local, and getFileInfo drops the lock across its git diff I/O rather than holding it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The patch-building scope ran RefreshPatchBuildingPanel directly on the refresh worker, where it read the commit-files selection and set the patch view's origin off the UI thread — the latter raced the UI thread's draw. Bounce it onto the UI thread, exactly as the staging panel just above already does, guarded on the generation so a repo switch drops it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
moveMainContextToTop copies the current top view's content into the view
it's promoting, to avoid a flicker. The source can be a main view with a
live streaming task (e.g. resolving a conflict promotes the merge-conflicts
view over a main view that's mid-diff), and CopyContent both read and
published that source's buffer unsafely:
- it read the source's lines/viewLines while locking only the
destination, racing the task's concurrent Write; and
- it aliased the source's row slices into the destination, so the
source's ongoing appends (growslice reading the shared array) and
refreshViewLinesIfNeeded's in-place wrapping-cache writes (&lines[i])
kept racing this view's rendering after the copy.
Lock the source for the read, and shallow-clone the row slices so the
destination gets its own arrays. The per-row cell data is immutable once
written, so it stays shared -- the clone cost is proportional to the
number of rows, not their contents.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dropping a range of stashes ran a refresh after each drop. A refresh issued from the UI thread does its git work on a worker and applies the model update in the background, so firing one per iteration let the workers race: an earlier drop's refresh (which read a stash list that still contained a later-dropped entry) could apply its result last, leaving the stash view showing an entry that git had already removed. Refresh once, after all the drops, so a single worker reads the final stash list. The indices are captured up front and dropped highest-first, so the remaining lower indices stay valid without an intervening refresh. It's also cheaper: one `git stash list` instead of one per entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
stefanhaller
added
the
maintenance
stefanhaller
force-pushed
the
synchronize-async-view-rendering
branch
from
45b8511 to
2afc0ec
Compare
RefreshSuggestions dispatched to an AsyncHandler worker that read State.FindSuggestions and the prompt's TextArea (via GetPromptInput) from the worker goroutine. The main thread rewrites both in preparePromptPanel when it (re)creates a prompt panel, so an in-flight suggestions worker races those writes -- two data races surfaced under -race (filter_by_path/reword_commit_in_filtering_mode). Capture both on the UI thread (RefreshSuggestions is only ever called from UI-thread handlers) before dispatching to the worker. This is also more correct: we search for the input as it was when dispatched, which is what this request's AsyncHandler id corresponds to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
stefanhaller
force-pushed
the
synchronize-async-view-rendering
branch
from
2afc0ec to
cc923b4
Compare
This was old code that was supposed to make a race less likely, but now that we capture model stuff on the UI thread we don't need it any more.
Now that we solved all known concurrency issues and our tests should be 100% deterministic, reduce MaxAttempts to 1 so that tests fail immediately. We don't want to paper over existing flakiness any more.
stefanhaller
force-pushed
the
synchronize-async-view-rendering
branch
from
cc923b4 to
0020dbe
Compare
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.
Several fixes for the last concurrency problems we still had: we used to access view properties on worker threads, which isn't allowed (writing to a view itself is fine and guarded by a mutex, but other fields are not, and even that mutex had a few holes that are plugged here).
With this, our integration test suite seems to be fully deterministic and race-free, so we also remove the retry safety net (MaxAttempts=2) that we were using to address flakiness.