Time: honor Daylight Saving and ENV["TZ"]

[asterite]

With this, Time.now behaves exactly like in Ruby, and ENV["TZ"] is honored.

Running this script:

time_zones = [
  "",
  "America/New_York",
  "Africa/Luanda",
  "Africa/Tunis",
  "EST",
  "Europe/Andorra",
  "Europe/Berlin",
  "PST",
  "America/Noronha",
  "America/Belem",
  "America/Fortaleza",
  "America/Recife",
  "America/Araguaina",
  "America/Maceio",
  "America/Bahia",
  "America/Sao_Paulo",
  "America/Campo_Grande",
  "America/Cuiaba",
  "America/Santarem",
  "America/Porto_Velho",
  "America/Boa_Vista",
  "America/Manaus",
  "America/Eirunepe",
  "America/Rio_Branco",
  "America/Argentina/Buenos_Aires",
]
time_zones.each do |tz|
  ENV["TZ"] = tz
  puts "~~~"
  puts tz
  puts Time.now
end

produces the same output in Ruby and Crystal, so I guess this implementation is right.

It turned out that gettimeofday doesn't set the timezone and doesn't honor ENV["TZ"], or so it seems. Checking Ruby's source code I see they use other C functions. One of them is localtime_r, and that does set the daylight saving in the tm struct. I guess if it's tm_isdst is not zero then the change is by one hour, I don't know if in other places DST changes time in a different way.

It would be awesome if you can test this change in your timezone.

I added a spec to check that the timezone does change when changing ENV["TZ"], though testing that dst is picked up correctly is probably harder.

[luislavena]

Shouldn't TZ be restored to the previous value instead of set to empty string?

[asterite]

You are right, I'll fix it.

[jhass]

Maybe out of scope of this, but should Time.new assume a given localtime and set the right timezone at the given date, like in Ruby?

pry(main)> Time.new(2016, 6, 1)
=> 2016-06-01 00:00:00 +0200
pry(main)> Time.new(2016, 1, 1)
=> 2016-01-01 00:00:00 +0100

[asterite]

@jhass Ruby can do that because each Time carries a timezone. In Crystal (like in C#) a Time only knows if it's local or UTC (or that info is unknown).

To be honest, I don't really like that. I think we can make it so that each Time carries around timezone info. In the future we thought about adding another type for this, TimeWithZone, but it will make things harder to work with, I think. For example in Go a Time carries that info, so I don't see why we can't do that too. Time will be a bit bigger, probably not the size of an UInt64 anymore, but writing code will be easier, and we'll just need one Time type for all use cases.

@waj what do you think?

[asterite]

To add to the above comment, if I run this in Ruby:

ENV["TZ"] = "Europe/Berlin"
t1 = Time.now
puts t1

ENV["TZ"] = "America/New_York"
t2 = Time.now
puts t2

puts t1
puts t2

I get:

2016-04-19 22:18:14 +0200
2016-04-19 16:18:14 -0400
2016-04-19 22:18:14 +0200
2016-04-19 16:18:14 -0400

while in Crystal I get:

2016-04-19 22:18:05 +0200
2016-04-19 16:18:05 -0400
2016-04-19 22:18:05 -0400
2016-04-19 16:18:05 -0400

So the result is actually incorrect in Crystal if ENV["TZ"] is changed in the middle of a program and you have a reference to an old time. However, I don't know how frequently (or if) ENV["TZ"] is changed in the middle of a program.

In any case, I'd like Time to carry the timezone offset so the above works correctly.

By the way, if I run this in Ruby:

ENV["TZ"] = "Europe/Berlin"
t1 = Time.now

ENV["TZ"] = "America/New_York"
t2 = Time.now

puts t1
puts t2

I get:

2016-04-19 16:20:54 -0400
2016-04-19 16:20:54 -0400

Maybe a bug in Ruby?

[jhass]

Yes, let's have Time have a timezone, it makes things a lot saner IMO. If one needs to have more compact time they can work with unix timestamps or create a shard for that.

[jhass]

The needs for ENV["TZ"] to change within a program are probably rare, especially since only the process itself can do it. And if we let Time carry the zone and add methods to change it (zone=, in_zone), I can't imagine much need to do it.

Yet properly handling it would be better I think.

[asterite]

I don't think we'll add methods to change a time's properties, Time is immutable right now and that's why it can be a struct. We can probably have methods to return a new Time in a different zone.

[jhass]

Btw

$ jruby
ENV["TZ"] = "Europe/Berlin"
t1 = Time.now

ENV["TZ"] = "America/New_York"
t2 = Time.now

puts t1
puts t2
2016-04-19 22:32:47 +0200
2016-04-19 16:32:47 -0400
$ cat | rbx
ENV["TZ"] = "Europe/Berlin"
t1 = Time.now

ENV["TZ"] = "America/New_York"
t2 = Time.now

puts t1
puts t2
2016-04-19 16:34:31 -0400
2016-04-19 16:34:31 -0400

[ysbaddaden]

From my experiments and readings:

• gettimeofday is long deprecated and must be replaced with clock_gettime which comes with some great niceties (nanosecond precision, ability to ask for realtime or monotonic clocks...);
• you can't trust the timezone struct set by gettimeofday (eg: it's trash on musl-libc) and we must always pass it NULL;
• $timezone is nice but doesn't applies DST;
• $daylight can be used to know whether $timezone is enough (0) or if we must check for daylight too (1);
• using localtime_r and timegm and the tm struct yields correct offset. It's tm_gmtoff and tm_zone properties are correct after you call tzset at least once;
• I had no problem changing ENV["TZ"] and applying tzset to change the current locale.

I'm not sure we need to retain a timezone on Time. Wouldn't retaining the offset be enough? Along with a to_zone(name) method to change the offset (and only the offset). Now that I say this, retaining the zone is required when adding a Time::Span that could result in crossing the dates for summer/winter time, which would change the offset.

[ysbaddaden]

Also: just changing the TZ environment variable doesn't necessarily changes the timezone info. For example you must call tzset when using the *_r time variants. Calling localtime transparently calls tzset but localtime_r doesn't.

[ysbaddaden]

Bold assumption. Those 2 lines can be replaced with offset = tm.tm_gmtoff. Ideally:

ifdef darwin
  ret = LibC.gettimeofday(out timeval, nil)
  raise Errno.new("gettimeofday") unless ret == 0
  tv_sec, tv_usec = timeval.tv_sec, timeval.tv_usec
else
  ret = LibC.clock_gettime(LibC::CLOCK_REALTIME, out timespec)
  raise Errno.new("clock_gettime") unless ret == 0
  tv_sec, tv_usec = timespec.tv_sec, timespec.tv_nsec / 1_000
end

ret = LibC.localtime_r(pointerof(tv_sec), out tm)
raise Errno.new("localtime_r") unless ret == 0
offset = tm.tm_gmtoff / 60

ticks = tv_sec.to_i64 * Span::TicksPerSecond + tv_usec.to_i64 * 10_i64 + UnixEpoch
yield ticks, offset * Span::TicksPerMinute

[ysbaddaden]

BTW: what about using offset instead of tz, in order to reflect that offset is tz + dst?

[ysbaddaden]

Also we may:

if LibC.daylight == 0
  # current TZ doesn't have any DST, neither in past, present or future
  offset = LibC.timezone / 60
else
  # current TZ may have DST, either in past, present or future
  ret = LibC.localtime_r(pointerof(tv_sec), out tm)
  raise Errno.new("localtime_r") unless ret == 0
  offset = tm.tm_gmtoff / 60
end

[asterite]

@ysbaddaden I applied all your suggestions and they worked like a charm! ❤️🌟

Well, except clock_gettime. I'll do that next, but could you give me the real value of CLOCK_REALTIME for linux32 and linux64?

[asterite]

@ysbaddaden Nevermind, seems to be 0.

[jhass]

I don't follow the difference between this one and utc_ticks above, and why they're implemented differently.

[asterite]

I did some more refactors, and only use gettimeofday in darwin. I think there's a problem with the computed offset, it's always computed relative to "now", basically what @jhass showed: creating two Time instance where one falls in DST and the other doesn't, they will always show the offset for "now". This can be fixed, but not in this PR. It should be done in a separate one where we can improve Time's precision to nanosecond, and probably carry an offset/location with it (but this needs discussion, and I probably need to discuss it with @waj too). For now I think this is as good as it gets. At least now DST is honoured for "now", which was the main problem this PR tries to fix.

[ysbaddaden]

I agree this is enough for now. For future:

local_offset_in_minutes only returns the local offset that is only valid now. This leads to a bug in Time::Format::Formatter for example, where it assumes that the local offset is valid whatever the time of the year —DST breaks this promise, but timezones does break it too, since an offset may have changed at some point in time (eg: North Korea in 2015). I don't think we should retain this method.

What about we merely retain the offset value as utc_offset? After all this only a presentation problem since ticks are always UTC. This would simplify Time::Format::Parser that wouldn't have to apply the offset anymore; the formatter would merely utc_offset; to_local would involve calling LibC.localtime_r; to_utc would merely set the offset to 0 and new would also call LibC.localtime_r if the offset is nil. This will involve a bunch of calls to LibC.tzset thought, but I guess it's fast enough —ENV["TZ"] is ususally empty, and I suppose it won't reload /etc/localtime if it didn't change.

If you agree with that, I'll try to push a PR.

[asterite]

@ysbaddaden I agree, but I'll have to discuss this with @waj. I remember he told me, because that's how it's also done in C#, that in most cases you don't the timezone of a Time so it's much more lightweight if it's just represented as an UInt64... though probably nowadays it doesn't add much overhead if we carry more information (nanoseconds precision and timezone/offset info).