A mark list handed to a reader must not change under them - #157
Open
JohnCampionJr wants to merge 1 commit into
Open
A mark list handed to a reader must not change under them#157JohnCampionJr wants to merge 1 commit into
JohnCampionJr wants to merge 1 commit into
Conversation
BufferLine.Marks returned the live List<LineMark>, and AddMark appended to it. A host doing shell integration reads that list from its own thread — asking where the prompt is, what the last mark was — while the parser is still emitting marks on whatever thread drains the pty. Enumerating a List<T> across an append throws InvalidOperationException, and the reader crashes for having read a documented public property. Seen for real: an Avalonia host asking for the last mark on the line while a shell was starting up, on Windows CI, roughly one run in three. AddMark now publishes a new list instead of appending to the one already handed out, so a reader keeps a complete, consistent snapshot. The field is volatile: the reference swap is atomic either way, but without the release/acquire pair a reader on a weak memory model could see the new reference before the list it points at is fully built. Costs one small allocation per mark. Marks arrive a handful of times per command rather than per cell, so this is nowhere near the print path — and the copy is sized for its result, so the append cannot reallocate on top of it. The tests are deliberately thread-free. The property that fixes the race is "an append does not touch a list already handed out", which is exactly testable; both new tests fail on the old code, one with the production InvalidOperationException. Links, sized runs, placements and images have the same shape but are not the same change: those lists are edited in place by span splitting, not only appended to, and are read inside the render pass. Left alone deliberately.
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
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.
What
BufferLine.Marksreturns the liveList<LineMark>, andAddMarkappends to it. A host doing shell integration reads that list from its own thread — asking where the prompt is, what the last mark on a line was — while the parser is still emitting marks on whatever thread drains the pty. Enumerating aList<T>across an append throwsInvalidOperationException: Collection was modified, so the host crashes for having read a documented public property, and nothing it can do on its side prevents it.Seen for real: an Avalonia host asking for the last mark on the cursor line while a shell was starting up, on Windows CI, roughly one run in three. The same read is fine for thousands of runs when no mark happens to land in that window, which is what makes it nasty to attribute.
Fix
AddMarkpublishes a new list instead of appending to the one already handed out, so a reader keeps a complete, consistent snapshot of whatever was there when it asked._marksbecomesvolatile: the reference swap is atomic either way, but without the release/acquire pair a reader on a weak memory model (arm64, which is most of the Macs this runs on) could see the new reference before the list it points at is fully built.This is only safe to do because
_marksis genuinely append-only —AddMarkandClearMarksare the only writers, and nothing edits an element in place. I checked that before reaching for copy-on-write.Cost: one small allocation per mark. Marks arrive a handful of times per command, not per cell, so this is nowhere near the print path — and the copy is sized for its result, so the
Addthat follows cannot reallocate on top of it.Deliberately not included:
Links,SizedRuns,PlacementsandImageshave the same shape, but they are not the same change. Those lists are edited in place by span splitting rather than only appended to, so copy-on-write is not a mechanical translation, and they are read inside the render pass where the cost lands differently. Happy to look at them separately if you want them covered.Tests
Two added to
ShellIntegrationMarkAnchorTests, both thread-free on purpose. The property that fixes the race is "an append does not touch a list already handed out", and that is exactly testable; a threaded test would only reproduce sometimes and would pass on the broken code.A_mark_list_stays_enumerable_while_more_marks_arrive— fails onmainwith the productionInvalidOperationException, fromList<T>.Enumerator.MoveNext.A_mark_list_already_handed_out_does_not_grow— fails onmainwithAssert.Single() Failure: The collection contained 2 items.Both verified failing before the fix and passing after. Full suite: 2221 passed, 1 skipped.
🤖 Generated with Claude Code