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

DateTimeOffset.ToLocalTime() produces spurious offsets when time is before 1847 #11718

Closed
dancrn opened this issue Dec 27, 2018 · 19 comments
Closed

Comments

@dancrn
Copy link

dancrn commented Dec 27, 2018

Hi, I'm dan.

I use dotnet at work. Recently, I've come across this.. unusual bug, regarding time zone offsets with the DateTimeOffset struct.

Description

I have a code sample;

var arbitraryTime = DateTimeOffset.Parse("01/12/1847 00:01:15 +00:00");
Console.WriteLine($"Time 1 is: {arbitraryTime.ToLocalTime()}");

var otherArbitraryTime = arbitraryTime.AddTicks(0);
Console.WriteLine($"Time 2 is: {otherArbitraryTime.ToLocalTime()}");

var magicArbitraryTime = arbitraryTime.AddTicks(-1);
Console.WriteLine($"Time 3 is: {magicArbitraryTime.ToLocalTime()}");

When I run this on my machine, I get this output:

$ dotnet run test.csproj
Time 1 is: 01/12/1847 00:01:15 +00:00
Time 2 is: 01/12/1847 00:01:15 +00:00
Time 3 is: 01/12/1847 00:00:14 -00:01

Note that, after calling .ToLocalTime(), a spurious offset of -1 minute has been added to the offset time span. More generally, this is an issue because:

DateTimeOffset.MinValue.ToLocalTime(); // == 01/01/0001 00:00:00 -00:01

After updating some libraries we use, we've found that they will call .ToLocalTime() (for some reason), and this returned value, as far as our code is concerned, is a non-UTC time zone, in a place where we're really not expecting this to happen.

After a bit of investigation, I've found out that the UK switched from LMT (I did not know this was a time zone before now!) to GMT on 01/12/1847, and that seems to coincide with the issue I'm seeing - I'm certain that is not a pure coincidence :) However, I'm at a total loss of how to proceed with this - short of changing my timezone settings..

Reproduction

Now, this is the tricky part. Only I see this bug, and we deploy to AWS, on linux (Docker & EBS, etc.).

I'm running Ubuntu 18.04, and i'm running dotnet 2.1.302. My Locale is set to en_GB.UTF-8, with a timezone setting of Europe/London.

If there's any more information I can supply, please let me know.

@danmoseley
Copy link
Member

So if I understand correctly, you're asking for DateTimeOffset.MinValue to have a "floor" at 01/01/0001 00:00:00 00:00 and at least on Ubuntu in en_GB it's somehow earlier than that.

@krwq

@dancrn
Copy link
Author

dancrn commented Dec 28, 2018

I suppose so. I haven't checked explicitly, but I guess (or hope) it would be a value less than DateTimeOffset.MinValue. The problem that we're seeing however, is that the offset component is non-zero - that's all that is an issue for us.

Thanks,

@rizwansaeed
Copy link

I also see the same behaviour on macOS with dotnet 2.1.402 with the system timezone set to Europe/London

@danmoseley
Copy link
Member

@tarekgh thoughts ?

@tarekgh
Copy link
Member

tarekgh commented Feb 5, 2019

CC @eerhardt

dumping the zone data I am seeing the zone data showing a minute and 15 seconds offset as we are reporting it. look at year 1847 data and you can see this one minute offset

tarek@tarek-Virtual-Machine:~/NetCoreApp$ zdump -v /usr/share/zoneinfo/GB-Eire 
/usr/share/zoneinfo/GB-Eire  -9223372036854775808 = NULL
/usr/share/zoneinfo/GB-Eire  -9223372036854689408 = NULL
/usr/share/zoneinfo/GB-Eire  Wed Dec  1 00:01:14 1847 UT = Tue Nov 30 23:59:59 1847 LMT isdst=0 gmtoff=-75
/usr/share/zoneinfo/GB-Eire  Wed Dec  1 00:01:15 1847 UT = Wed Dec  1 00:01:15 1847 GMT isdst=0 gmtoff=0
/usr/share/zoneinfo/GB-Eire  Sun May 21 01:59:59 1916 UT = Sun May 21 01:59:59 1916 GMT isdst=0 gmtoff=0
/usr/share/zoneinfo/GB-Eire  Sun May 21 02:00:00 1916 UT = Sun May 21 03:00:00 1916 BST isdst=1 gmtoff=3600
/usr/share/zoneinfo/GB-Eire  Sun Oct  1 01:59:59 1916 UT = Sun Oct  1 02:59:59 1916 BST isdst=1 gmtoff=3600
/usr/share/zoneinfo/GB-Eire  Sun Oct  1 02:00:00 1916 UT = Sun Oct  1 02:00:00 1916 GMT isdst=0 gmtoff=0

@tarekgh
Copy link
Member

tarekgh commented Feb 5, 2019

Looks our calculation if off by 15 seconds. we may need to look deeper why this is happening. here is some more details:

We are using the date/time in Utc 1847-12-01T00:01:14.9999999Z we should be using the rule:

/usr/share/zoneinfo/GB-Eire  Wed Dec  1 00:01:14 1847 UT = Tue Nov 30 23:59:59 1847 LMT isdst=0 gmtoff=-75

which will have offset -75 seconds from the Utc.

converting the date to local we are getting 1847-12-01T00:00:14.9999999-00:01 while we expect to get 1847-11-30T23:59:59.9999999-00:01 which means we are off by 15 seconds. we maybe we are ignoring the seconds while doing the conversion.

@eerhardt
Copy link
Member

eerhardt commented Feb 6, 2019

The comment above it says why.

See https://docs.microsoft.com/en-us/dotnet/api/system.timezoneinfo.baseutcoffset?view=netframework-4.7.2#remarks.

The BaseUtcOffset value is represented as a whole number of minutes. It cannot include a fractional number of minutes.

It is documented on our public API that offsets cannot be fractions of minutes. This is for serialization purposes with things like SQL Server, XML, etc.

See also this check:

https://github.com/dotnet/coreclr/blob/1b2810b1ad92d2671da76d88245198b62f5b964e/src/System.Private.CoreLib/shared/System/TimeZoneInfo.cs#L1941-L1944

@tarekgh
Copy link
Member

tarekgh commented Feb 6, 2019

The comment above it says why.

I saw the comment but was not getting why we are not allowing the fraction. your serialization comment kind clarified why. looking at the ISO date format, it allows hh:mm only and not having a seconds. or serializing the dates with the ISO formats is not going to work anyway. So now it make sense.

by that, I am closing this issue with mentioning we have limitation in our TimeZoneInfo not supporting offsets with minute fraction. This will happen only in the historical zones like the one mentioned in this issue when using years before 1848

@tarekgh tarekgh closed this as completed Feb 6, 2019
@dancrn
Copy link
Author

dancrn commented Feb 6, 2019

please reconsider the closing of this issue, this is causing problems with:

DateTimeOffset.MinValue.ToLocalTime()

whilst i’m sure there aren’t many dates in real world use that get used before 1847, i expect this is used frequently. at least it is so for us..!

@tarekgh
Copy link
Member

tarekgh commented Feb 6, 2019

Could you please tell more what is exactly the problem you are facing with DateTimeOffset.MinValue.ToLocalTime()? I mean what is wrong getting 01/01/0001 00:00:00 -00:01? which still looks right I guess. I am reopening the issue till we know exactly what is the ask.

@tarekgh tarekgh reopened this Feb 6, 2019
@dancrn
Copy link
Author

dancrn commented Feb 6, 2019

i’ll provide more detail tomorrow! thank you for reconsidering this.

@tarekgh
Copy link
Member

tarekgh commented Mar 6, 2019

@dancrn any news?

@tarekgh
Copy link
Member

tarekgh commented Mar 12, 2019

@dancrn closing this one, but feel free to respond back when having any more info.

@tarekgh tarekgh closed this as completed Mar 12, 2019
@dancrn
Copy link
Author

dancrn commented Mar 13, 2019

@tarekgh sorry for taking forever to respond - i'll give you more information now though.

ok, to recap, the issue is being exposed when we have a default-valued DateTimeOffset struct. this value cannot be reliably round-tripped in a large number of libraries. having examined the source of a few libraries (namely Npgsql and Json.NET), they will make a call to DateTimeOffset.ToLocalTime() deserialising DateTimeOffset values. i.e.

// given an input date
string dateTimeString = '0001-01-01 00:00:00.000000+00:00';

// parse it into a DateTimeOffset 
DateTimeOffset.TryParse(dateTimeString, out DateTimeOffset parsedDateTimeOffset);

// a lot of libraries have inexplicably decided to return times with the offset of the 
// local system, losing the supplied offset from the input.. 
var localTime = parsedDateTimeOffset.ToLocalTime()

// this call produces the spurious offset - it will return '0001-01-01 00:00:00.000000-00:01'
return parsedDateTimeOffset.ToUniversalTime();

whether or not these libraries should be doing this conversion (it is our strong opinion that they should not be doing this - as it loses information that we've been supplied from our API endpoints, and other reasons), is left as an exercise to the reader. regardless, both Json.NET and Npgsql do this, and it's unlikely this behavior is going to change.

i've created a set of tests, which seem to suggest (on macOS 10.14.2, at least), that a lot of timezones exhibit this behavior - 132 time zones fail this test whereas 189 do not. i've tried to be clear and explicit with what i'm doing with these tests, and they should work on a machine in any locale. all it requires is the appropriate nUnit packages.

using System;
using System.Collections.Generic;
using System.Linq;
using NUnit.Framework;

namespace DateTimeTests
{
  public class RoundTripTests
  {
    public static IEnumerable<TimeZoneInfo> GetTimeZones()
      => TimeZoneInfo.GetSystemTimeZones();
    
    [Test, TestCaseSource(nameof(GetTimeZones))]
    public void DateTime_RoundTrip_DoesNotModifyValue(TimeZoneInfo timeZone)
    {
      // starting backward from the year 2000, down to year 0,
      for (int i = 2000; i > 0; i--)
      {
        //create a date in that year, of kind Utc
        DateTime initialTime = DateTime.SpecifyKind(new DateTime(i, 1, 1, 0, 0, 0), DateTimeKind.Utc);
        
        //convert it to a local time
        DateTime localTime = TimeZoneInfo.ConvertTimeFromUtc(initialTime, timeZone);
        
        //now convert it back to a Utc time
        DateTime roundTrippedTime = TimeZoneInfo.ConvertTimeToUtc(localTime, timeZone);
  
        //this should be lossless
        Assert.That(roundTrippedTime, Is.EqualTo(initialTime));
      }
    }

    [Test]
    public void DateTimeOffset_RoundTrip_DoesNotModifyValue()
    {
      Assert.That(
        GetBadlyBehavedTimeZonesStrings(),
        Does.Contain(TimeZoneInfo.Local.StandardName),
        "It would appear as though this system will not be affected by this issue."
      );

      // do the same thing as above, but with a DateTimeOffset
      DateTimeOffset initialTime = DateTimeOffset.MinValue;
      DateTimeOffset localTime = initialTime.ToLocalTime();
      DateTimeOffset utcTime = localTime.ToUniversalTime();

      Assert.That(utcTime, Is.EqualTo(initialTime));
    }
    
    //gets the set of id strings of timezones that appear to have this issue
    private static ISet<string> GetBadlyBehavedTimeZonesStrings()
      => GetTimeZones().Where(TimeZoneIsBadlyBehaved).Select(zone => zone.StandardName).ToHashSet();

    //this essentially runs the same code as the first test
    private static bool TimeZoneIsBadlyBehaved(TimeZoneInfo zone)
    {
      DateTime time = DateTime.SpecifyKind(DateTime.MinValue, DateTimeKind.Utc);
      return time != TimeZoneInfo.ConvertTimeToUtc(TimeZoneInfo.ConvertTimeFromUtc(time, zone), zone);
    }
  }
}

if there is any other information i can supply, please let me know. again, sorry for being so slow with this..

@tarekgh
Copy link
Member

tarekgh commented Mar 13, 2019

@dancrn thanks for the info. Using '0001-01-01 00:00:00.000000+00:00' as the default is not good idea anyway because the conversion to local for sure will not work. I mean you'll not be able to round trip that with all time zones. I would suggest you use some other defaults (e.g. UnixEpoch). sorry, we have some limitation which would be difficult to change.

@dancrn
Copy link
Author

dancrn commented Mar 13, 2019

thanks for the reply!

we're not trying to round trip this value - it's an odd choice of behavior by libraries that we cannot control :(

for various technical reasons, we need to be able to support this value of DateTimeOffset. In any case, the DateTimeOffset.MinValue round tripping works on windows, it's only on macOS and Linux that has this issue - although all platforms share the DateTime round tripping issue for some timezones.

to leave this as it is could have some real impact:

  1. we can't reliably store the postgres data type timestamp with timezone with value 0001-01-01 00:00:00+00 without ensuring we run on a machine with the UTC timezone set as its timezone.
  2. we can't rely on Json.Net to read correct values from our incoming data, which is very important to us. note that this is not limited to DateTimeOffset.MinValue! one of the DateTime tests (that starts backward from year 2000) gives this output:
Expected: 1911-01-02 00:00:00
But was:  1910-12-31 23:56:00

@tarekgh
Copy link
Member

tarekgh commented Mar 13, 2019

To understand more about your scenario: the date "0001-01-01 00:00:00+00", who originally instantiate this date? is it you or this is something can be passed to you from outside data? looks this date is not really have any meaning more than just default. would it be ok to check if you have such date and at that time always use UTC tz?

@dancrn
Copy link
Author

dancrn commented Mar 14, 2019

we can consume a DateTimeOffset value of 0001-01-01 00:00:00+00 from our web api, although we also need to use this value in some instances for correctness of our business logic.

we already enforce that it is for the UTC time zone. libraries that we use in our code base perform this round tripping (again, i don't know why they do this) and this is where the issue becomes apparent for us.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants