dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.95k stars 4.65k forks source link

Localization concerns with display names in TimeZoneInfo internal cache #50311

Open mattjohnsonpint opened 3 years ago

mattjohnsonpint commented 3 years ago

Consider the following program:

using System;
using System.Globalization;

class Program
{
    static void Main()
    {
        TimeZoneInfo tzi;

        CultureInfo.CurrentUICulture = CultureInfo.GetCultureInfo("en-US");
        tzi = TimeZoneInfo.FindSystemTimeZoneById("America/Los_Angeles");
        Console.WriteLine(tzi.StandardName); // "Pacific Standard Time"
        Console.WriteLine(tzi.DaylightName); // "Pacific Daylight Time"
        Console.WriteLine(tzi.DisplayName);  // "(UTC-08:00) Pacific Time (Los Angeles)"

        CultureInfo.CurrentUICulture = CultureInfo.GetCultureInfo("fr-FR");
        TimeZoneInfo.ClearCachedData();

        tzi = TimeZoneInfo.FindSystemTimeZoneById("America/Los_Angeles");
        Console.WriteLine(tzi.StandardName); // "heure normale du Pacifique nord-américain"
        Console.WriteLine(tzi.DaylightName); // "heure d’été du Pacifique"
        Console.WriteLine(tzi.DisplayName);  // "(UTC-08:00) heure du Pacifique nord-américain (Los Angeles)"
    }
}

This will work fine on Linux/OSX (but not Windows due to #50310). However there are a couple of problems:

I believe the internal cache of display names will need to be broken out per-language to resolve these concerns.

ghost commented 3 years ago

Tagging subscribers to this area: @tarekgh, @safern See info in area-owners.md if you want to be subscribed.

Issue Details
Consider the following program: ```csharp using System; using System.Globalization; class Program { static void Main() { TimeZoneInfo tzi; CultureInfo.CurrentUICulture = CultureInfo.GetCultureInfo("en-US"); tzi = TimeZoneInfo.FindSystemTimeZoneById("America/Los_Angeles"); Console.WriteLine(tzi.StandardName); // "Pacific Standard Time" Console.WriteLine(tzi.DaylightName); // "Pacific Daylight Time" Console.WriteLine(tzi.DisplayName); // "(UTC-08:00) Pacific Time (Los Angeles)" CultureInfo.CurrentUICulture = CultureInfo.GetCultureInfo("fr-FR"); TimeZoneInfo.ClearCachedData(); tzi = TimeZoneInfo.FindSystemTimeZoneById("America/Los_Angeles"); Console.WriteLine(tzi.StandardName); // "heure normale du Pacifique nord-américain" Console.WriteLine(tzi.DaylightName); // "heure d’été du Pacifique" Console.WriteLine(tzi.DisplayName); // "(UTC-08:00) heure du Pacifique nord-américain (Los Angeles)" } } ``` This will work fine on Linux/OSX (but not Windows due to #50310). However there are a couple of problems: - The need to call `ClearCachedData` to change the language is non-obvious. - An app might need to switch languages for different users, thus requiring the cache to be cleared often, thus defeating the benefit of a cache. - There could be more than one active thread with different users in different cultures simultaneously. Thus there is a chance that another thread could populate the cache with a different language, resulting in unexpected language in the above program if the timing is just right. - A call to `GetSystemTimeZones` iterates through all time zones, populating the cache for each one. But a call to `FindSystemTimeZoneById` only populates the cache for that one time zone. Thus at any given time, the cache might be in an inconsistent state - having entries belonging to more than one language. I believe the internal cache of display names will need to be broken out per-language to resolve these concerns.
Author: mattjohnsonpint
Assignees: -
Labels: `area-System.Globalization`, `untriaged`
Milestone: -
mattjohnsonpint commented 3 years ago

An alternate approach would be to remove the _displayName, _standardDisplayName, and _daylightDisplayName private fields from the TimeZoneInfo object. They would no longer be part of the state of the object and thus would no longer be part of the cache. The DisplayName, StandardName, and DaylightName properties would simply look up the correct localization at runtime based on the CurrentUICulture.

This would be a tradeoff between memory utilization of the cache and performance of the display string lookup. I believe it would be worthwhile though.

The only complication I can think of is how to deal with "custom" time zones, as built with TimeZoneInfo.CreateCustomTimeZone, where the names are passed in as parameters. The fields may have to remain (or put elsewhere) for that purpose only.

AntonLapounov commented 3 years ago

We could cache language-neutral @path,-stringID[;comment] resource descriptions instead and resolve them on demand using another cache. (That might be the same idea you mentioned above.)

mattjohnsonpint commented 3 years ago

Yes, that's a possibility, though it could load quite a bit into memory unnecessarily. I believe all the resource files are memory mapped, both in the ICU and in the NLS implementations. Thus, it would only be saving a tiny bit of CPU that's used for the lookup and concatenation pieces. I think the better idea is just to remove display names from the cache altogether.

mattjohnsonpint commented 3 years ago

@tarekgh - I'd like to get this in for 6.0. Ok to send a PR?

tarekgh commented 3 years ago

We need to fix https://github.com/dotnet/runtime/issues/55307 first before we do anything here. I expect we'll have some non-trivial change in handling the TZ names with that fix.