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

Daily rule occurs twice in a day when start is in last DST hour #189

Closed
forrest opened this issue Aug 15, 2013 · 14 comments
Closed

Daily rule occurs twice in a day when start is in last DST hour #189

forrest opened this issue Aug 15, 2013 · 14 comments
Labels

Comments

@forrest
Copy link

forrest commented Aug 15, 2013

Hey,

Getting this infinite loop causing all kinds of chaos right now. Any help would be greatly appreciated.

Forrest

Time.zone = "America/Denver"
start_at = Time.parse("Sun, 03 Nov 2013 01:30:00 MDT -06:00")
schedule = IceCube::Schedule.new(start_at)
schedule.add_recurrence_rule IceCube::Rule.daily
next_occurance = schedule.next_occurrence(start_at)
next_occurance.to_date.should_not == start_at.to_date # FAIL
@forrest
Copy link
Author

forrest commented Aug 15, 2013

https://github.com/forrest/ice_cube/compare/189_fix has the failing spec added.

@avit
Copy link
Collaborator

avit commented Aug 15, 2013

I'm not seeing the failure here:

>> next_occurrence = schedule.next_occurrence(start_at)
=> 2013-11-04 00:30:00 -0800

What's your system TZ by the way? (ENV['TZ'] || system("date '+%Z'"))

@avit
Copy link
Collaborator

avit commented Aug 15, 2013

Ok, I'm seeing the error now with start_at = Time.zone.parse instead of Time.parse

@forrest
Copy link
Author

forrest commented Aug 15, 2013

It fails if I get rid of the zone info all together:

  # DST in 2013 is November 6th -> 7th at 1:00AM
  it 'not changing days with daily rule going over DST boundary' do
    start_at = Time.parse("3 Nov 2013 01:00:00 MDT")
    schedule = IceCube::Schedule.new(start_at)
    schedule.add_recurrence_rule IceCube::Rule.daily
    next_occurance = schedule.next_occurrence(start_at)
    next_occurance.to_date.should_not == start_at.to_date
  end

or if I specify the zone.parse

    Time.zone = "America/Denver"
    start_at = Time.zone.parse("Sun, 03 Nov 2013 01:30:00 MDT -06:00")
    schedule = IceCube::Schedule.new(start_at)
    schedule.add_recurrence_rule IceCube::Rule.daily
    next_occurance = schedule.next_occurrence(start_at)
    next_occurance.to_date.should_not == start_at.to_date

@avit
Copy link
Collaborator

avit commented Aug 15, 2013

Here's what I see:

require 'active_support/time'
Time.zone = "America/Denver"
t0 = Time.zone.parse("Sun, 03 Nov 2013 01:30:00 MDT -06:00")
schedule = IceCube::Schedule.new(t0) { |s| s.rrule IceCube::Rule.daily }
schedule.first(3)
  1. Sun, 03 Nov 2013 01:30:00 MDT -06:00
  2. Sun, 03 Nov 2013 01:30:00 MST -07:00
  3. Mon, 04 Nov 2013 01:30:00 MST -07:00

Note the change in time zone between 1 and 2. Pretty sure this only affects daily rules... I was positive I had a test case for this in daily_rule_spec. 😒

@forrest
Copy link
Author

forrest commented Aug 16, 2013

You have a very similar test just above where I placed mine. Don't know why it's passing in one case and failing in another.

My infinite loop is partly based on the use in my code. I reset the day each time, so it's resetting back to -06:00. I can fix mine to avoid the loop. Unfortunately we'll still have an incorrect date in the middle of the run.

@avit
Copy link
Collaborator

avit commented Aug 16, 2013

Hey Forrest, I changed the title from "infinite loop" since that's not an accurate description: this issue is actually a pretty unique and somewhat interesting edge case. Allow me to expand on what's actually happening:

When DST is ending, the clock time "01:00:00" to "01:59:59" happens twice: once in summer/daylight time, and again when the clock rolls back to standard time. I distinguish these two instances of "1 AM" as the last hour of daylight time, and the first hour of standard time:

Term Description
DT Daylight Time (Summer), e.g. -0600 MDT
ST Standard Time (Winter), e.g. -0700 MST
DT Last Hour First Sunday in November, Between 01:00 DT and 1:59:59 DT
ST First Hour First Sunday in November, Between 01:00 ST and 1:59:59 ST

You'll find it's actually fairly unnatural to create a local Time within the DT Last Hour:

dt_time = Time.local(2013, 11, 3, 0, 59, 59)
#=> 2013-11-03 00:59:59 -0600

st_time = Time.local(2013, 11, 3, 1, 00, 00)
#=> 2013-11-03 01:00:00 -0700

These two look like they should be one second apart, in fact they are 1 hour + 1 second apart; the whole DT Last Hour is skipped. You can fool it with dt_time + 1 to "slide" into the DT Last Hour. And, of course, you can also get there using parse.

IceCube also jumps over the DT Last Hour when traversing daily rules, so that:

  • We follow the ruby way when using Time.new (.local) to avoid ambiguity
  • We avoid the possibility of duplicates by traversing both DT Last Hour & ST First Hour.

The only time you'll run into your double-occurrence problem is when you explicitly create a daily schedule that starts within DT Last Hour. Normally when traversing over DST, the intervals between daily occurrences are:

Occurrence interval to previous time
2013-11-01 01:30:00 -0600 start = 0
2013-11-02 01:30:00 -0600 24 hours
2013-11-03 01:30:00 -0700 25 hours
2013-11-04 01:30:00 -0700 24 hours

(In March there's a 23 hours)

When you create a schedule that actually starts in DT Last Hour, that's not a "normal" time we'd ever traverse for a daily rule, so your initial start time before the schedule adjusts itself to get on the right track is an extra:

Occurrence interval to previous time
2013-11-03 01:30:00 -0600 start = 0
2013-11-03 01:30:00 -0700 1 hour
2013-11-04 01:30:00 -0700 24 hours

I would recommend not creating things that start in DT Last Hour. Traversing over it works fine, just not as the start time.

I suppose that's not 100% avoidable, so it might be reasonable to skip the "correct" scheduled daily occurrence for Nov 3 so there aren't two occurrences on the same day. It just means the 25-hour interval happens one day later than normal:

Occurrence interval to previous time
2013-11-03 01:30:00 -0600 start = 0
2013-11-04 01:30:00 -0700 25 hours
2013-11-05 01:30:00 -0700 24 hours

Does that make sense?

@forrest
Copy link
Author

forrest commented Aug 16, 2013

Hey @avit,
Thanks for the extremely thorough investigation and write up. What you're saying makes sense. Unfortunately, as it's client input into a multi-tenant system which is supporting multiple timezones, we can't entirely avoid this situation. I put a super ugly hack into our code to prevent things from getting out of hand, but this is just a temporary fix.

Is it possible to add a check in ValidatedRule#shift_time_by_validation to check if the time is in the DT Last Hour? If so, it could set dst_adjust to true? Just a thought, as you know this library significantly better than me.

@ajsharp
Copy link
Contributor

ajsharp commented Nov 4, 2013

Would love to see a follow-up to this issue. Could definitely see other people getting bitten by this. Thanks @forrest and @avit for digging into it.

@avit
Copy link
Collaborator

avit commented Nov 5, 2013

What Would iCal Do?

screen shot 2013-11-05 at 10 00 00 am

This is:

  • start_time: 1:00 AM (before rollback; i.e. last DT hour)
  • until: 2:00 AM
  • repeat: daily

That first occurrence is also 2 hours long, but the second 1AM is not shown. There is no second occurrence after the clock change, and the 1 hour time difference is deferred until the next day. That's what I was suggesting, so let's do it that way. (I haven't looked at fixing this yet, @ajsharp are you keen to take a look?)

@ajsharp
Copy link
Contributor

ajsharp commented Nov 5, 2013

@avit Thanks for mapping this out. I don't have time to dig into this issue this week, but I might next. For clarity's sake, let me see if I have this straight:

There is no second occurrence after the clock change, and the 1 hour time difference is deferred until the next day.

So, in iCal you scheduled a recurring daily two-hour appointment on Sunday. Are you saying that the occurrence on November 3rd at 1AM (the last hour of daylight savings time) was basically cut into two one-hour occurrences? One one-hour occurrence that began at 1am before the clock rolled back, and one one-hour occurrence that occurred after the clock rolled back, also at 1am?

I think that makes sense (insofar as daylight savings itself "makes sense").

Disclaimer: I haven't thought about this problem nearly as hard as you guys have, so please excuse any ignorance around this :)

@avit
Copy link
Collaborator

avit commented Nov 5, 2013

Are you saying that the occurrence on November 3rd at 1AM (the last hour of daylight savings time) was basically cut into two one-hour occurrences?

No. It's still one occurrence, duration 2 hours, but, there's only one "1 AM" shown on the grid. The other days are also 2h durations, so they stop at 2:59:59 instead. Just for giggles, I tried creating an event for each "1AM", and they show up side-by-side even though they're 1 hour apart:

screen shot 2013-11-05 at 11 56 17 am

Not that this has anything to do with IceCube, just showing that everyone has their own problems dealing with DST.

I think that makes sense (insofar as daylight savings itself "makes sense").

Totally... 😖

@ajsharp
Copy link
Contributor

ajsharp commented Nov 5, 2013

No. It's still one occurrence, duration 2 hours, but, there's only one "1 AM" shown on the grid.

Ah, got it, thanks for explaining.

That handling makes sense for this particular example (a 2-hour appointment where the first hour coincides with the last hour of daylight savings). However, let me throw out a couple of other examples that don't quite compute to me, at least not in a way that I can think about them in an algorithmic way:

  • Scheduling a 1-hour occurrence that starts at the same time as the first hour of standard time (ST).
  • Scheduling a 3-hour occurrence that starts at 1AM DT on November 3rd (the example you've laid out here, but 3 hours instead of two).

@avit
Copy link
Collaborator

avit commented Nov 5, 2013

Scheduling a 1-hour occurrence that starts at the same time as the first hour of standard time (ST).

No problem here. First hour of ST has already rolled over and is "on track".

Scheduling a 3-hour occurrence that starts at 1AM DT on November 3rd (the example you've laid out here, but 3 hours instead of two).

We convert end_time into duration internally, so whatever is the duration of the first occurrence will extend to all occurrences. In this case, because it's created in that limbo hour, the occurrences will have an extra hour duration if you defined it using an end_time.

Note from my first iCal screenshot above, it looks like I'm setting up 1:00AM to 2:00AM, which looks like 1 hour duration... but actually it's two hours because of the time difference from 1:00AM DT to 2:00AM ST.

@avit avit closed this as completed in c93fa7e Mar 25, 2014
avit added a commit to avit/ice_cube that referenced this issue Jul 9, 2017
Simplify the method we use for calculating time changes across DST boundaries by
performing arithmetic under UTC to get "absolute" values. This is a revert of
the "unrolling in pairs" method introduced in c93fa7e for fixing ice-cube-ruby#189.

The method of converting to UTC and back for doing time arithmetic only applies
when DST needs to be taken into consideration. For rules with less than daily
intervals, both hours still need to occur. This distinction was added in the
original fix.

However, the method was not correctly tested for non-ActiveSupport times (local
system TZ) and the advancing of time to the next occurrence was validating both
the last DT hour (before fall back) and first ST hour (after fall back) causing
double occurrences for the "same" hour on the clock face.

Fixes ice-cube-ruby#398
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants