Fix data race in MigraDoc FontHandler's single-entry font cache - #382
Open
tgburgin wants to merge 1 commit into
Open
Fix data race in MigraDoc FontHandler's single-entry font cache#382tgburgin wants to merge 1 commit into
tgburgin wants to merge 1 commit into
Conversation
FontToXFont memoises the last (Font, XFont) pair in two independent statics, _lastFont and _lastXFont, written without synchronisation. A reader can confirm _lastFont is its own Font and then read an _lastXFont that another thread has already replaced, so the method returns an XFont belonging to a different Font - typically a different size. Nothing throws; the document is silently typeset with the wrong font. Publish both references as one immutable entry assigned to a single volatile field instead, so a reader sees either the complete previous entry or the complete new one. Behaviour, allocation count and the cache-hit fast path are otherwise unchanged.
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.
Fixes #381.
FontHandler.FontToXFontmemoises the last(Font, XFont)pair in two independent statics written without synchronisation, so a reader can confirm_lastFontis its ownFontand then read an_lastXFontthat another thread has already replaced. The method then returns anXFontbelonging to a differentFont, usually at a different size. Nothing throws — the document is silently typeset wrong, on measurement (wrong widths, wrong wrapping) or on drawing (aTfat the wrong size mid-line). #381 has the full interleaving, the reproduction and the measurements.The change
Both references are now assigned in the constructor of one immutable
FontCacheEntryand published by a single reference assignment to avolatilefield. A reader takes one snapshot of that field, so it sees either the complete previous entry or the complete new one — never theFontfrom one and theXFontfrom another.One file, no public API change.
Why not a lock
The fast path stays lock-free, which matters because this is on every layout and every draw.
volatilecosts nothing on x64 reads. AMonitorwould also work — andLocks.EnterFontManagement()already exists next door inPdfSharp.Internal.Threading— but it would put a lock acquisition on the hottest path in the renderer for no benefit over an atomic publish.Preserved deliberately
WeakReferencesemantics, including theFORCE_MEMORYLEAKconditional, exactly as they were.WeakReferences before; it now allocates two plus one small entry object, only on a miss. A hit allocates nothing, as before.Verification
Isolating the cache shape and hammering both versions with two equally-hot keys, 8 threads, .NET 10 / Windows x64:
Against the real
FontToXFontby reflection with two stableFontinstances at 9 pt and 6.5 pt, 8 threads × 200,000 calls, the current code tears in both directions —asked 6.5pt, returned 9ptandasked 9pt, returned 6.5pt. (Fewer absolute tears there only becauseXFontconstruction dominates the loop.)No test added
A test for this is inherently probabilistic — the window is a few instructions wide — and I did not want to introduce a flaky test into your suite. The reproduction in #381 is self-contained if you would like one, and I am happy to add it in whatever form you prefer.