Skip to content

Commit df2e62c

Browse files
authored
🐛Fix concurrency issue getting abbreviations (#1305)
Fixes #1303 Change back to `ConcurrencyDictionary`, there was a subtle write-on-read when loading the initial values that could throw in `UnitAbbreviationsCache.GetAbbreviations()`: ```cs if (!AbbreviationsMap.TryGetValue(key, out IReadOnlyList<string>? abbreviations)) AbbreviationsMap[key] = abbreviations = ReadAbbreviationsFromResourceFile(unitInfo.QuantityName, unitInfo.PluralName, culture); ``` ``` System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct. at System.Collections.Generic.Dictionary`2.FindValue(TKey key) at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value) at UnitsNet.UnitAbbreviationsCache.GetAbbreviations(UnitInfo unitInfo, IFormatProvider formatProvider) at UnitsNet.UnitAbbreviationsCache.TryGetUnitAbbreviations(Type unitType, Int32 unitValue, IFormatProvider formatProvider, String[]& abbreviations) at UnitsNet.UnitAbbreviationsCache.GetUnitAbbreviations(Type unitType, Int32 unitValue, IFormatProvider formatProvider) ```
1 parent 2dd76b8 commit df2e62c

File tree

1 file changed

+38
-28
lines changed

1 file changed

+38
-28
lines changed

UnitsNet/CustomCode/UnitAbbreviationsCache.cs

Lines changed: 38 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Copyright 2013 Andreas Gullberg Larsen ([email protected]). Maintained at https://github.com/angularsen/UnitsNet.
33

44
using System;
5+
using System.Collections.Concurrent;
56
using System.Collections.Generic;
67
using System.Diagnostics.CodeAnalysis;
78
using System.Globalization;
@@ -35,14 +36,12 @@ public sealed class UnitAbbreviationsCache
3536
[Obsolete("Use UnitsNetSetup.Default.UnitAbbreviations instead.")]
3637
public static UnitAbbreviationsCache Default => UnitsNetSetup.Default.UnitAbbreviations;
3738

38-
private readonly object _syncRoot = new();
39-
4039
private QuantityInfoLookup QuantityInfoLookup { get; }
4140

4241
/// <summary>
4342
/// Culture name to abbreviations. To add a custom default abbreviation, add to the beginning of the list.
4443
/// </summary>
45-
private IDictionary<AbbreviationMapKey, IReadOnlyList<string>> AbbreviationsMap { get; } = new Dictionary<AbbreviationMapKey, IReadOnlyList<string>>();
44+
private ConcurrentDictionary<AbbreviationMapKey, IReadOnlyList<string>> AbbreviationsMap { get; } = new();
4645

4746
/// <summary>
4847
/// Create an instance of the cache and load all the abbreviations defined in the library.
@@ -295,23 +294,25 @@ public IReadOnlyList<string> GetAllUnitAbbreviationsForQuantity(Type unitEnumTyp
295294
}
296295

297296
/// <summary>
298-
///
297+
/// Get all abbreviations for the given unit and culture.
299298
/// </summary>
300-
/// <param name="unitInfo"></param>
301-
/// <param name="formatProvider"></param>
302-
/// <returns></returns>
303-
/// <exception cref="ArgumentNullException"></exception>
299+
/// <param name="unitInfo">The unit.</param>
300+
/// <param name="formatProvider">The culture to get localized abbreviations for. Defaults to <see cref="CultureInfo.CurrentCulture"/>.</param>
301+
/// <returns>The list of abbreviations mapped for this unit. The first in the list is the primary abbreviation used by ToString().</returns>
302+
/// <exception cref="ArgumentNullException"><paramref name="unitInfo"/> was null.</exception>
304303
public IReadOnlyList<string> GetAbbreviations(UnitInfo unitInfo, IFormatProvider? formatProvider = null)
305304
{
305+
if (unitInfo == null) throw new ArgumentNullException(nameof(unitInfo));
306306
if (formatProvider is not CultureInfo)
307307
formatProvider = CultureInfo.CurrentCulture;
308308

309309
var culture = (CultureInfo)formatProvider;
310310
var cultureName = GetCultureNameOrEnglish(culture);
311311

312312
AbbreviationMapKey key = GetAbbreviationMapKey(unitInfo, cultureName);
313-
if (!AbbreviationsMap.TryGetValue(key, out IReadOnlyList<string>? abbreviations))
314-
AbbreviationsMap[key] = abbreviations = ReadAbbreviationsFromResourceFile(unitInfo.QuantityName, unitInfo.PluralName, culture);
313+
314+
IReadOnlyList<string> abbreviations = AbbreviationsMap.GetOrAdd(key,
315+
valueFactory: _ => ReadAbbreviationsFromResourceFile(unitInfo.QuantityName, unitInfo.PluralName, culture));
315316

316317
return abbreviations.Count == 0 && !culture.Equals(FallbackCulture)
317318
? GetAbbreviations(unitInfo, FallbackCulture)
@@ -324,36 +325,45 @@ public IReadOnlyList<string> GetAbbreviations(UnitInfo unitInfo, IFormatProvider
324325
/// <param name="unitInfo">The unit to add for.</param>
325326
/// <param name="formatProvider">The culture this abbreviation is for, defaults to <see cref="CultureInfo.CurrentCulture"/>.</param>
326327
/// <param name="setAsDefault">Whether to set as the primary/default unit abbreviation used by ToString().</param>
327-
/// <param name="abbreviations">One or more abbreviations to add.</param>
328+
/// <param name="newAbbreviations">One or more abbreviations to add.</param>
328329
private void AddAbbreviation(UnitInfo unitInfo, IFormatProvider? formatProvider, bool setAsDefault,
329-
params string[] abbreviations)
330+
params string[] newAbbreviations)
330331
{
331332
if (formatProvider is not CultureInfo)
332333
formatProvider = CultureInfo.CurrentCulture;
333334

334335
var culture = (CultureInfo)formatProvider;
335336
var cultureName = GetCultureNameOrEnglish(culture);
336337

337-
// Restrict concurrency on writes.
338-
// By using ConcurrencyDictionary and immutable IReadOnlyList instances, we don't need to lock on reads.
339-
lock(_syncRoot)
340-
{
341-
var currentAbbreviationsList = new List<string>(GetAbbreviations(unitInfo, culture));
338+
AbbreviationMapKey key = GetAbbreviationMapKey(unitInfo, cultureName);
342339

343-
foreach (var abbreviation in abbreviations)
340+
AbbreviationsMap.AddOrUpdate(key,
341+
addValueFactory: _ =>
344342
{
345-
if (!currentAbbreviationsList.Contains(abbreviation))
346-
{
347-
if (setAsDefault)
348-
currentAbbreviationsList.Insert(0, abbreviation);
349-
else
350-
currentAbbreviationsList.Add(abbreviation);
351-
}
352-
}
343+
List<string> bundledAbbreviations = ReadAbbreviationsFromResourceFile(unitInfo.QuantityName, unitInfo.PluralName, culture).ToList();
344+
return AddAbbreviationsToList(setAsDefault, bundledAbbreviations, newAbbreviations);
345+
},
346+
updateValueFactory: (_, existingReadOnlyList) => AddAbbreviationsToList(setAsDefault, existingReadOnlyList.ToList(), newAbbreviations));
347+
}
353348

354-
AbbreviationMapKey key = GetAbbreviationMapKey(unitInfo, cultureName);
355-
AbbreviationsMap[key] = currentAbbreviationsList.AsReadOnly();
349+
private static IReadOnlyList<string> AddAbbreviationsToList(bool setAsDefault, List<string> list, IEnumerable<string> abbreviations)
350+
{
351+
foreach (var newAbbrev in abbreviations)
352+
{
353+
if (setAsDefault)
354+
{
355+
// Remove if it already exists, then insert at beginning.
356+
// Normally only called with a single abbreviation.
357+
list.Remove(newAbbrev);
358+
list.Insert(0, newAbbrev);
359+
}
360+
else if (!list.Contains(newAbbrev))
361+
{
362+
list.Add(newAbbrev);
363+
}
356364
}
365+
366+
return list.AsReadOnly();
357367
}
358368

359369
private static AbbreviationMapKey GetAbbreviationMapKey(UnitInfo unitInfo, string cultureName)

0 commit comments

Comments
 (0)