Added support for DST with configurable behavior

[abecciu]

The current implementation does not handle DST correctly. In some particular cases, it causes infinite loops or returns nonsense time values.

This PR adds formal support for DST and lets the user customize the behavior according to his needs.

In the case of a DST leap, there's the option to unskip events that would have been skipped when to clock moves forward. And in the case of DST fall, there are options to fire early, late, or both times for events that would be repeated when the clock moves backward.

A new method (ParseWithOptions) is added to configure the parser with the DST options. And the Parse method sets DSTLeapUnskip and DSTFallFireEarly options by default.

[cshiong]

when this will be merge to master?

[matthewmueller]

any chance we could get this reviewed and merged @gorhill ?

[gorhill]

It's not a project on which I am working anymore -- so to have a pull request with such amount of changes is not something I feel comfortable bringing in. I have a gut feeling that as I understand the issue, the solution could be simpler: a wrapper around Next() which would simply check if the output time is less than or equal to the input time, and when this occurs it simply calls again once more Next().

[cshiong]

this code change has a serious issue.
when set {DSTFlags: cronexpr.DSTLeapUnskip | cronexpr.DSTFallFireLate})
which mean unskip DST leap time and all job scheduled during that hour (2:01am-2:59am) will fall at 3am. this works as expected, but the issue is all the job scheduled after 3am in DST start day will all set at 3am too. no matter what time it is like 8:19 or 19:27 etc.

[matthewmueller]

That's fair and thanks for the response. I'd like to pull in this library, but the issues regarding infinite loops during DST worry me. I'm wondering if there's a simple fix to address that issue? Missing an event during a timezone shift is okay to me though. Thu, Jan 5, 2017 at 1:16 PM cshiong <[email protected]> wrote:

…

this code change has an serious issue. when set {DSTFlags: cronexpr.DSTLeapUnskip | cronexpr.DSTFallFireLate}) which mean unskip DST leap time and all job scheduled during that hour (2:01am-2:59am) will fall at 3am. this works as expected, but the issue is all the job scheduled after 3am in DST start day will all set at 3am too. no matter what time it is like 8:19 or 19:27 etc. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKZOxVs0sBr43in8Rea0S92igBJ-y59ks5rPUGVgaJpZM4Kj8Sy> .

[abecciu]

@cshiong interesting. I can't reproduce that behavior. Could you please send me a gist with a code example? Thanks!

[abecciu]

@gorhill my implementation is kind of a wrapper around Next(). The thing is handling DST correctly with certain flexibility is not as simple as just "calling once moreNext()".

I'd appreciate it if you can take a closer look at the implementation.

[abecciu]

@matthewmueller please, give this PR a try and let me know if you find anything wrong. Works perfectly for all my use cases. It should for yours too.

[cshiong]

@abecciu here is the sample:

{DSTFlags: cronexpr.DSTLeapUnskip | cronexpr.DSTFallFireLate})
fromTimeS := "2016-03-12T14:06:00-08:00"
expectedExecutionTimeS := "2016-03-13T14:05:00-07:00"

Schedule: "5 14 * * * *",
Timezone: "America/Los_Angeles",

but the actual time set to :2016-03-13T03:00:00-07:00

[abecciu]

@cshiong thanks for the example! Just fixed it, please try again.

[matthewmueller]

Thanks! Will definitely give it a go and let you know!

…

On Thu, Jan 12, 2017 at 1:10 PM Augusto Becciu ***@***.***> wrote: @cshiong <https://github.com/cshiong> thanks for the example! Just fixed it, please try again. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKZO75H8soNf9F_gNthP-jGoXwo0G1Qks5rRcPwgaJpZM4Kj8Sy> .

[cshiong]

it works fine now, Abecciu fixed the bug, all my test cases passed. thanks!

[matthewmueller]

@abecciu just had a chance to test out your dst-support branch. It seems to be working for the leap forward, but not the leap backward. Here's what I'm testing:

cron, err := cron.Parse("0 * * * *")
if err != nil {
	t.Fatal(err)
}

layout := "2006-01-02 15:04:05"
loc, err := time.LoadLocation("America/Los_Angeles")
if err != nil {
	t.Fatal(err)
}

first, err := time.ParseInLocation(layout, "2016-01-01 00:00:00", loc)
if err != nil {
	t.Fatal(err)
}

last, err := time.ParseInLocation(layout, "2016-12-29 23:00:00", loc)
if err != nil {
	t.Fatal(err)
}

times, err := cron.NextN(first, uint(last.Sub(first).Hours()))
if err != nil {
	t.Fatal(err)
}

for i, ti := range times {
	fmt.Printf("%d: %s\n", i, ti.Format(layout))
}

On the leap forward on Sunday, March 13, 2:00 am 2016:

1727: 2016-03-13 00:00:00
1728: 2016-03-13 01:00:00
1729: 2016-03-13 03:00:00
1730: 2016-03-13 04:00:00
1731: 2016-03-13 05:00:00

On the fall back on Sunday, November 6, 2:00 am 2016:

7438: 2016-11-06 00:00:00
7439: 2016-11-06 01:00:00
7440: 2016-11-06 02:00:00
7441: 2016-11-06 03:00:00
7442: 2016-11-06 04:00:00
7443: 2016-11-06 05:00:00

Shouldn't there be two 2016-11-06 01:00:00 in that case? I'd really appreciate your help – thanks!

---

edit: looking at the conversion to UTC, it looks like it's being skipped over:

7438: 2016-11-06 00:00:00 –> 2016-11-06 07:00:00
7439: 2016-11-06 01:00:00 –> 2016-11-06 08:00:00
7440: 2016-11-06 02:00:00 –> 2016-11-06 10:00:00
7441: 2016-11-06 03:00:00 –> 2016-11-06 11:00:00
7442: 2016-11-06 04:00:00 –> 2016-11-06 12:00:00
7443: 2016-11-06 05:00:00 –> 2016-11-06 13:00:00

[abecciu]

@matthewmueller you're using the default settings which are set for firing only once (the earliest time) in case of backward leap.

If you want it to fire both times, you have to instantiate your parser like so:

cron, err := cronexpr.ParseWithOptions("0 * * * *", cronexpr.Options{
    DSTFlags: cronexpr.DSTLeapUnskip | cronexpr.DSTFallFireEarly | cronexpr.DSTFallFireLate
})

Let me know if that works for you.

[matthewmueller]

@abecciu Ahh got it! I also realized that the default implementation is probably what I want. Thanks for your help!