Skip to content

Make FontRegistry's table of font data thread safe - #4266

Open
HeikoKlare wants to merge 1 commit into
eclipse-platform:masterfrom
HeikoKlare:fontregistry-multidisplay-step1c
Open

Make FontRegistry's table of font data thread safe#4266
HeikoKlare wants to merge 1 commit into
eclipse-platform:masterfrom
HeikoKlare:fontregistry-multidisplay-step1c

Conversation

@HeikoKlare

@HeikoKlare HeikoKlare commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

FontRegistry keeps its symbolic-name-to-FontData mapping in a plain HashMap that is read and written from arbitrary threads:

  • put(String, FontData[]) is public and carries no UI-thread restriction, unlike the methods handing out Font instances.
  • getFontData(), getDescriptor(), getKeySet() and hasValueFor() read it and are likewise unrestricted — getKeySet() even hands out a live view.
  • createFont() writes to it from whichever thread realizes a font, i.e. from any Display's thread.

Unsynchronized HashMap mutation from several threads can corrupt the table itself, not merely produce stale reads. Use a ConcurrentHashMap, so concurrent access is safe and getKeySet() returns a weakly-consistent view rather than one that may fail arbitrarily while being iterated.

Also folds the internal put()'s read-compare-write of that table into a single atomic Map#put: the previous get()/put() pair could interleave so that two threads both concluded the mapping was unchanged, or that the mapping they replaced had already been overwritten. Storing the given array when it is content-equal to the existing one is a no-op for every reader, so behaviour is unchanged.

The table of realized FontRecords and the stale-font list are deliberately left alone. They are not fully protected either — put() mutates them and carries no UI-thread restriction — but that race is pre-existing and unchanged here. It is addressed in follow-up changes that this one prepares for, where those structures get restructured anyway.

🤖 Generated with Claude Code

FontRegistry keeps its symbolic-name-to-FontData mapping in a plain
HashMap that is read and written from arbitrary threads:

- put(String, FontData[]) is public and carries no UI-thread
  restriction, unlike the methods that hand out Font instances,
- getFontData(), getDescriptor(), getKeySet() and hasValueFor() read it
  and are likewise unrestricted, with getKeySet() even handing out a
  live view of the table, and
- createFont() writes to it (via the internal put() overload) from
  whichever thread realizes a font, i.e. from any SWT Display's thread.

Unsynchronized HashMap mutation from several threads can corrupt the
table itself, not merely produce stale reads. Use a ConcurrentHashMap
instead, so concurrent access is safe and getKeySet() returns a
weakly-consistent view rather than one that may fail arbitrarily while
being iterated.

Also fold the internal put()'s read-compare-write of that table into a
single atomic Map#put: the previous get()/put() pair could interleave
so that two threads both concluded the mapping was unchanged, or that
the mapping they replaced was one that had already been overwritten.
Storing the given array when it is content-equal to the existing one is
a no-op for every reader, so this does not change behavior.

The table of realized FontRecords is deliberately left alone here: it
is guarded by the documented UI-thread restriction of the methods
returning Font instances.

Assisted-by: Claude Opus 5 <noreply@anthropic.com>
@HeikoKlare
HeikoKlare force-pushed the fontregistry-multidisplay-step1c branch from fc60749 to 81ae8d9 Compare August 21, 2026 15:08
@HeikoKlare
HeikoKlare requested a lite review from Copilot August 21, 2026 15:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes FontRegistry’s font-data mapping safe for concurrent access.

Changes:

  • Replaces HashMap with ConcurrentHashMap.
  • Uses atomic map replacement.
  • Preserves weakly consistent key-set iteration.
  • A critical issue remains: cache invalidation can still concurrently mutate unsynchronized font-record and stale-font structures.
Suppressed comments (2)

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java:821

  • This replaces an equal FontData[] with the caller's array, unlike the previous get-then-put sequence. Because getFontData() returns the stored array directly (line 612), a caller that retained and later mutates the previous array now mutates an orphaned array instead of the registry; array identity is also observable. Preserve the existing array when the contents are equal while retaining atomic update semantics.
		FontData[] existing = stringToFontData.put(symbolicName, fontData);

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/FontRegistry.java:199

  • ConcurrentHashMap rejects null keys, so this changes the existing public behavior of hasValueFor(null) (and getKeySet().contains(null)) from returning false with the old HashMap to throwing NullPointerException. Neither hasValueFor nor ResourceRegistry declares a non-null precondition, and put already prevents null keys, so these queries should retain the old false result by guarding null before querying the map.
	private final Map<String, FontData[]> stringToFontData = new ConcurrentHashMap<>(7);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results

   858 files  ±0     858 suites  ±0   48m 1s ⏱️ - 2m 32s
 8 172 tests ±0   7 929 ✅ +1  243 💤 ±0  0 ❌ ±0 
20 418 runs  ±0  19 762 ✅ +1  656 💤 ±0  0 ❌ ±0 

Results for commit 81ae8d9. ± Comparison against base commit 5b3e6dd.

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.

2 participants