Skip to content

Commit 50a22e9

Browse files
GillibaldCopilot
andauthored
[Text] Make sure TryGetGlyphTypeface does not fail for concurrent access (#21269)
* Make sure TryGetGlyphTypeface does not fail for concurrent access * Update tests/Avalonia.Base.UnitTests/Media/FontManagerTests.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update gitignore * Make TryGetFontCollection more robust and add unit tests * Introdcue GetOrCreateFontCollection helper for safe disposal of losing instances * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 85263da commit 50a22e9

4 files changed

Lines changed: 359 additions & 20 deletions

File tree

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,3 +221,6 @@ src/Browser/Avalonia.Browser/wwwroot
221221
api/diff
222222
src/Browser/Avalonia.Browser/staticwebassets
223223
.serena
224+
225+
# Claude agent worktrees
226+
.claude/worktrees/

src/Avalonia.Base/Media/FontManager.cs

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -351,36 +351,58 @@ internal IReadOnlyList<Typeface> GetFamilyTypefaces(FontFamily fontFamily)
351351
return [];
352352
}
353353

354-
private bool TryGetFontCollection(Uri source, [NotNullWhen(true)] out IFontCollection? fontCollection)
354+
internal bool TryGetFontCollection(Uri source, [NotNullWhen(true)] out IFontCollection? fontCollection)
355355
{
356356
Debug.Assert(source.IsAbsoluteUri);
357357

358-
if (source.Scheme == SystemFontScheme)
358+
// Both the systemfont: scheme and SystemFontsKey (fonts:SystemFonts) map to the system
359+
// font collection. SystemFontsKey is checked before the generic IsFontCollection branch
360+
// so that the SystemFontCollection is created on demand regardless of which URI form is used.
361+
if (source.Scheme == SystemFontScheme || source == SystemFontsKey)
359362
{
360-
source = SystemFontsKey;
363+
fontCollection = GetOrCreateFontCollection(SystemFontsKey, PlatformImpl,
364+
static (_, impl) => new SystemFontCollection(impl));
365+
return true;
361366
}
362367

363-
if (!_fontCollections.TryGetValue(source, out fontCollection))
368+
// Other fonts: URIs are only returned when they have been explicitly registered
369+
// via AddFontCollection — no implicit creation to avoid caching null for unknown keys.
370+
if (source.IsFontCollection())
364371
{
365-
if (source == SystemFontsKey)
366-
{
367-
fontCollection = new SystemFontCollection(PlatformImpl);
368-
}
369-
else
370-
{
371-
if (source.IsAbsoluteResm() || source.IsAvares())
372-
{
373-
fontCollection = new EmbeddedFontCollection(source, source);
374-
}
375-
}
372+
return _fontCollections.TryGetValue(source, out fontCollection);
373+
}
376374

377-
if (fontCollection != null)
378-
{
379-
return _fontCollections.TryAdd(fontCollection.Key, fontCollection);
380-
}
375+
if (source.IsAbsoluteResm() || source.IsAvares())
376+
{
377+
fontCollection = GetOrCreateFontCollection(source, 0,
378+
static (key, _) => new EmbeddedFontCollection(key, key));
379+
return true;
381380
}
382381

383-
return fontCollection != null;
382+
fontCollection = null;
383+
return false;
384+
}
385+
386+
/// <summary>
387+
/// Thread-safe get-or-create that disposes any candidate that loses the insertion race,
388+
/// preventing resource leaks that <see cref="ConcurrentDictionary{TKey,TValue}.GetOrAdd(TKey,Func{TKey,TValue})"/>
389+
/// can cause when the factory is invoked concurrently by multiple threads.
390+
/// </summary>
391+
private IFontCollection GetOrCreateFontCollection<TState>(Uri key, TState state, Func<Uri, TState, IFontCollection> factory)
392+
{
393+
if (_fontCollections.TryGetValue(key, out var existing))
394+
return existing;
395+
396+
var candidate = factory(key, state);
397+
398+
// GetOrAdd(key, value) atomically inserts or returns the existing value;
399+
// it never invokes a factory, so only one IFontCollection instance survives.
400+
var winner = _fontCollections.GetOrAdd(key, candidate);
401+
402+
if (!ReferenceEquals(winner, candidate))
403+
candidate.Dispose(); // Our candidate lost the race – dispose it to avoid the leak.
404+
405+
return winner;
384406
}
385407

386408
private string GetDefaultFontFamilyName(FontManagerOptions? options)

tests/Avalonia.Base.UnitTests/Media/FontManagerTests.cs

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
using System;
2+
using System.Threading;
3+
using System.Threading.Tasks;
24
using Avalonia.Media;
35
using Avalonia.UnitTests;
46
using Xunit;
@@ -86,5 +88,54 @@ public void Should_Return_First_Installed_Font_Family_Name_When_Default_Family_N
8688
Assert.Equal("DejaVu", FontManager.Current.DefaultFontFamily.Name);
8789
}
8890
}
91+
92+
[Fact]
93+
public async Task TryGetGlyphTypeface_Should_Be_Thread_Safe_For_Embedded_Fonts()
94+
{
95+
using (UnitTestApplication.Start(TestServices.MockPlatformRenderInterface))
96+
{
97+
var fontManager = FontManager.Current;
98+
99+
const string fontUri =
100+
"resm:Avalonia.Base.UnitTests.Assets?assembly=Avalonia.Base.UnitTests#Noto Mono";
101+
var collectionKey =
102+
new Uri("resm:Avalonia.Base.UnitTests.Assets?assembly=Avalonia.Base.UnitTests");
103+
104+
// Warm up to validate the font URI is correct.
105+
Assert.True(fontManager.TryGetGlyphTypeface(new Typeface(new FontFamily(fontUri)), out _));
106+
107+
const int iterations = 50;
108+
int failures = 0;
109+
110+
for (int i = 0; i < iterations; i++)
111+
{
112+
fontManager.RemoveFontCollection(collectionKey);
113+
114+
using var barrier = new Barrier(2);
115+
bool r1 = false, r2 = false;
116+
117+
var t1 = Task.Run(() =>
118+
{
119+
barrier.SignalAndWait();
120+
r1 = fontManager.TryGetGlyphTypeface(new Typeface(new FontFamily(fontUri)), out _);
121+
}, TestContext.Current.CancellationToken);
122+
123+
var t2 = Task.Run(() =>
124+
{
125+
barrier.SignalAndWait();
126+
r2 = fontManager.TryGetGlyphTypeface(new Typeface(new FontFamily(fontUri)), out _);
127+
}, TestContext.Current.CancellationToken);
128+
129+
await Task.WhenAll(t1, t2);
130+
131+
if (!r1 || !r2)
132+
{
133+
Interlocked.Increment(ref failures);
134+
}
135+
}
136+
137+
Assert.Equal(0, failures);
138+
}
139+
}
89140
}
90141
}

0 commit comments

Comments
 (0)