dotnet / runtime

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

[Android] Timezone APIs on Android #93216

Open Yqwed opened 1 year ago

Yqwed commented 1 year ago

This is a spin-off from https://github.com/mono/mono/pull/21682, as @mdh1418 suggested to do.

tl;dr:

  1. As I understand, current implementation reads Android's tzdata directly. Here are few problems with that: neither its location nor its structure is documented or guaranteed. I don't know what .NET's API surface is, but maybe ICU4C would suffice. Overall it endangers users as PR above illustrates.
  2. How does current .NET implementation handles TZif4 files?
  3. Do you have tests like this? I think old Mono implementation would fail on Asia/Gaza as it has predefined transition up to 2086 (does not fit into 32-bit section of TZif ver. 1)
  4. We usually make tzdata changes early in the release cycle. Can you include Android beta builds into testing matrix?

Happy to answer your questions!

Thanks, Almaz

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-globalization See info in area-owners.md if you want to be subscribed.

Issue Details
This is a spin-off from https://github.com/mono/mono/pull/21682, as @mdh1418 suggested to do. tl;dr: 1. As I understand, current implementation reads Android's tzdata directly. Here are few problems with that: neither its location nor its structure is documented or guaranteed. I don't know what .NET's API surface is, but _maybe_ ICU4C would suffice. Overall it endangers users as PR above illustrates. 2. How does current .NET implementation handles [TZif4](https://www.ietf.org/archive/id/draft-murchison-rfc8536bis-07.html) files? 3. Do you have tests like [this](https://android.googlesource.com/platform/libcore/+/refs/heads/main/luni/src/test/java/libcore/java/time/TimeApisConsistencyTest.java)? I think old Mono implementation would fail on `Asia/Gaza` as it has predefined transition up to 2086 (does not fit into 32-bit section of TZif ver. 1) 4. We usually [make](https://r.android.com/2162196) tzdata changes early in the release cycle. Can you include Android beta builds into testing matrix? Happy to answer your questions! Thanks, Almaz
Author: Yqwed
Assignees: -
Labels: `area-System.Globalization`, `untriaged`
Milestone: -
ghost commented 1 year ago

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger See info in area-owners.md if you want to be subscribed.

Issue Details
This is a spin-off from https://github.com/mono/mono/pull/21682, as @mdh1418 suggested to do. tl;dr: 1. As I understand, current implementation reads Android's tzdata directly. Here are few problems with that: neither its location nor its structure is documented or guaranteed. I don't know what .NET's API surface is, but _maybe_ ICU4C would suffice. Overall it endangers users as PR above illustrates. 2. How does current .NET implementation handles [TZif4](https://www.ietf.org/archive/id/draft-murchison-rfc8536bis-07.html) files? 3. Do you have tests like [this](https://android.googlesource.com/platform/libcore/+/refs/heads/main/luni/src/test/java/libcore/java/time/TimeApisConsistencyTest.java)? I think old Mono implementation would fail on `Asia/Gaza` as it has predefined transition up to 2086 (does not fit into 32-bit section of TZif ver. 1) 4. We usually [make](https://r.android.com/2162196) tzdata changes early in the release cycle. Can you include Android beta builds into testing matrix? Happy to answer your questions! Thanks, Almaz
Author: Yqwed
Assignees: -
Labels: `area-System.Globalization`, `os-android`, `untriaged`
Milestone: -
steveisok commented 1 year ago

Appreciate the feedback! I'll try to answer your questions to the best of my knowledge.

  1. As I understand, current implementation reads Android's tzdata directly. Here are few problems with that: neither its location nor its structure is documented or guaranteed. I don't know what .NET's API surface is, but maybe ICU4C would suffice. Overall it endangers users as PR above illustrates.

Up until this point, we've felt that was an acceptable risk reading from tzdata as it has the least impact on startup performance and I don't believe ICU4C covers everything. As an alternative, we could call the java API's via JNI, but I would like to avoid that unless it's absolutely necessary.

  1. How does current .NET implementation handles TZif4 files?

It appears we only support up to v3 and we will fall back to v1 in the case of a unsupported version https://github.com/dotnet/runtime/blob/a5461cbc8ed20e0981fd4e846a180f35b07dcc0a/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs#L1324-L1327

@tarekgh are there any plans on supporting v4?

  1. Do you have tests like this? I think old Mono implementation would fail on Asia/Gaza as it has predefined transition up to 2086 (does not fit into 32-bit section of TZif ver. 1)

The way dotnet/runtime parses tzdata appears to be a bit more robust than what we had in mono/mono. I'm not sure if Asia/Gaza is part of our test cases, but it would be interesting to try it and see if there's an issue.

Good question re: the TimeApisConsistencyTest test. @tarekgh, would you happen to know if we have an equivalent.

  1. We usually make tzdata changes early in the release cycle. Can you include Android beta builds into testing matrix?

Yes, we have the capability of adding different emulators to our CI. This is something we should do more aggressively.

Yqwed commented 1 year ago

Thanks for quick answer!

Up until this point, we've felt that was an acceptable risk reading from tzdata as it has the least impact on startup performance and I don't believe ICU4C covers everything. As an alternative, we could call the java API's via JNI, but I would like to avoid that unless it's absolutely necessary.

Yeah, I understand why you took that path :) Like Java APIs, do you also need to know information about past and upcoming transitions?

@tarekgh are there any plans on supporting v4?

It is available as a draft only, but AFAIU current zic can produce TZif4 files. But we don't support it either. I think it's better to be vocal about such things, because otherwise it will take a while to find the root cause.

I don't know whether it is a good idea to fallback to V1 - it has 32-bit data only and slim format skips it entirely, producing valid TZif file (that's exactly what happened in https://github.com/mono/mono/pull/21682). And even if it has 32-bit data we are getting closer to 2038, were it may not work correctly.

Yes, we have the capability of adding different emulators to our CI. This is something we should do more aggressively.

SGTM!

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-datetime See info in area-owners.md if you want to be subscribed.

Issue Details
This is a spin-off from https://github.com/mono/mono/pull/21682, as @mdh1418 suggested to do. tl;dr: 1. As I understand, current implementation reads Android's tzdata directly. Here are few problems with that: neither its location nor its structure is documented or guaranteed. I don't know what .NET's API surface is, but _maybe_ ICU4C would suffice. Overall it endangers users as PR above illustrates. 2. How does current .NET implementation handles [TZif4](https://www.ietf.org/archive/id/draft-murchison-rfc8536bis-07.html) files? 3. Do you have tests like [this](https://android.googlesource.com/platform/libcore/+/refs/heads/main/luni/src/test/java/libcore/java/time/TimeApisConsistencyTest.java)? I think old Mono implementation would fail on `Asia/Gaza` as it has predefined transition up to 2086 (does not fit into 32-bit section of TZif ver. 1) 4. We usually [make](https://r.android.com/2162196) tzdata changes early in the release cycle. Can you include Android beta builds into testing matrix? Happy to answer your questions! Thanks, Almaz
Author: Yqwed
Assignees: -
Labels: `os-android`, `area-System.DateTime`
Milestone: -
tarekgh commented 1 year ago

It is available as a draft only, but AFAIU current zic can produce TZif4 files. But we don't support it either. I think it's better to be vocal about such things, because otherwise it will take a while to find the root cause.

I am seeing the draft is expired and it is not clear to me if this is approved spec now or just abandoned? It is not a good idea to take dependency on such expired draft till we have confirmation it is going to be supported.

Good question re: the TimeApisConsistencyTest test. @tarekgh Tarek Mahmoud Sayed FTE, would you happen to know if we have an equivalent.

This test according to the comments there is testing different Java implementation when using ICU or when bionic. This is irrelevant to .NET as we don't use ICU anyway and only depend on reading the tzdata. We treat Asia/Gaza as any other time zone and not special casing it in any way.

On Android, java.time is implemented using ICU4J, bionic and java.util.TimeZone read transitions
 * from tzdata file. These tests ensure that they have consistent view on what offset was on a given
 * time zone at a given time.
Yqwed commented 1 year ago

I am seeing the draft is expired and it is not clear to me if this is approved spec now or just abandoned? It is not a good idea to take dependency on such expired draft till we have confirmation it is going to be supported.

Yes. What I am saying is that in the wild you might observe it with the relatively recent zic version.

This test according to the comments there is testing different Java implementation when using ICU or when bionic. This is irrelevant to .NET as we don't use ICU anyway and only depend on reading the tzdata. We treat Asia/Gaza as any other time zone and not special casing it in any way.

What I am suggesting is you can compare libc vs .NET. It might have caught https://github.com/mono/mono/pull/21682 if dates outside of 2038 were tested. I am mentioning Asia/Gaza because it observes DST, but due to the way transitions are set they are pregenerated and there is no TZ string in its TZif files.

tarekgh commented 1 year ago

Yes. What I am saying is that in the wild you might observe it with the relatively recent zic version.

are we aware of any product using zic generated files with v4?

What I am suggesting is you can compare libc vs .NET. It might have caught https://github.com/mono/mono/pull/21682 if dates outside of 2038 were tested. I am mentioning Asia/Gaza because it observes DST, but due to the way transitions are set they are pregenerated and there is no TZ string in its TZif files.

Are you interested in submitting a PR adding a test for that? If not, that is ok, we can try to add this test later.

Yqwed commented 1 year ago

are we aware of any product using zic generated files with v4?

My point is that it's outside of your control and AFAIK no distro gives guarantees about TZif version of files under /usr/share/zoneinfo (i.e. it is not an API, unlike libc). I am not asking to support it, but suggesting to let user know when that happens, be it logging or throwing an exception instead of silently falling back to UTC,

Are you interested in submitting a PR adding a test for that? If not, that is ok, we can try to add this test later.

I have 0 knowledge of .NET/C# and being a googler does not help either :) It is not urgent, I am just raising awareness as it might affect users in the same way as the bug mentioned in https://github.com/dotnet/runtime/issues/93216#issue-1933048192 did and still does, as developers need to rebuild and republish their apps.

tarekgh commented 1 year ago

My point is that it's outside of your control and AFAIK no distro gives guarantees about TZif version of files under /usr/share/zoneinfo (i.e. it is not an API, unlike libc). I am not asking to support it, but suggesting to let user know when that happens, be it logging or throwing an exception instead of silently falling back to UTC,

throwing is not a good idea here as this can cause the whole app/service to crash. We are trying to be tolerant in such cases as apps can continue running if such issues arises. we may look at if we can either workaround the issue or log some info about it when encountering it.

I have 0 knowledge of .NET/C# and being a googler does not help either :) It is not urgent, I am just raising awareness as it might affect users in the same way as the bug mentioned in https://github.com/dotnet/runtime/issues/93216#issue-1933048192 did and still does, as developers need to rebuild and republish their apps.

Thanks for your help so far, we appreciate that. We can try later to look at this time zone and find out which dates/times are problematic to validate it.

Yqwed commented 1 year ago

throwing is not a good idea here as this can cause the whole app/service to crash. We are trying to be tolerant in such cases as apps can continue running if such issues arises. we may look at if we can either workaround the issue or log some info about it when encountering it.

It depends. By the time users notice that everything falls back to UTC it might be too late. Though logging definitely wouldn't hurt.

uniquecorn commented 12 months ago

Hi, I'm not sure if this is the same issue but TimeZoneInfo.Local returns UTC time when set to Peru Standard Time on Android.

tarekgh commented 12 months ago

@mdh1418 can you help investigating Peru Standard Time issue on Android?

mdh1418 commented 12 months ago

Hi @uniquecorn , could you provide more details regarding what dotnet sdk version is being used, whether an Android device or simulator is being used, what is the Android OS version, and any other information you think would be helpful for reproducing what you're seeing.

Building a .NET 8 Android App on an Android emulator with API 33 with Time zone set to GMT-05:00 Peru Standard Time with the following

        Console.WriteLine($"Local: {TimeZoneInfo.Local}");
        Console.WriteLine($"UTC: {TimeZoneInfo.Utc}");

I see

12-02 05:37:28.586 21088 21105 I DOTNET  : Local: (UTC-05:00) Peru Standard Time (Lima)
12-02 05:37:28.586 21088 21105 I DOTNET  : UTC: (UTC) Coordinated Universal Time
uniquecorn commented 12 months ago

Hi @uniquecorn , could you provide more details regarding what dotnet sdk version is being used, whether an Android device or simulator is being used, what is the Android OS version, and any other information you think would be helpful for reproducing what you're seeing.

Building a .NET 8 Android App on an Android emulator with API 33 with Time zone set to GMT-05:00 Peru Standard Time with the following


        Console.WriteLine($"Local: {TimeZoneInfo.Local}");

        Console.WriteLine($"UTC: {TimeZoneInfo.Utc}");

I see


12-02 05:37:28.586 21088 21105 I DOTNET  : Local: (UTC-05:00) Peru Standard Time (Lima)

12-02 05:37:28.586 21088 21105 I DOTNET  : UTC: (UTC) Coordinated Universal Time

Sorry I didn't specify but I'm using the dotnet implementation in Unity which I'm not even sure of the version. I'm also not sure about the device models, it was reported by multiple users of my app and when I checked debug logs for those users TimeZoneInfo.Local returns UTC. If the issue is not on your end then it's likely that Unity's dotnet is not updated with the Peru timezone. Thank you for your help though!