[WIP] Make sure TryGetGlyphTypeface does not fail for concurrent access#21269
[WIP] Make sure TryGetGlyphTypeface does not fail for concurrent access#21269Gillibald wants to merge 2 commits intoAvaloniaUI:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This WIP PR aims to prevent FontManager.TryGetGlyphTypeface failures under concurrent access by making font collection retrieval/creation more concurrency-safe, and adds a unit test that exercises concurrent embedded-font lookups.
Changes:
- Update
FontManager.TryGetFontCollectionto use_fontCollections.GetOrAdd(...)when retrieving/creating system and embedded font collections. - Add a new unit test that repeatedly forces embedded font collection recreation and performs concurrent
TryGetGlyphTypefacecalls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Avalonia.Base/Media/FontManager.cs |
Switches font collection caching logic to ConcurrentDictionary.GetOrAdd for concurrent access. |
tests/Avalonia.Base.UnitTests/Media/FontManagerTests.cs |
Adds a concurrency regression test for embedded font lookups. |
| fontCollection = _fontCollections.GetOrAdd(source, static (key, platformImpl) => | ||
| { | ||
| if (source == SystemFontsKey) | ||
| if (key == SystemFontsKey) | ||
| { | ||
| fontCollection = new SystemFontCollection(PlatformImpl); | ||
| } | ||
| else | ||
| { | ||
| if (source.IsAbsoluteResm() || source.IsAvares()) | ||
| { | ||
| fontCollection = new EmbeddedFontCollection(source, source); | ||
| } | ||
| return new SystemFontCollection(platformImpl); | ||
| } | ||
|
|
||
| if (fontCollection != null) | ||
| if (key.IsAbsoluteResm() || key.IsAvares()) | ||
| { | ||
| return _fontCollections.TryAdd(fontCollection.Key, fontCollection); | ||
| return new EmbeddedFontCollection(key, key); | ||
| } | ||
| } | ||
|
|
||
| return null!; | ||
| }, PlatformImpl); |
There was a problem hiding this comment.
ConcurrentDictionary<Uri, IFontCollection> does not allow null values. Returning null! from the GetOrAdd valueFactory will cause GetOrAdd to throw (e.g., when source is a fonts: key that hasn’t been registered, or any other unsupported scheme). Consider handling unsupported schemes by returning false without calling GetOrAdd (or by using TryGetValue for non-embedded collections), and only using GetOrAdd for sources you can always create a non-null collection for (system fonts / embedded fonts).
| var fontCollections = (ConcurrentDictionary<Uri, IFontCollection>) | ||
| typeof(FontManager) | ||
| .GetField("_fontCollections", BindingFlags.NonPublic | BindingFlags.Instance)! | ||
| .GetValue(fontManager)!; |
There was a problem hiding this comment.
This test reaches into FontManager internals via reflection to clear the cache. Since FontManager already exposes RemoveFontCollection(Uri key), using that API would make the test less brittle (field name/type changes won’t break it) and also aligns with production usage.
Co-authored-by: Copilot <[email protected]>
MrJul
added
bug
area-textprocessing
backport-candidate-12.0.x
What does the pull request do?
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues