Time#change doesn't work when DST just changed #12163

Closed
vicentllongo opened this Issue Sep 8, 2013 · 17 comments

Comments

Projects
None yet
6 participants

Not 100% sure about the reason, so I'm not gonna speculate :)

I can reproduce this in Rails 3.2.14 and 4.0.0:

>> timezone = ActiveSupport::TimeZone.new('Europe/Madrid')
>> time = timezone.at(1319936400) #DST just changed from CEST to CET
>> time.inspect
=> Sun, 30 Oct 2011 02:00:00 CET +01:00
>> time.to_i
=> 1319936400
>> time = time.change(min: 0) # time should stay the same, but it won't
>> time.inspect
=> Sun, 30 Oct 2011 02:00:00 CEST +02:00
>> time.to_i
=> 1319932800

The same thing happens on any change inside that hour: min, sec or/and usec.

Contributor

rajcybage commented Sep 9, 2013

it is only happening for 'Europe/Madrid' in my case it is working fine

   irb(main):001:0> timezone = ActiveSupport::TimeZone.new('Eastern Time (US & Canada)')
 => #<ActiveSupport::TimeZone:0x90ad5f @current_period=nil, @name="Eastern Time (US & Canada)",       @utc_offset=nil, @tzinfo=#<TZInfo::TimezoneProxy: America/New_York>>

 irb(main):010:0> time=timezone.at(115200)
   => Fri, 02 Jan 1970 03:00:00 EST -05:00

  irb(main):011:0> time2 = time.change(min: 0)
  => Fri, 02 Jan 1970 03:00:00 EST -05:00  #same as before

  irb(main):012:0> time.to_i  
  => 115200

  irb(main):013:0> time2.to_i
  => 115200 #same as before

This only happens when DST changes after summer, in the case of the timezone of your example, for last year it happened the 4th of November at 02:00:00 EDT, which became 01:00:00 EST.

I think the problem is that "2012-11-04 01:00:00" happened twice, and Time#change is not aware of dst.

>> timezone = ActiveSupport::TimeZone.new('Eastern Time (US & Canada)')
 => #<ActiveSupport::TimeZone:0x007fd38088b070 @name="Eastern Time (US & Canada)", @utc_offset=nil, @tzinfo=#<TZInfo::TimezoneProxy: America/New_York>, @current_period=nil> 
>> time = timezone.at(1352008800)
 => Sun, 04 Nov 2012 01:00:00 EST -05:00 
>> time2 = time.change(min: 0)
 => Sun, 04 Nov 2012 01:00:00 EDT -04:00 
>> time.to_i
 => 1352008800 
>> time2.to_i
 => 1352005200
Contributor

rajcybage commented Sep 9, 2013

👍

But for many cases it will be false and every time we have to check dst? you can simply convert it to utc

Owner

rafaelfranca commented Sep 12, 2013

cc @pixeltrix, this is too much black magic to my brain

This is also a problem for Time#beginning_of_hour and friends, because they rely on Time#change. Also of note is that Time and TimeWithZone both show this bug, but behave differently! I don't know how to set up a test that works consistently for Time, since it uses the local time zone.

require "active_support/time"
Time.zone = ActiveSupport::TimeZone["Central Time (US & Canada)"]

# Nov 3, 2013 is on the DST boundary in most of the US
t = Time.parse("2013-11-3 01:30:00") # => 2013-11-03 01:30:00 -0600
s = Time.zone.parse("2013-11-3 01:30:00") # => Sun, 03 Nov 2013 01:30:00 CDT -05:00
s == t - 1.hour # => true
s.beginning_of_hour == (t - 1.hour).beginning_of_hour # => false (!!!)

(t - 1.hour) > (t - 1.hour).beginning_of_hour # => false (!!!)
s > s.beginning_of_hour # => true

t < t.end_of_hour # => true
(s + 1.hour) < (s + 1.hour).end_of_hour # => false (!!!)

I installed ActiveSupport purely because time is so complicated it seemed wiser to rely on a tried-and-true implementation than to try to roll my own, so I would certainly like to see this fixed.

Owner

pixeltrix commented Oct 22, 2013

The problem stems from the fact there are two 02:00:00 - 03:00:00 time periods - one for CEST and one for CET. The ActiveSupport::TimeZone#period_for_local method basically delegates to the TZInfo gem method of the same name which returns the first one (CEST).

The only possible fix is to check to see if the current period is in the list of periods returned by periods_for_local and use that one in preference to the first one in the list - this means any change options that fall within that hour will keep the same period. Does this seem acceptable?

This sounds good to me. I think it would also be good to patch either Time or TimeWithZone so that parse()ing a time in an ambiguous period yields consistent results between the two classes.

After further consideration, I tentatively propose this alternate behavior: if t is in one of the periods returned by periods_for_local, choose that period; otherwise, if t is after the last period, choose the last period; otherwise, t must be before the first period, in which case choose the first period. This makes Time.change more consistent with arithmetic operations on Times and Durations, but less consistent with things like Time.new. It's also convenient for rounding Time objects to the nearest few hours. The same would apply to TimeWithZone objects. What do you think?

Owner

pixeltrix commented Oct 23, 2013

The point is that t exists in both periods, if it's before the start of the overlap then periods_for_local will only return a single period and if it's after the overlap then it will also only return a single period. The solution I propose is that we keep the time zone consistent if possible and this will also fix beginning_of_hour, end_of_hour, etc.

Sorry, I did not make it at all clear that I meant t to be the original time and t.change(...) the new time. So the proposal is that we choose the new time to be in the period closest to the original time, if the original time is not in any of the periods under consideration for the new time. That is, this would happen:

require "active_support/time"
Time.zone = ActiveSupport::TimeZone["Central Time (US & Canada)"]

Time.parse("2013-11-3 00:00:00").change(hour: 1, min: 45)
# => 2013-11-03 01:45:00 -0500 (currently -0600)
Time.parse("2013-11-3 12:00:00").change(hour: 1, min: 45)
# => 2013-11-03 01:45:00 -0600

Time.zone.parse("2013-11-3 00:00:00").change(hour: 1, min: 45)
# => Sun, 03 Nov 2013 01:45:00 CDT -05:00
Time.zone.parse("2013-11-3 12:00:00").change(hour: 1, min: 45)
# => Sun, 03 Nov 2013 01:45:00 CST -06:00 (currently CDT -05:00)

Oh, of course. That's what you said. Yes, I agree.

jarmo commented Nov 4, 2013

We have the same problem. Check this out:

require "active_support/all"

> after_dst_change = Time.new(2013, 10, 27, 3, 0, 0).in_time_zone("EET")
=> Sun, 27 Oct 2013 03:00:00 EET +02:00

> before_dst_change = after_dst_change - 3600
=> Sun, 27 Oct 2013 03:00:00 EEST +03:00

Let's use #change for the before_dst_change:

> before_dst_change.change(min: 0)
=> Sun, 27 Oct 2013 03:00:00 EEST +03:00

Everything works as expected. Now let's try the same thing with after_dst_change:

> after_dst_change.change(min: 0)
=> Sun, 27 Oct 2013 03:00:00 EEST +03:00

This is totally unexpected and wrong. What is the possible solution for this problem?

Owner

pixeltrix commented Nov 4, 2013

@jarmo I have a patch for this but it will be master only since it's a change of behaviour - I'll post a monkey patch here that you can put in an initializer.

jarmo commented Nov 4, 2013

@pixeltrix Well, it's a fix for a definite bug. It does not make sense to release those things, hehe.

But a temporary patch is appreciated too, thank You!

Owner

pixeltrix commented Nov 4, 2013

@jarmo whether it's a bug or change of behaviour depends on whether you're affected by it now or after I'd backported it :trollface:

jarmo commented Nov 4, 2013

Are you saying that there is a chance that when you use #change during the change of DST then sometimes you are affected and sometimes you aren't?

As i understand then you're always affected, but you just might not have noticed it. I did, because it affects quite a critical part of our application.

pixeltrix was assigned Jan 5, 2014

pixeltrix closed this in b594492 Jan 26, 2014

Owner

pixeltrix commented Jan 26, 2014

Here's a patch if you need to make it work in a Rails 4.0 application:

require 'active_support/time'

module ActiveSupport
  class TimeWithZone

    def initialize(utc_time, time_zone, local_time = nil, period = nil)
      @utc, @time_zone, @time = utc_time, time_zone, local_time
      @period = @utc ? period : get_period_and_ensure_valid_local_time(period)
    end

    def change(options)
      new_time = time.change(options)
      periods = time_zone.periods_for_local(new_time)
      self.class.new(nil, time_zone, new_time, periods.include?(period) ? period : nil)
    end

  private

    def get_period_and_ensure_valid_local_time(period)
      # we don't want a Time.local instance enforcing its own DST rules as well,
      # so transfer time values to a utc constructor if necessary
      @time = transfer_time_values_to_utc_constructor(@time) unless @time.utc?
      begin
        period || @time_zone.period_for_local(@time)
      rescue ::TZInfo::PeriodNotFound
        # time is in the "spring forward" hour gap, so we're moving the time forward one hour and trying again
        @time += 1.hour
        retry
      end
    end
  end

  class TimeZone
    def periods_for_local(time)
      tzinfo.periods_for_local(time)
    end
  end
end

@pixeltrix pixeltrix added a commit that referenced this issue Jan 31, 2014

@pixeltrix pixeltrix Maintain the current timezone in wrap_with_time_zone
Extend the solution from the fix for #12163 to the general case where
`Time` methods are wrapped with a time zone.

Fixes #12596.
63f8fab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment