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

BclDateTimeZone doesn't handle base-offset changes #342

Closed
GoogleCodeExporter opened this Issue Mar 15, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@GoogleCodeExporter

GoogleCodeExporter commented Mar 15, 2015

Consider the following tests:

    [Test]
    public void CanHandleBaseOffsetChange_Tzdb()
    {
        var tz = DateTimeZoneProviders.Tzdb["Europe/Moscow"];
        var dt = Instant.FromUtc(2013, 6, 1, 0, 0, 0).InZone(tz).LocalDateTime;
        var expected = new LocalDateTime(2013, 6, 1, 4, 0, 0);
        Assert.AreEqual(expected, dt);
    }

    [Test]
    public void CanHandleBaseOffsetChange_Bcl()
    {
        var tz = DateTimeZoneProviders.Bcl["Russian Standard Time"];
        var dt = Instant.FromUtc(2013, 6, 1, 0, 0, 0).InZone(tz).LocalDateTime;
        var expected = new LocalDateTime(2013, 6, 1, 4, 0, 0);
        Assert.AreEqual(expected, dt);
    }

    [Test]
    public void CanHandleBaseOffsetChange_Tzi()
    {
        var tz = TimeZoneInfo.FindSystemTimeZoneById("Russian Standard Time");
        var dt = TimeZoneInfo.ConvertTimeFromUtc(new DateTime(2013, 6, 1), tz);
        var expected = new DateTime(2013, 6, 1, 4, 0, 0);
        Assert.AreEqual(expected, dt);
    }

Only the TZDB test pases.  The TZI test fails due to the fact that 
TimeZoneInfo.AdjustmentRule doesn't track year-to-year changes to the base 
offset.  This is reported in https://support.microsoft.com/kb/3012229 and fixed 
in .Net 4.6 preview (coreclr).

NodaTime's BclDateTimeZone implementation has the same issue, because it loads 
its data from TimeZoneInfo.  In order to work around the issue, we would need 
to load BCL data either using Win32 APIs, or by reading directly from the 
registry.

The base offset changes ARE in the registry, so there's no reason that we would 
need to rely on the TZI fix.  Even if we did, we'd have to use reflection to 
get at the new internal field that was added to TimeZoneInfo.AdjustmentRule.

Original issue reported on code.google.com by mj1856 on 8 Feb 2015 at 6:46

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Also, note that the issue only occurs if the latest Windows time zone updates 
are installed which contain the 2014 changes for Russia's time zones.  These 
originally rolled out in KB2998527, but have since deployed to most Windows 
machines via the December 2014 DST Cumulative Update in KB3013410.

Original comment by mj1856 on 8 Feb 2015 at 6:49

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

If the offset were exposed via a public property, I'd be happy enough for Noda 
Time to use reflection to get at the property on systems where it's available, 
and not otherwise. I'd rather not rely on access to an internal field/property.

I'd also *prefer* not to use the registry directly, as it ties it more closely 
to the Windows rather than the BCL, if you see what I mean.

To my mind, it seems to indicate that TimeZoneInfo.AdjustmentRule is 
effectively incomplete as of .NET 4.6 - it doesn't provide enough information 
to predict the results of using a TimeZoneInfo.

Original comment by jonathan.skeet on 8 Feb 2015 at 7:11

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

I just remembered - the base offset delta is also included in the serialization 
info.  So we could consider calling TimeZoneInfo.ToSerializedString and then 
building our representation by parsing the output.

Original comment by mj1856 on 8 Feb 2015 at 7:54

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Of course, that would still only fix the problem when targeting coreclr...

Original comment by mj1856 on 8 Feb 2015 at 7:58

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Hmph. I suspect this is what's making my tests fail after installing VS2015 
CTP5.

However, I have a sneaky plan. For each AdjustmentRule, we can work out a 
DateTime which should be in standard time, and ask the TimeZoneInfo for the UTC 
offset on that DateTime - that should reveal the BaseUtcOffset for that rule.

That'll take a certain amount of implementation though - and the implementation 
has changed quite a bit for 2.0. I'm tempted to leave this as a known issue for 
1.x, and release 1.3.1 regardless.

Original comment by jonathan.skeet on 11 Feb 2015 at 10:47

  • Changed state: Accepted
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Fixed in changes 98ed5ffc4d7c (1.3) and bff6a46f2837 (2.0).

In the process of the fix, I've exposed (internally) some useful types which 
could be handy for reimplementing some time zone building anyway...

Original comment by jonathan.skeet on 14 Feb 2015 at 6:19

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Original comment by malcolm.rowe on 19 Feb 2015 at 1:11

  • Added labels: Milestone-1.3.1

@malcolmr malcolmr added the bug label Mar 15, 2015

@malcolmr malcolmr modified the milestone: 1.3.1 Mar 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment