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

DateTimeZoneProviders.Bcl.GetSystemDefault() is broken on non-Windows Mono #235

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

Comments

Projects
None yet
2 participants
@GoogleCodeExporter

GoogleCodeExporter commented Mar 15, 2015

All versions of Mono on non-Windows platforms are broken with respect to 
DateTimeZoneProviders.Bcl.GetSystemDefault(), with the possible exception of 
MonoDroid, which I can't check.

Because:
1. TimeZoneInfo.Local always provides a TimeZoneInfo with a fixed ID of "Local".
2. TimeZoneInfo.GetSystemTimeZones() does not actually contain a TimeZoneInfo 
with ID "Local", and so calls to DateTimeZoneProviders.Bcl.GetSystemDefault() 
will fail with e.g. "DateTimeZoneNotFoundException: Time zone Local is unknown 
to source TimeZoneInfo: 3.5.0.0".

We could fix this by special-casing the local timezone in 
BclDateTimeZoneSource, by making GetIds() unconditionally include 
TimeZoneInfo.Local.Id.

Note that TimeZoneInfo.FindSystemTimeZoneById(TimeZoneInfo.Local.Id) _does_ 
work, so I believe that would be all that's needed.

[Note also that DateTimeZoneProviders.Tzdb.GetSystemDefault() will never work 
on non-Windows Mono, as we have no CLDR mapping for a Windows time zone named 
"Local".]

I'd like to explore taking this for 1.1.1, given that other fix in 1.1.1 is in 
the same area. Jon: what do you think?

Original issue reported on code.google.com by malcolm.rowe on 31 Jul 2013 at 4:20

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Note that TimeZoneInfo.Local is broken on Mono on Windows too, in that it goes 
bang very quickly indeed.

I'm not sure I'd want to return an ID of "Local" itself - in particular, we 
wouldn't want to end up propagating that in strings etc. I suppose we could 
*advertise* an available ID of "Local" but then *return* a DateTimeZone with a 
more useful ID. Hmm.

I think I'd be happy checking the TimeZoneInfo against TimeZoneInfo.Local when 
we need to.

Agree that we should try to address this for 1.1.1 though.

Original comment by jonathan.skeet on 31 Jul 2013 at 4:46

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

I'm not sure which part of TimeZoneInfo.Local doesn't work on Mono/Windows 
([citation needed]), but in any case, I'm not sure how we could return a more 
useful ID than "Local" - it's not obvious that TimeZoneInfo.Local is guaranteed 
to be equivalent to any of the values returned by 
TimeZoneInfo.GetSystemTimeZones() (and in fact I could well believe that it 
might not be).

Original comment by malcolm.rowe on 31 Jul 2013 at 5:35

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

This is what happens to TimeZoneInfo.Local. Complete program:

    using System;

    class Test
    {
        static void Main()
        {
            var info = TimeZoneInfo.Local;
        }
    }

Result of running on 3.0.6 on Windows:

Unhandled Exception:
System.TimeZoneNotFoundException: Exception of type 
'System.TimeZoneNotFoundException' was thrown.
  at System.TimeZoneInfo.get_Local () [0x00000] in <filename unknown>:0
  at Test.Main () [0x00000] in <filename unknown>:0 [ERROR] FATAL UNHANDLED EXCEPTION: System.TimeZoneNotFoundException: Exception of type 'System.TimeZoneNotFoundException' was thrown.
  at System.TimeZoneInfo.get_Local () [0x00000] in <filename unknown>:0
  at Test.Main () [0x00000] in <filename unknown>:0

Ick :)

I see what you mean now about using "Local" as a "real" ID on its own. It feels 
like if we *could* identify it as one of the zones in GetSystemTimeZones, that 
would be a better experience... but if not, maybe that's good enough. We should 
put big warnings in the user guide about this though.

Original comment by jonathan.skeet on 31 Jul 2013 at 5:45

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Interesting. Mono >= 2.11 added support for Windows (though possibly not XP): 
it checks the value of the 'TimeZoneKeyName' value in the 
HKLM\SYSTEM\CurrentControlSet\Control\TimeZoneInformation registry key, then 
uses that to look into the key with that name under 
HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Time Zones.

(Does that look like it should work in your case?)

Yes, if your experience is accurate: GetSystemDefault() is basically broken on 
Mono.

Original comment by malcolm.rowe on 31 Jul 2013 at 5:55

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

I've now managed to find Windows installers for Mono 2.11.4 and 3.0.10, both of 
which fail in the same way. There's a bug for it here: 
https://bugzilla.xamarin.com/show_bug.cgi?id=11817

Not sure why the fix you've mentioned doesn't actually work, but it does sound 
like time zones are basically borked on Mono, one way or another. I don't know 
whether that affects what we do for Mono on non-Windows... I'll try installing 
the latest version on MacOSX and see what that does in terms of Local ID.

Original comment by jonathan.skeet on 31 Jul 2013 at 6:34

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Mono 3.2.0 on MacOSX gives the same ID of "Local". Grr.

Original comment by jonathan.skeet on 31 Jul 2013 at 6:42

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Yes: all non-Windows versions of Mono do that.  (Except possibly for MonoDroid, 
as mentioned.)

Original comment by malcolm.rowe on 31 Jul 2013 at 8:32

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Just tried it on MonoDroid (on a Nexus 4 with Android 4.3) - TimeZoneInfo.Local 
returns null. Yet another failure mode...

Original comment by jonathan.skeet on 31 Jul 2013 at 8:48

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

This issue was closed by revision 02f4a857bf93.

Original comment by malcolm.rowe on 31 Jul 2013 at 10:33

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

I've implemented the fix described in the initial report above. Let me know 
whether you think it's okay to apply this to 1.1.x.

Original comment by malcolm.rowe on 31 Jul 2013 at 10:36

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Sorry I didn't chime in earlier...

Is there some reason we can't just look at the TZ environment variable?  My 
understanding is that it will likely be the actual TZDB zone name.

Technically, there are actually a few different formats it can be:
- A single number which is a fixed UTC offset, in minutes, with the sign 
inverted (positive values are west of UTC), such as "300"
- A time zone name followed by the offset, such as "EST 300"
- A POSIX style timezone string like EST+5EDT,M4.1.0/2,M10.5.0/2
- An IANA time zone id like "America/New_York"

IMHO - when running on Mono, we should check for the TZ environement string.  
If it exists and is an IANA zone name, it should be used directly.  If it is 
one of the other values, we should use a fixed-offset zone, a custom zone, or 
if possible try to infer the correct IANA zone.  For BCL zones, we should try 
to use the CLDR data to map back to a windows zone id.  Only if all of that 
fails should we return null or "Local".


Original comment by mj1856 on 31 Jul 2013 at 10:53

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

There may be a few other variations like "EST 5", "EST+5", "EST +05:00", etc.  
There are some docs here:

http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
http://linux.die.net/man/3/timezone

Original comment by mj1856 on 31 Jul 2013 at 10:55

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

TZ isn't always set. In fact, it's not set for me right now.

$ echo $TZ

$ 

tzname(3) has this to say:
If the TZ variable does not appear in the environment, the tzname variable  is
initialized with the best approximation of local wall clock time, as specified
by the tzfile(5)-format file localtime found in the system timezone  directory
see  below).  (One also often sees /etc/localtime used here, a symlink to the
right file in the system timezone directory.)

And /etc/localtime is what Mono actually reads on UNIX-like systems (including 
OS X).  It's in the same format as TZDB data[*] (recurrences, zone 
abbreviations, etc), but crucially doesn't include a time zone _name, that 
normally being what you use to find it (in the case of the TZDB data, you'd 
look for e.g. /usr/share/zoneinfo/Europe/London).

[*] or possibly the other way round.

Original comment by malcolm.rowe on 31 Jul 2013 at 11:12

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Interesting.  But can we use /etc/localtime or TZ if they ARE set?

Original comment by mj1856 on 31 Jul 2013 at 11:33

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

/etc/localtime doesn't help us, since it would just provide what 
TimeZoneInfo.Local does (that is, it has no ID).  We could conceivably 
special-case BclDateTimeZone to recognise when the (assumed to be singleton) 
TimeZoneInfo.Local instance was passed in, and replace "Local" with the value 
of $TZ, if set (and assuming that it was a valid system timezone)... but I 
think this is something that should be really handled by Mono rather than us.

Note to self: we'll want to take followup revision b3f3c929722d if we backport 
this to 1.1.x.

Original comment by malcolm.rowe on 1 Aug 2013 at 7:43

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

As Malcolm says, this is something that should be handled by Mono, not us. 
Unlike the PCL version (where the best API is simply missing) there's a 
perfectly valid API here, it's just not being implemented properly.

I think it may be worth backporting the existing fix to 1.1.1, but I'd rather 
not go to extremes to get go behind Mono's back for the local time zone.

Original comment by jonathan.skeet on 1 Aug 2013 at 8:24

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

This issue was closed by revision a4308264e3d5.

Original comment by malcolm.rowe on 1 Aug 2013 at 8:49

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Backported to 1.1.x for 1.1.1 as revision a4308264e3d5, revision aae5e43c952f, 
and revision 9d74726f515a.

Original comment by malcolm.rowe on 1 Aug 2013 at 8:50

  • Added labels: Milestone-1.1.1
  • Removed labels: Milestone-1.1-consider
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

My only concern is in translation error.  Say mono picks up a TZDB zone from 
/etc/localtime, converts it to a BCL zone (using CLDR?).  Our 
Bcl.GetSystemDefault() should be fine using the mono value.  But do we really 
want Tzdb.GetSystemDefault() to translate that through CLDR again?  It may be 
that /etc/localtime has a zone that gets lost in translation.

Or can Mono give it back to us in it's original form?

Original comment by mj1856 on 1 Aug 2013 at 1:45

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

Fundamentally using Tzdb.GetSystemDefault is broken unless TimeZoneInfo gives 
the Windows time zone ID, which I suspect it won't on Mono when not on Windows. 
I would *expect* Mono on non-Windows to give a TZDB name, in which case 
developers would need to take that and ask DateTimeProviders.Tzdb for that 
explicitly.

Yes, that's a bit odd - but I don't think we'd want to put that logic into Noda 
Time itself, as we don't *know* that Mono will use TZDB IDs anywhere anyway. 
It's all a mess - and I'd rather not try to fix it at all than fix it badly.

Original comment by jonathan.skeet on 1 Aug 2013 at 1:50

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Mar 15, 2015

In case somebody else comes across this in search of a solution on Linux/Mono, 
here is a workaround that works for me on Debian Wheezy:

        /// <summary>
        /// Alternative to NodaTime.DateTimeZoneProviders.Tzdb.GetSystemDefault that works on both Windows and 
        /// Linux/Mono systems.
        /// System.TimeZoneInfo.Local on Mono contains "Local" for the Local property.  NodaTime cannot deal with this,
        /// and throws DateTimeZoneNotFoundException whenever you call 
        /// NodaTime.DateTimeZoneProviders.Tzdb.GetSystemDefault().
        /// This workaround will, on PlatformID.Unix systems, manually open and read the content of the /etc/timezone
        /// file as the timezone ID.  Typically, this will be something like "US/Central" which NodaTime can indeed
        /// recognize and happily give us back a NodaTime.DateTimeZone.
        /// </summary>
        public static NodaTime.DateTimeZone GetSystemCurrentDateTimeZone()
        {
            // If we aren't running on Linux (which assumes Mono), then get the DTZ from NodaTime like normal.
            //
            if (Environment.OSVersion.Platform != PlatformID.Unix)
            {
                return NodaTime.DateTimeZoneProviders.Tzdb.GetSystemDefault();
            }

            // Otherwise we have to exert a little manual effort first.
            //
            string sTimezoneId; 

            try
            {
                using(var reader = File.OpenText("/etc/timezone"))
                {
                    sTimezoneId = reader.ReadLine();
                }
            }
            catch(Exception ex)
            {
                throw new Exception("Could not open and read '/etc/timezone'", ex);
            }

            return NodaTime.DateTimeZoneProviders.Tzdb.GetZoneOrNull(sTimezoneId);
        }

Original comment by BrandonLWhite on 5 Jun 2014 at 9:41

@malcolmr malcolmr added the bug label Mar 15, 2015

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

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