Skip to content

Fix data race in MigraDoc FontHandler's single-entry font cache - #382

Open
tgburgin wants to merge 1 commit into
empira:masterfrom
tgburgin:fix/fonthandler-cache-race
Open

Fix data race in MigraDoc FontHandler's single-entry font cache#382
tgburgin wants to merge 1 commit into
empira:masterfrom
tgburgin:fix/fonthandler-cache-race

Conversation

@tgburgin

Copy link
Copy Markdown

Fixes #381.

FontHandler.FontToXFont memoises the last (Font, XFont) pair in two independent statics written without synchronisation, so a reader can confirm _lastFont is its own Font and then read an _lastXFont that another thread has already replaced. The method then returns an XFont belonging to a different Font, usually at a different size. Nothing throws — the document is silently typeset wrong, on measurement (wrong widths, wrong wrapping) or on drawing (a Tf at 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 FontCacheEntry and published by a single reference assignment to a volatile field. A reader takes one snapshot of that field, so it sees either the complete previous entry or the complete new one — never the Font from one and the XFont from another.

var entry = _lastEntry;
if (entry != null &&
    entry.FontRef.TryGetTarget(out var lastFont) && font == lastFont &&
    entry.XFontRef.TryGetTarget(out var lastXFont))
    return lastXFont;

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. volatile costs nothing on x64 reads. A Monitor would also work — and Locks.EnterFontManagement() already exists next door in PdfSharp.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

  • Single-entry cache. Not widened to a dictionary — that is a different decision with different memory behaviour, and this PR is a correctness fix.
  • WeakReference semantics, including the FORCE_MEMORYLEAK conditional, exactly as they were.
  • Allocation count. A miss allocated two 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:

tears / 400,000
two independent statics (current) 104,457
single published entry (this PR) 0

Against the real FontToXFont by reflection with two stable Font instances at 9 pt and 6.5 pt, 8 threads × 200,000 calls, the current code tears in both directions — asked 6.5pt, returned 9pt and asked 9pt, returned 6.5pt. (Fewer absolute tears there only because XFont construction 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MigraDoc FontHandler's single-entry font cache is racy: concurrent rendering silently returns the wrong font

1 participant