Type initialization for Culture fails if installed cultures contains invalid neutral ancestry

[General-Fault]

The Gu.Localization.Culture static initializer throws a KeyNotFoundException while setting the NeutralCultrureRegionMap if the computer has a neutral culture installed (or one incorrectly marked as neutral) without a corresponding specific culture.

I have not been able to determine the full steps to reproduce simply because I don't know how the clients computer got into this state. However I can describe the state and perhaps someone with more knowledge about configuring CultureInfo's on the system can share steps to create the state.

Essentially, the computer had two cultures marked as neutral "bal" and "bal-Arab". Perhaps "bal-Arab" should not be marked as neutral. When the static class Gu.Localization.Culture is initialized, it creates a NeutralCultureRegionMap by iterating the neutral cultures (in this case both "bal" and "bal-Arab") and creates "specific" cultures. So for the neutral "en" culture, it creates "en-US" as the specific culture, then it attempts to find that culture in Gu.Localization.Culture.NameCultureMap using the indexer. For the client computer, neither "bal" and "bal-Arab" neutral cultures have an associated specific culture.

private static readonly Dictionary<CultureInfo, RegionInfo> NeutralCultureRegionMap =
            AllCultures
                .Where(x => x.IsNeutralCulture)
                .Select(x => NameCultureMap[CultureInfo.CreateSpecificCulture(x.Name).Name])
                .Distinct(CultureInfoComparer.ByTwoLetterIsoLanguageName)
                .ToDictionary(
                    x => x.Parent,
                    x => CultureRegionMap[x],
                    CultureInfoComparer.ByTwoLetterIsoLanguageName);

This script

 [System.Globalization.CultureInfo]::GetCultures([System.Globalization.CultureTypes]::AllCultures) | ? {$_.Name.StartsWith("bal")} | fl *

Produced this output (with some irrelevant info striped):

Parent : bal
LCID : 8192
Name : bal-Arab
IetfLanguageTag : bal-Arab
DisplayName : Unknown Language (bal-Arab)
ThreeLetterWindowsLanguageName : ZZZ
TwoLetterISOLanguageName : bal
IsNeutralCulture : True
CultureTypes : NeutralCultures, InstalledWin32Cultures <-- Note incorrect CultureType
UseUserOverride : True
IsReadOnly : False

Parent : bal-Arab
LCID : 12288
Name : bal-Arab-001
DisplayName : Unknown Locale (bal-Arab-001)
TwoLetterISOLanguageName : bal
ThreeLetterWindowsLanguageName : ZZZ
IsNeutralCulture : False
CultureTypes : SpecificCultures, InstalledWin32Cultures
UseUserOverride : True
IsReadOnly : False

Parent :
LCID : 13312
Name : bal
DisplayName : Unknown Language (bal)
TwoLetterISOLanguageName : bal
ThreeLetterWindowsLanguageName : ZZZ
IsNeutralCulture : True
CultureTypes : NeutralCultures, InstalledWin32Cultures
UseUserOverride : True
IsReadOnly : False

When Gu.Localization is used in any way, this error will be thrown:

System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Gu.Localization.Culture.<>c.<.cctor>b__12_12(CultureInfo x)
   at System.Linq.Enumerable.WhereSelectArrayIterator`2.MoveNext()
   at System.Linq.Enumerable.<DistinctIterator>d__64`1.MoveNext()
   at System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer)
   at Gu.Localization.Culture..cctor() -- System.TypeInitializationException: The type initializer for 'Gu.Localization.Translator' threw an exception. ---> System.TypeInitializationException: The type initializer for 'Gu.Localization.Culture' threw an exception. ---> System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Gu.Localization.Culture.<>c.<.cctor>b__12_12(CultureInfo x)
   at System.Linq.Enumerable.WhereSelectArrayIterator`2.MoveNext()
   at System.Linq.Enumerable.<DistinctIterator>d__64`1.MoveNext()
   at System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer)
   at Gu.Localization.Culture..cctor()
   --- End of inner exception stack trace ---
   at Gu.Localization.Culture.TryGet(String name, CultureInfo& culture)
   at Gu.Localization.ResourceCultures.GetAllCultures(DirectoryInfo executingDirectory)
   at Gu.Localization.Translator.GetAllCultures()
   at Gu.Localization.Translator..cctor()
   --- End of inner exception stack trace ---
   at Gu.Localization.Translator.ContainsCulture(CultureInfo language)

[JohanLarsson]

Thanks for reporting! And what a beautiful issue!
I don't know this stuff well and it will be somewhat tricky if we don't have a good way to repro but I'll have a look.

[JohanLarsson]

I wrote a PR with some cargo culting. If you have time you can review if it makes any sense. Chances are it just pushes the problem to crop up later when running the app. Ideal case is if we could somehow repro and create an UI-test for it. That would be the only way to guard against future regressions.

[General-Fault]

Thank you @JohanLarsson.
I created a patch for my client with the odd localization. I was waiting for him to confirm that it works before submitting a PR. I really didn't anticipate such a swift response to this. Thank you.

The technique I took though was pretty quick and dirty (and slow).

        private static readonly Dictionary<CultureInfo, RegionInfo> NeutralCultureRegionMap =
            AllCultures
                .Where(x => x.IsNeutralCulture)
                .Select(x => CultureInfo.CreateSpecificCulture(x.Name))
                .Where(x => NameCultureMap.ContainsKey(x.Name))
                .Select(x => NameCultureMap[x.Name])
                .Distinct(CultureInfoComparer.ByTwoLetterIsoLanguageName)
                .Where(x => CultureRegionMap.ContainsKey(x))
                .ToDictionary(
                    x => x.Parent,
                    x => CultureRegionMap[x],
                    CultureInfoComparer.ByTwoLetterIsoLanguageName);

This works (I think) much like your take simply by not creating the NeutralCultureRegionMap entry for "broken" CultureInfos. Since The only thing using that collection is TryGetRegion, which returns false like I think it should here, this is probably good behavior. But what do you think about making the TryGet and TryGetRegion methods lazy-loading?
Then this scenario can be made more easily testable by making AllCultures writable.
I'll take a pass at it if you like the idea.

[JohanLarsson]

Which version do you prefer? Also about speed it is probably plenty fast enough as it is only run once at app startup.

[General-Fault]

I think you are right to add try/catch. Otherwise they are essentially the same. I'd say stick with your commit. It has the benefit of being done.
I'm sensitive to app startup since that's been a complaint about my app - though this is probably only a very small part of that load-up time.

Actually, what I think you really get from late loading is testability. Set the AllCultures in the test method and I think you can test this issue.

While playing around with the idea of lazy loading, I found what may be a redundancy in TryGet. Since TwoLetterISOLanguageNameCultureMap is created on a filtered list of AllCultures where Name == TwoLetterISOLanguageName, isn't TwoLetterISOLanguageNameCultureMap a subset of NameCultureMap? Tell me if I read that wrong... it's getting late... it's possible.

So I ended up with:

        internal static bool TryGet(string name, out CultureInfo culture)
        {
            if (name == null)
            {
                culture = null;
                return false;
            }

            if (NameCultureMap.TryGetValue(name, out culture)) return true;
            culture = AllCultures.FirstOrDefault(x => StringComparer.OrdinalIgnoreCase.Equals(x.Name, name));
            if (culture != null)
            {
                NameCultureMap.Add(culture.Name, culture);
                return true;
            }

            //I don't think this is necessary. The list is filtered by Name == TwoLetterISOLanguageName. So doesn't that make TwoLetterISOLanguageNameCultureMap a subset of NameCultureMap? And we already looked in there.
            //if (TwoLetterISOLanguageNameCultureMap.TryGetValue(name, out culture)) return true;
            //culture = AllCultures.FirstOrDefault(
            //    x => StringComparer.OrdinalIgnoreCase.Equals(x.Name, x.TwoLetterISOLanguageName)
            //         && StringComparer.OrdinalIgnoreCase.Equals(x.TwoLetterISOLanguageName, name));

            //if (culture != null)
            //{
            //    TwoLetterISOLanguageNameCultureMap.Add(culture.TwoLetterISOLanguageName, culture);
            //    return true;
            //}

            return false;
        }

[JohanLarsson]

I would expect the added startup time to be in the microseconds. We can run some benechmarks and optimize later if needed. Things can probably be made lazy there.

[General-Fault]

I think you are right about that.
Actually, what I think you really get from late loading is testability. If you can set the AllCultures in the test method before trying to fetch a region I think you can test this issue.

[JohanLarsson]

Ah, nice, very valid point. We should refactor to that so that we can write an UI-test that uses the cultures your client had. That way we can be sure to not have regressions in the future.

I don't remember the reason for all those maps to be honest, looks a bit like I had a stroke when reading the code.

[General-Fault]

I have a start on it. But getting late. I can submit a PR on it tomorrow evening, assuming I can replicate the invalid culture setup.

[JohanLarsson]

https://www.nuget.org/packages/Gu.Localization/6.4.3

Takes 15 minutes or so for nuget to index it.

[JohanLarsson]

If you decide to try writing UI-tests for it I think it is best to make a small separate app for this scenario that we use in the tests.

[General-Fault]

Thank you very much for publishing that so quickly!

Honestly, my initial thought was to only write a test method (probably in a new test fixture) that created an array of CultureInfos containing a neutral culture without a specific culture. Then try calling one of the methods on Translator that would cause the static Culture constructor to be executed. That would test this specific scenario, and you already have quite the nice testing infrastructure. I suspect you have something in mind that I'm missing... I'd like to be a thorough as I can.