dotnet / runtime

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

TimeZoneInfo.ToSerializedString/FromSerializedString do not round trip on Unix #19794

Open justinvp opened 7 years ago

justinvp commented 7 years ago

The following test passes on Windows but fails on Unix:

public static IEnumerable<object[]> SystemTimeZonesTestData()
{
    foreach (TimeZoneInfo tz in TimeZoneInfo.GetSystemTimeZones())
    {
        yield return new object[] { tz };
    }
}

[Theory]
[MemberData(nameof(SystemTimeZonesTestData))]
public static void ToSerializedString_FromSerializedString_RoundTrips(TimeZoneInfo timeZone)
{
    string serialized = timeZone.ToSerializedString();
    TimeZoneInfo deserializedTimeZone = TimeZoneInfo.FromSerializedString(serialized);
    Assert.Equal(timeZone, deserializedTimeZone);
    Assert.Equal(serialized, deserializedTimeZone.ToSerializedString());
}

I believe it's due to how GetAdjustmentRules is implemented on Unix: https://github.com/dotnet/coreclr/blob/0a11492d52faa85c551761f8390be5de9d74e5ec/src/mscorlib/src/System/TimeZoneInfo.Unix.cs#L131-L153

vs. on Windows: https://github.com/dotnet/coreclr/blob/0a11492d52faa85c551761f8390be5de9d74e5ec/src/mscorlib/src/System/TimeZoneInfo.Win32.cs#L87

See dotnet/corefx#14795

Also, when you pass an AdjustmentRule[] array to TimeZoneInfo.CreateCustomTimeZone to create a custom instance of TimeZoneInfo, calling GetAdjustmentRules on Windows will always give you a cloned copy of the same adjustment rules, whereas on Unix it looks like it will modify the adjustment rules that are returned.

tarekgh commented 7 years ago

I moved this to the future release because fixing this will need some major changes how we read and handle the adjustment rules in Linux which will be risky for v2. We should have a better story for time zones in general in the future.

SUPERSC0TT commented 5 years ago

Bumping to see if there are any plans to get this fixed. This bug means that users who create custom time zones via microsoft's documentation will not be able to use GetAdjustmentRules() to see the correct definitions, namely the Transition Rules Start/End are not returned correctly.

tarekgh commented 5 years ago

@SUPERSC0TT there is no plan to address this very soon but we may get into that in the near future. the reason is we need to look at the whole TZ support and address the issues in more better designed way.

Could you please share some more information about your TZ usage? like do you use custom TZ? why? what is your scenarios you are using with?

We appreciate your feedback.

yahorsi commented 4 years ago

Guys, timezones are basic, we've got into this as well, and issues such this are a show-stopper if we need to move 2 the Linux ((

tarekgh commented 4 years ago

@yahorsi could you please tell more about your scenario? why you want serialize TZI at all? are you creating your own custom TZ?

yahorsi commented 4 years ago

We're actually hit by aa bit another issue, the one that is described here:

https://github.com/dotnet/corefx/issues/17120

And caused by the following:

            // The rules we use in Unix care mostly about the start and end dates but don't fill the transition start and end info.
            // as the rules now is public, we should fill it properly so the caller doesn't have to know how we use it internally
            // and can use it as it is used in Windows

Our scenario is simple, we have a 'constant' custom Timezone that is parsed using TimeZoneInfo.FromSerializedString. In some parts of the code we just need to read transition start and end info, and it works on Windows and fails on Linux.

As I see in the code it should be very easy to fix. The question is why it was made in the first place. As behaviour is really confusing. Seems like internally transition start and end info is correctly used and only "fails" when you call GetAdjustmentRules

tarekgh commented 4 years ago

As I see in the code it should be very easy to fix. The question is why it was made in the first place.

Internally the data stored inside adjustment rules in Linux is different than what we have in Windows. We have another issues (like the one you pointed at) tracking fixing exposing the rules correctly on Linux too. That is why you are seeing this issue. TZI originally designed around how Windows stored TZ data which is different than what Linux/IANA does which made us to workaround that by internally storing different data. There is some rules that cannot be easily expressed by what we expose in Adjustment rules and that is why there is some challenge here.

Will try to look at that in the next few weeks. I marked this issue for 5.0 release.

yahorsi commented 4 years ago

What people usually expect is that code just works the same way on the different operating systems. And we're hurt even if we didn't deploy on Linux as many today's build servers are using Linux and so, tests are just failing (

Is there any option to have the fix for the 2.2.* branch?

tarekgh commented 4 years ago

@danmosemsft @ericstj to advise regarding servicing this in previous releases.

We don't have the fix yet and depend on the fix we can evaluate the risk. meanwhile, we may try to find you a workaround which may help parsing the Linux serialized TZ.

yahorsi commented 4 years ago

Sorry, do you mean it should be serialized in different way for linux and windows? Now it parses just fine, at least it does not throw, and seems time conversion from/to UTC/local works fine as well, what fails (wrong transition start and end info) is just GetAdjustmentRules call

Just FYI, this is how our serialized string looks now:

            public const string TimeZoneZrhSerialized = "Central European Standard Time;60;(UTC+01:00) Sarajevo, Skopje, Warsaw, Zagreb;Central European Standard Time;Central European Daylight Time;[01:01:0001;12:31:9999;60;[0;02:00:00;3;5;0;];[0;03:00:00;10;5;0;];];";
tarekgh commented 4 years ago

Sorry, do you mean it should be serialized in different way for linux and windows?

No. what I meant, if there is a way to detect the string is serialized on Linux and interpret the serialized data differently to get the original intended information. I don't know yet if this is possible. I just wanted to say, if there is any reasonable workaround can be used here.

ericstj commented 4 years ago

We do not accept servicing requests for past releases through Github, please see https://dotnet.microsoft.com/platform/support/policy/dotnet-core, https://github.com/dotnet/core/blob/master/microsoft-support.md for details around backporting to past releases. The short answer is: if you need this for .NETCore 2.1, please open a support request, .NETCore 2.2 is no longer recieving hotfixes (only security fixes).

yahorsi commented 4 years ago

Thank you, we will have a look at possible workarounds and if needed I will create a proper fix request. BTW there is still time before 3.1 release, I'm afraid 5.0 is too far in the future

ericstj commented 4 years ago

There is still time for 3.1 but that window is closing quickly, we're already in "Ask mode" where we are scrutinizing risk of all fixes going into 3.1. If we can come up with a low risk fix in the next week it has a chance at getting in.

ralsu091 commented 4 years ago

Could you please share some more information about your TZ usage? like do you use custom TZ? why? what is your scenarios you are using with?

My use case for calling GetAdjustmentRules is for me to send down the month, week and hour of start/end transitions to an iot device so it can observe daylight saving time.

Here is another issue that is affected by this: https://github.com/dotnet/runtime/issues/26278#issuecomment-629847388

Clockwork-Muse commented 4 years ago

My use case for calling GetAdjustmentRules is for me to send down the month, week and hour of start/end transitions to an iot device so it can observe daylight saving time.

@ralsu091 - I'm assuming you're listing a subset of the fields, because month+week+hour(+year, assumedly) isn't sufficient. A DST transition isn't guaranteed to be on any particular day-of-week. For that matter, although unlikely, it's perfectly possible to have multiple transitions of various sorts in any given week.

You don't list what device you're using, but if it's something Linux-based, why not just update the actual timezone file (not necessarily through apt-get, just scp the relevant file, or wget on a schedule or something)? Although you really only need to worry about that if the device has a reason to operate in local time.

ademchenko commented 4 years ago

@tarekgh, let me describe how we use timezones. We are cross-platform and we are also facing the exception

 "The elements of the AdjustmentRule array must be in chronological order and must not overlap.",
        "stackTrace": "   at System.TimeZoneInfo.ValidateTimeZoneInfo(String id, TimeSpan baseUtcOffset, AdjustmentRule[] adjustmentRules, Boolean& adjustmentRulesSupportDst) in /_/src/System.Private.CoreLib/shared/System/TimeZoneInfo.cs:line 1989\n   at 

It looks like .net core poorly supports timezones in Linux, and it seems like it is something that careful attention should be given to from your side. Now about our scenario. First of all, IANA timezones are de-facto the standard and so we store timezones and show them to end-users only in IANA format. Of course, we need not only ids to show and to store but also the actual timezone objects to be able to make time conversions. The latter causes some difficulty while running on Windows since there are no IANA timezones there. What we and I believe, most people do is to convert IANA timezone to some Windows timezone that suits the corresponding IANA timezone in a way of base offset and adjustment rules. For that reason, we use the widespread library TimeZoneConverter which doesn't have any alternative. And, to recapitulate: having IANA id we need to show the user some timezone with that id and correct parameters - offset and adjustment. The only way to do that on Windows is to kind of merge IANA id with Windows parameters. That is possible only with a custom timezone:

 private static Dictionary<string, TimeZoneInfo> GetTimeZones()
        {
            return
                TZConvert.KnownIanaTimeZoneNames
                    .Select(tzId =>
                    {
                        var timeZoneInfo = TZConvert.GetTimeZoneInfo(tzId);

                        var customTimeZone = TimeZoneInfo.CreateCustomTimeZone(tzId, timeZoneInfo.BaseUtcOffset, null, null, null,
                            timeZoneInfo.GetAdjustmentRules(), !timeZoneInfo.SupportsDaylightSavingTime);

                        return customTimeZone;
                    })
                    .ToDictionary(v => v.Id);
        }

We would like to leave that code cross-platform but we can't do that as of yet, since CreateCustomTimeZone raises the exception on Linux.

Clockwork-Muse commented 4 years ago

@ademchenko - Frankly, if you're doing a lot of work with timezones, especially crossplatform, I'd really recommend you use NodaTime, and forget the BCL stuff. Especially since IANA and the Windows timezone database rev at different rates - you are checking for/correcting for that, right? Note too that IANA->Windows is a lossy conversion; some Windows zones refer to multiple IANA zones.

perlun commented 2 months ago

Ran into this ourselves because we had a test which made assumptions that the TimeZoneInfo could be round-tripped. Interestingly enough, it works on Unix too, for the UTC time zone (likely because it doesn't have any adjustment rules).

[Test]
public void UTC_test() {
    var tz = TimeZoneInfo.Utc;

    // Works correctly on both Linux and Windows
    var serialized = tz.ToSerializedString();
    var roundTrippedTimezone = TimeZoneInfo.FromSerializedString(serialized);

    roundTrippedTimezone.Should()
        .Be(tz);
}

[Test]
public void Stockholm_test() {
    var tz = TimeZoneInfo.FindSystemTimeZoneById("Europe/Stockholm");

    // The FromSerializedString() call fails on Linux
    var serialized = tz.ToSerializedString();
    var roundTrippedTimezone = TimeZoneInfo.FromSerializedString(serialized);

    roundTrippedTimezone.Should()
        .Be(tz);
}