Skip to content
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.

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

TimeZoneInfo.AdjustmentRule information is not correct on Unix #20626

Closed
eerhardt opened this issue Mar 14, 2017 · 10 comments · Fixed by #49733
Closed

TimeZoneInfo.AdjustmentRule information is not correct on Unix #20626

eerhardt opened this issue Mar 14, 2017 · 10 comments · Fixed by #49733

Comments

@eerhardt
Copy link
Member

On Unix, we are trying to populate the "DaylightTransitionStart" and "DaylightTransitionEnd" values for the AdjustmentRules. However, this design is flawed and leads to incorrect data being reported by the API.

See https://github.com/dotnet/corefx/issues/2465#issuecomment-127645308 and the whole issue for discussion on how the Windows-centric TimeZoneInfo.AdjustmentRule API was modified to fit the information that is available on Unix.

We are trying to hydrate the DaylightTransition Start and End data here https://github.com/dotnet/coreclr/blob/c2b5d5a707e3bb037df847e17b4e55e19fd5bfa8/src/mscorlib/src/System/TimeZoneInfo.Unix.cs#L124-L158. However, this data is incorrect on Unix. Take the Africa/Casablanca time zone from 1940-1945:

From our tests

            // Africa/Casablanca had DST from
            //     1940-02-25T00:00:00.0000000Z {+01:00:00 DST=True}
            //     1945-11-17T23:00:00.0000000Z { 00:00:00 DST=False}

Printing the AdjustmentRules from around that time:

Baseoffset: 00:00:00
DateStart       DateEnd         DaylightDelta   DaylightTransitionStart DaylightTransitionEnd
11/17/1945      06/10/1950      00:00:00        M11.w1.d17 11:00 PM     M6.w1.d10 11:59 PM
02/25/1940      11/17/1945      01:00:00        M2.w1.d25 1:00 AM       M11.w1.d17 11:59 PM
11/18/1939      02/24/1940      00:00:00        M11.w1.d18 11:00 PM     M2.w1.d24 11:59 PM
09/12/1939      11/18/1939      01:00:00        M9.w1.d12 1:00 AM       M11.w1.d18 11:59 PM
10/26/1913      09/11/1939      00:00:00        M10.w1.d26 12:30 AM     M9.w1.d11 11:59 PM

Specifically, look at this row:

DateStart       DateEnd         DaylightDelta   DaylightTransitionStart DaylightTransitionEnd
02/25/1940      11/17/1945      01:00:00        M2.w1.d25 1:00 AM       M11.w1.d17 11:59 PM

What this says is that for times in this time zone between 02/25/1940 and 11/17/1945, Daylight Savings Time starts on the 25th day of February and ends on the 17th day of November of every year. So from Nov 17 to Feb 25 every year, the time zone should not be in daylight savings time, which is incorrect because the time zone was in DST all year round those 5 years.

I don't think we should be populating these Daylight TransitionTime values at all when giving the AdjustmentRules to public consumers. Anyone who wants to consume this information is going to be broken trying to use it. This is just going to lead to incorrect information being displayed to their users.

Code to print the above table

    class Program
    {
        static void Main(string[] args)
        {
            TimeZoneInfo tz = TimeZoneInfo.FindSystemTimeZoneById("Africa/Casablanca");
            System.Console.WriteLine($"Baseoffset: {tz.BaseUtcOffset}");

            StringBuilder builder = new StringBuilder();
            builder.Append("DateStart");
            builder.Append("\t");
            builder.Append("DateEnd");
            builder.Append("\t");
            builder.Append("\t");
            builder.Append("DaylightDelta");
            builder.Append("\t");
            builder.Append("DaylightTransitionStart");
            builder.Append("\t");
            builder.Append("DaylightTransitionEnd");
            System.Console.WriteLine(builder);

            var rules = tz.GetAdjustmentRules();
            DateTime startDate = new DateTime(1945, 12, 31);
            //DateTime startDate = DateTime.Now;
            foreach (var rule in rules.Where(r => r.DateStart < startDate).Reverse().Take(5))
            {
                builder = new StringBuilder();
                builder.Append(rule.DateStart.ToString("MM/dd/yyyy"));
                builder.Append("\t");
                builder.Append(rule.DateEnd.ToString("MM/dd/yyyy"));
                builder.Append("\t");
                builder.Append(rule.DaylightDelta);
                builder.Append("\t");
                PrintTransition(rule.DaylightTransitionStart, builder);
                builder.Append("\t");
                PrintTransition(rule.DaylightTransitionEnd, builder);
                System.Console.WriteLine(builder);
            }
        }

        private static void PrintTransition(TimeZoneInfo.TransitionTime transition, StringBuilder builder)
        {
            builder.Append($"M{transition.Month}.w{transition.Week}.d{transition.Day} {transition.TimeOfDay.ToShortTimeString()}");
        }
    }

/cc @tarekgh @mj1856

@tarekgh
Copy link
Member

tarekgh commented Mar 14, 2017

#20624

@joperezr
Copy link
Member

Is this a dupe of #20624? If so, then we should just close this

@tarekgh
Copy link
Member

tarekgh commented Mar 16, 2017

it is related but not duplicate. please keep both opened. thanks

@eerhardt
Copy link
Member Author

Note that for Windows, it doesn't even have these historical transition times in the data. So not fixing this bug in 2.0 is fine, since the information we provide on Unix is already more correct than Windows.

@jskeet
Copy link

jskeet commented May 25, 2018

One general point - the code has this comment:

// 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

My experience trying to interpret the rules in Noda Time is that they definitely can't use the returned rules as if they were the Windows rules. If you want a really high bar for compatibility between Unix and Windows here, I'd be happy to try some sample data representations and see whether there's a way of making Noda Time take a single path through both :)

(As an aside, .NET on Windows doesn't handle its own data correctly anyway, or at least didn't - see https://codeblog.jonskeet.uk/2014/09/30/the-mysteries-of-bcl-time-zone-data/ for details.)

@tarekgh
Copy link
Member

tarekgh commented May 25, 2018

@jskeet

I'd be happy to try some sample data representations and see whether there's a way of making Noda Time take a single path through both :)

That will be great. in general, net core still has problems reporting the adjustment rules correctly on Linux. and on Windows, we depend on Windows data (we read from the registry) which has some limitations (e.g. cannot have 2 daylight transitions in the same year, and not having the historical data too).

We have some plans to investigate how we can get accurate data and fix the related issues. if you have ideas, we welcome to explore them.

For my education, does Noda Time depends on TimeZoneInfo in getting the adjustment rules? I had the impression that Noda time carry its own data. or I may be missing something here.

@jskeet
Copy link

jskeet commented May 25, 2018

Yes, we have our own copy of the IANA data and you can load extra data at execution time - but we also allow a conversion of TimeZoneInfo to DateTimeZone for interoperability. I definitely understand that's a bit of a second class citizen in terms of data :)

@tarekgh
Copy link
Member

tarekgh commented May 25, 2018

@jskeet I want to get your feedback regarding carrying IANA data in your library. Do you regularly update your library (I mean you have a new version) when you update the data? and how the applications using your library get the updates? I mean does the apps need to republished because of that? I am asking because this is the classical problem with carrying the data and that is why we still depend on the OS for the data and carrying ours.

@jskeet
Copy link

jskeet commented May 25, 2018

@tarekgh: Yes, we release a new version when there's a data change, as a patch. Recently we've been releasing patches to 1.3.x, 1.4.x, 2.0.x, 2.1.x and 2.2.x (where 2.3.0 has only just come out) but we're likely to reduce that soon to just 1.4.x, 2.2.x and 2.3.x.

In addition, we publish a new data file on the web site. Applications can check https://nodatime.org/tzdb/latest.txt to find the URL of the latest data file, and download it if it's newer than the version they've already got. It's easy to create a DateTimeZoneProvider from the file.

The intention is that between those two methods, it should be easy for anyone to get the latest version of the data in most situations. Additionally, I think it's important that the data is self-versioned, i.e. you can ask which version of the data you're using. That's really helpful for diagnostic purposes.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2020
@tarekgh tarekgh modified the milestones: Future, 6.0.0 Nov 10, 2020
@mattjohnsonpint
Copy link
Contributor

@tarekgh @eerhardt - See another case in mattjohnsonpint/TimeZoneConverter#83 I believe it's the same problem. Thanks.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 17, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants