Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concurrency checks in UnitValueAbbreviationLookup no longer working correctly #1303

Closed
oysteinkrog opened this issue Aug 11, 2023 · 1 comment · Fixed by #1305
Closed

Concurrency checks in UnitValueAbbreviationLookup no longer working correctly #1303

oysteinkrog opened this issue Aug 11, 2023 · 1 comment · Fixed by #1305
Labels

Comments

@oysteinkrog
Copy link

oysteinkrog commented Aug 11, 2023

Describe the bug
The concurrency checks in UnitAbbreviationsCache are no longer working correctly.
Specificially, we sometimes see this error in our tests:

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)
	   at UnitsNet.UnitAbbreviationsCache.GetDefaultAbbreviation(Type unitType, Int32 unitValue, IFormatProvider formatProvider)
	   at UnitsNet.UnitFormatter.GetFormatArgs[TUnitType](TUnitType unit, Double value, IFormatProvider culture, IEnumerable`1 args)
	   at UnitsNet.QuantityFormatter.ToStringWithSignificantDigitsAfterRadix[TUnitType](IQuantity`1 quantity, IFormatProvider formatProvider, Int32 number)
	   at UnitsNet.QuantityFormatter.FormatUntrimmed[TUnitType](IQuantity`1 quantity, String format, IFormatProvider formatProvider)
	   at UnitsNet.QuantityFormatter.Format[TUnitType](IQuantity`1 quantity, String format, IFormatProvider formatProvider)
	   at UnitsNet.Mass.ToString(String format, IFormatProvider provider)
	   at UnitsNet.Mass.ToString(String format)
	   at UnitsNet.Mass.ToString()

To Reproduce
Set up a concurrency situation (multiple threads) with e.g. UnitsNetSetup.Default.UnitAbbreviations.GetDefaultAbbreviation(unit, culture)

Expected behavior
Should never throw Exception.

Additional context
I see from the history in this file that at one point a ConcurrencyDictionary was used, but that it was later removed.
Perhaps re-introduce this, or simply add correct locking around access to the AbbreviationsMap object.

angularsen added a commit that referenced this issue Aug 12, 2023
Fixes #1303

Change back to `ConcurrencyDictionary`, there was a subtle write-on-read when loading the initial values that could throw.

```
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)
```
angularsen added a commit that referenced this issue Aug 12, 2023
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)
```
@angularsen
Copy link
Owner

I was just checking what dev team would first catch this subtle racing condition 🧐 Well done! 🥳

Just kidding, nice catch. This part of the code has had quite a bit of refactoring lately.
I wasn't able to write a unit test that reproduced this case, could you give the fix a try?

#1305

Nuget should be out shortly.

Release UnitsNet/5.31.0 · angularsen/UnitsNet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants