Runtime: CultureInfo.ClearCachedData may not fully clear if called concurrently with GetCultureInfo

[stephentoub]

https://dev.azure.com/dnceng/public/_build/results?buildId=336890&view=ms.vss-test-web.build-test-results-tab&runId=9927766&resultId=102025&paneView=debug
netcoreapp-Windows_NT-Debug-x64-(Windows.Nano.1809.Amd64.Open)windows.10.amd64.serverrs
https://github.com/dotnet/corefx/pull/40808

    System.Globalization.Tests.CultureInfoAll.ClearCachedDataTest [FAIL]
      expected to get a new object reference
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/System.Globalization/tests/CultureInfo/CultureInfoAll.cs(637,0): at System.Globalization.Tests.CultureInfoAll.ClearCachedDataTest()

[tarekgh]

I don't know how this possible can fail.

[danmosemsft]

I was wondering whether a concurrent test could cause this because the caches are static. However I can only see how it can cause the reverse problem. If there was another test that did ci.ClearCachedData(); this would cause this test above to fail on the previous assert. Currently there is not such a test (and I don't see product code that calls it indirectly)

[tarekgh]

Yes, even if the tests run concurrently, I am not seeing any way this can happen, except if the instructions get reordered and clear cached ran after the assert. which I don't think this can happen either.

[danmosemsft]

@stephentoub any idea? we're assuming reads and writes on a single thread are ordered 😄

[stephentoub]

@stephentoub any idea?

From a quick look, it seems an interleaving like the following is possible:

1. The s_nameCachedCultures table has already been initialized but does not contain "jp-JP":
   https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Globalization/CultureInfo.cs#L138

2. Thread 1 calls CultureInfo.GetCultureInfo("jp-JP"), storing the value of s_nameCachedCultures into a temporary:
   https://github.com/dotnet/coreclr/blob/f23c07f692e4ce8cf6385afb7246b7cfbeff28ee/src/System.Private.CoreLib/shared/System/Globalization/CultureInfo.cs#L1107

3. Thread 1 then looks up "jp-JP" in tempNameHT and fails, so it doesn't return out.
   https://github.com/dotnet/coreclr/blob/f23c07f692e4ce8cf6385afb7246b7cfbeff28ee/src/System.Private.CoreLib/shared/System/Globalization/CultureInfo.cs#L1133

4. Thread 1 then constructs the new CultureInfo("jp-JP", false) and stores it into the dictionary:
   https://github.com/dotnet/coreclr/blob/f23c07f692e4ce8cf6385afb7246b7cfbeff28ee/src/System.Private.CoreLib/shared/System/Globalization/CultureInfo.cs#L1182

5. Just after doing so, Thread 2 calls CultureInfo.GetCurrentCulture("jp-JP"). It will successfully find the instance that Thread 1 just published to the dictionary, as Thread 1 stored it into tempNameHT, which is just an alias for the existing s_nameCachedCultures that Thread 2 will also access.

6. Thread 2 then calls ClearCachedData on the resulting CultureInfo, which will set s_nameCachedCultures to null, in theory clearing out the cache.

7. However, Thread 1 then continues running, and before exiting, stores its tempNameHT, which was the original s_nameCachedCultures from before it was cleared, back into s_nameCachedCultures, effectively undoing the clear:
   https://github.com/dotnet/coreclr/blob/f23c07f692e4ce8cf6385afb7246b7cfbeff28ee/src/System.Private.CoreLib/shared/System/Globalization/CultureInfo.cs#L1236

8. Thread 2 then calls CultureInfo.GetCurrentCulture("jp-JP"), which will access the exact same dictionary that was there previously, and will thus get back the same instance.

I can repro the failure within 10 seconds or so running this code:
```C#
using System;
using System.Globalization;
using System.Threading;

class Program
{
static void Main()
{
ThreadPool.QueueUserWorkItem(_ =>
{
while (true)
{
CultureInfo ci = CultureInfo.GetCultureInfo("ja-JP");
ci.ClearCachedData();
if (ReferenceEquals(ci, CultureInfo.GetCultureInfo("ja-JP")))
{
Environment.FailFast("uh oh");
}
}
});

    ThreadPool.QueueUserWorkItem(_ =>
    {
        while (true) CultureInfo.GetCultureInfo("ja-JP");
    });

    ThreadPool.QueueUserWorkItem(_ =>
    {
        while (true) CultureInfo.GetCultureInfo("en-US");
    });

    Console.ReadLine();
}

}
```

[danmosemsft]

Yes. Masterful analysis!

[ViktorHofer]

You probably don't need more data but just in case: https://dnceng.visualstudio.com/public/_build/results?buildId=338743&view=ms.vss-test-web.build-test-results-tab&runId=9993546&resultId=112589&paneView=debug

[danmosemsft]

Yay for a flaky test caused by actual product issue!