Skip to content

Resolve the problem of 59th second while discovering timeperiods

The problem was described during discussion at dd4d270d:

We have two problems here, and I just left one of them (and we can discuss if this is good or not).

  1. (...)
  2. The specific case, and the bug inside of InPeriod(), is that the period is not discovered properly if you're at the 59 second of the minute. Let's consider two example time moments and analyze how InPeriod() will work:
    • We have a period pattern * 0 14 * * * *.
    • For time 2017-02-21T14:00:00 the expression.Next(now) will return us 2017-02-21T14:00:01. The difference between the current time and nextIn is inside of [-1s, 1s] interval, so we're assuming that the time is inside of defined period. The same would happen for each second up to 2017-02-21T14:00:58. Each time nextIn will return a time with +1s value.
    • For time 2017-02-21T14:00:59 however, the expression.Next(now) will return us 2017-02-22T14:00:00. This is done because of how github.com/gorhill/cronexpr works. In this case the difference between nextIn and current time is -23h59m1s which is a way out of the defined [-1s, 1s] interval.

Now, the problem of 59th second is a bug, but it's forced by the behavior of used library. We should consider if this is really a bug, and if we decide that it is, we should send a patch to the library's upstream. However, it's hard for me to imagine that someone will want to define the OffPeakPeriod with a resolution measured in seconds. Then, maybe, the easiest solution would be to cut the first part of the period (which for github.com/gorhill/cronexpr means seconds) and force 0 there.

The fix that I've made is however not a solution for the bug, but for randomly failing tests. The previous version was not trustworthy, because it was failing randomly only when the test was executed at hh:mm:59. In any other time it was passing without problems. This was occurring because we were partially controlling the environment - using strict hours and minutes, but not seconds. With current version the test will not fail in random moments. But to notice the problem of 59th second I'll push a updated version in a moment.

We should decide if:

  • we're OK with the problem of 59th second,
  • this is a bug and we should fix this in Runner (e.g. by altering the seconds field of each periodPattern),
  • this is a bug and we should send a fix to the library's upstream.