Skip to content

Android remove backward timezones #64028

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.

Sign up for GitHub

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

Merged
steveisok merged 13 commits into from
Jan 24, 2022
Merged

Android remove backward timezones #64028

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e08f971
[libraries][Android] Filter out backward timezone IDs using tzlookup …
mdh1418 Jan 19, 2022
7258e39
[tests] Add TimeZoneInfo Test to detect Backward TimeZones
mdh1418 Jan 20, 2022
3c86169
Address feedback
mdh1418 Jan 20, 2022
9f6adce
Address more feedback
mdh1418 Jan 20, 2022
602201f
[libraries][Android] Add isBackwards in lieu of removing timezone ids
mdh1418 Jan 20, 2022
86a56b8
Remove unneeded strings.resx additions
mdh1418 Jan 21, 2022
ec59f25
Temporarily show timezone displayNames in NoBackwardTimeZones
mdh1418 Jan 21, 2022
9da4754
Figure out backwards IDs on linux and maybe windows
mdh1418 Jan 21, 2022
2a96703
[libraries] ActiveIssue NoBackwardTimeZones for Linux Arm64
mdh1418 Jan 21, 2022
559df41
Cleanup
mdh1418 Jan 21, 2022
09e71d2
Skip test with TestPlatforms instead
mdh1418 Jan 21, 2022
b3b4845
Optimize further by not allocating a list for GetTimeZoneIDs
mdh1418 Jan 24, 2022
a65bc46
Fix indexing
mdh1418 Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2593,6 +2593,9 @@
<data name="InvalidOperation_TimerAlreadyClosed" xml:space="preserve">
<value>The Timer was already closed using an incompatible Dispose method.</value>
</data>
<data name="InvalidOperation_TimeZoneLookupParsing" xml:space="preserve">
<value>Internal Error in parsing tzlookup file "{1}".</value>
</data>
<data name="InvalidOperation_TypeCannotBeBoxed" xml:space="preserve">
<value>The given type cannot be boxed.</value>
</data>
Expand Down Expand Up @@ -3025,6 +3028,9 @@
<data name="OperationCanceled" xml:space="preserve">
<value>The operation was canceled.</value>
</data>
<data name="OutOfMemory_TimeZoneLookupParsing" xml:space="preserve">
<value>There is insufficient memory to allocate a buffer to read tzlookup file "{1}".</value>
</data>
<data name="Overflow_Byte" xml:space="preserve">
<value>Value was either too large or too small for an unsigned byte.</value>
</data>
Expand Down
87 changes: 85 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ private sealed class AndroidTzData
private int[] _lengths;
private string _tzFileDir;
private string _tzFilePath;
private bool _isFiltered;

private static string GetApexTimeDataRoot()
{
Expand Down Expand Up @@ -234,13 +235,94 @@ public AndroidTzData()
{
_tzFileDir = tzFileDir;
_tzFilePath = tzFilePath;
_isFiltered = false;
return;
}
}

throw new TimeZoneNotFoundException(SR.TimeZoneNotFound_ValidTimeZoneFileMissing);
}

// On some versions of Android, the tzdata file may still contain backward timezone ids.
// We attempt to use tzlookup.xml, which is available on some versions of Android to help
// validate non-backward timezone ids
// tzlookup.xml is an autogenerated file that contains timezone ids in this form:
//
// <timezones ianaversion="2019b">
// <countryzones>
// <country code="au" default="Australia/Sydney" everutc="n">
// <id alts="Australia/ACT,Australia/Canberra,Australia/NSW">Australia/Sydney</id>
// ...
// ...
// <id>Australia/Eucla</id>
// </country>
// <country ...>
// ...
// ...
// ...
// </country>
// </countryzones>
// </timezones>
//
// Once the timezone cache is populated with the IDs, we reference tzlookup id tags and
// remove IDs and their associated information (byteoffsets and lengths) from the current
// AndroidTzData instance.
private void FilterBackwardIDs(string lookupPath)
{
try
{
using (StreamReader sr = File.OpenText(lookupPath))
{
var tzLookupIDs = new HashSet<string>();
string? tzLookupLine;
while (sr.Peek() >= 0)
{
if (!(tzLookupLine = sr.ReadLine())!.TrimStart().StartsWith("<id"))
continue;

int idStart = tzLookupLine.IndexOf('>') + 1;
int idLength = tzLookupLine.LastIndexOf("</") - idStart;
if (idStart == 0 || idLength < 0)
{
// Either the start tag <id ... > or the end tag </id> are not found
continue;
}
string id = tzLookupLine.Substring(idStart, idLength);
tzLookupIDs.Add(id);
}
var cleanIDs = new List<string>();
var cleanOffsets = new List<int>();
var cleanLengths = new List<int>();
for (int i = 0; i < _ids.GetLength(0); ++i)
{
if (tzLookupIDs.Contains(_ids[i]))
{
cleanIDs.Add(_ids[i]);
cleanOffsets.Add(_byteOffsets[i]);
cleanLengths.Add(_lengths[i]);
}
}
_ids = cleanIDs.ToArray();
Copy link
Member

@tarekgh tarekgh Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ids = cleanIDs.ToArray();

The question here, if someone tries to manually create any of the backward zones (using FindSystemTimeZoneById) will fail. I think it will be better not to fail it. I am fine if you think otherwise.

Can we mark which Ids are backward and filter those only while enumerating the zones?

Copy link
Member Author

@mdh1418 mdh1418 Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, so instead of removing the backward timezones, we can instead create a parallel array of bools to track which are backward (where that array is updated by this function)?

Copy link
Member Author

@mdh1418 mdh1418 Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, now there is an _isBackwards boolean array that is created and set when the id's and related byteoffset and lengths information are set as well.

_byteOffsets = cleanOffsets.ToArray();
_lengths = cleanLengths.ToArray();
}
}
catch (FileNotFoundException)
Copy link
Member

@tarekgh tarekgh Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catch

we should catch all exceptions and then ignore the filtering all together without any throwing. Getting the duplicated display names would be better than throwing and causing app failures

Copy link
Member Author

@mdh1418 mdh1418 Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore the filtering altogether, as in should we not even try to filter if GetSystemTImeZones is called a second time?
Should we log a warning here, and what is the standard way for logging that in CoreLib?

Copy link
Member

@tarekgh tarekgh Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore the filtering altogether, as in should we not even try to filter if GetSystemTImeZones is called a second time?

Yes. We try the filtering once and cache the result to use for any subsequent calls. If filtering fails, ignore it moving forward and don't try it again. It is unlikely to succeed if it fail first time.

Should we log a warning here, and what is the standard way for logging that in CoreLib?

I don't think we log that. Maybe we can have assert in the debug builds instead.

Copy link
Member

@tarekgh tarekgh Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other note, we have the API TimeZoneInfo.ClearCachedData which clears all cached data. If this is called and we need to recreate the cache, we should do the filtering at that time.

Copy link
Member Author

@mdh1418 mdh1418 Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filtering is now based on an _isBackwards boolean array that is created and set when the IDs are populated. I believe the filtering still works after calling TimeZoneInfo.ClearCachedData

Copy link
Member

@tarekgh tarekgh Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the filtering still works after calling TimeZoneInfo.ClearCachedData

do you mean you recreate the _isBackwards array again or you are saying _isBackwards is created only once and used even after clearing the cache?

Here is a scenario you can validate:

  • Call FindSystemTimeZoneUsingId("America/Los_Angeles")
  • Call GetSystemTimeZones
  • Validate the behavior that backward zones are filtered
  • Clear the cache
  • Call GetSystemTimeZones
  • Validate the behavior one more time that backward zones are filtered

Copy link
Member Author

@mdh1418 mdh1418 Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its created whenever _ids are created (and _byteOffsets and _lengths),
Using

TimeZoneInfo LA = TimeZoneInfo.FindSystemTimeZoneById("America/Los_Angeles");
Console.WriteLine($"LA DisplayName: {LA.DisplayName}");
ReadOnlyCollection<TimeZoneInfo> tzCollection = TimeZoneInfo.GetSystemTimeZones();
HashSet<String> tzDisplayNames = new HashSet<String>();
foreach (TimeZoneInfo timezone in tzCollection)
{
    tzDisplayNames.Add(timezone.DisplayName);
}
Console.WriteLine($"{tzCollection.Count == tzDisplayNames.Count} tzCollection: {tzCollection.Count} tzDisplayNames: {tzDisplayNames.Count}");
TimeZoneInfo.ClearCachedData();
tzCollection = TimeZoneInfo.GetSystemTimeZones();
tzDisplayNames = new HashSet<String>();

foreach (TimeZoneInfo timezone in tzCollection)
{
    tzDisplayNames.Add(timezone.DisplayName);
}
Console.WriteLine($"{tzCollection.Count == tzDisplayNames.Count} tzCollection: {tzCollection.Count} tzDisplayNames: {tzDisplayNames.Count}");

Both evaluate to true

{
// Some versions of Android will not have tzlookup.xml, so just warn that there may be backward timezone IDs
}
catch (IOException)
{
throw new InvalidOperationException(SR.Format(SR.InvalidOperation_TimeZoneLookupParsing, lookupPath));
}
catch (OutOfMemoryException)
{
throw new OutOfMemoryException(SR.Format(SR.OutOfMemory_TimeZoneLookupParsing, lookupPath));
}

_isFiltered = true;
}

[MemberNotNullWhen(true, nameof(_ids))]
[MemberNotNullWhen(true, nameof(_byteOffsets))]
[MemberNotNullWhen(true, nameof(_lengths))]
Expand All @@ -258,7 +340,7 @@ private bool LoadData(string path)
}
return true;
}
catch {}
catch {} // TODO add warning

return false;
}
Expand Down Expand Up @@ -308,7 +390,6 @@ private void ReadIndex(Stream fs, int indexOffset, int dataOffset)
int indexSize = dataOffset - indexOffset;
const int entrySize = 52; // Data entry size
int entryCount = indexSize / entrySize;

_byteOffsets = new int[entryCount];
_ids = new string[entryCount];
_lengths = new int[entryCount];
Expand Down Expand Up @@ -372,6 +453,8 @@ private void LoadEntryAt(Stream fs, long position, out string id, out int byteOf

public string[] GetTimeZoneIds()
{
if (!_isFiltered)
FilterBackwardIDs(Path.Combine(_tzFileDir, "tzlookup.xml"));
return _ids;
}

Expand Down
13 changes: 13 additions & 0 deletions src/libraries/System.Runtime/tests/System/TimeZoneInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2921,6 +2921,19 @@ public static void AdjustmentRuleBaseUtcOffsetDeltaTest()
Assert.Equal(new TimeSpan(2, 0, 0), customTimeZone.GetUtcOffset(new DateTime(2021, 3, 10, 2, 0, 0)));
}

[Fact]
public static void NoBackwardTimeZones()
{
ReadOnlyCollection<TimeZoneInfo> tzCollection = TimeZoneInfo.GetSystemTimeZones();
HashSet<String> tzDisplayNames = new HashSet<String>();

foreach (TimeZoneInfo timezone in tzCollection)
{
tzDisplayNames.Add(timezone.DisplayName);
}
Assert.Equal(tzCollection.Count, tzDisplayNames.Count);
}

private static bool IsEnglishUILanguage => CultureInfo.CurrentUICulture.Name.Length == 0 || CultureInfo.CurrentUICulture.TwoLetterISOLanguageName == "en";

private static bool IsEnglishUILanguageAndRemoteExecutorSupported => IsEnglishUILanguage && RemoteExecutor.IsSupported;
Expand Down