Wrong datetime result with Cron.next() with seconds='*'

[larsjaas]

I wrote a cron schedule string wrong, and got some strange behaviour in return.

import cron4s._
import cron4s.lib.javatime._
import java.time._
import java.time.temporal.ChronoUnit
import org.scalatest.{FlatSpec, Matchers}

class Cron4sSpec extends FlatSpec with Matchers {
    "Cron" should "not advance to the next day" in {
        val from = LocalDateTime.parse("2017-02-18T16:39:42.541")
        val Right(cron) = Cron("* */10 * * * *")
        val next = cron.next(from).get

        println(s"from: ${from}")
        println(s"next: ${next}")

        from.until(next, ChronoUnit.SECONDS) <= 600 shouldBe true
    }
}

Output is

from: 2017-02-18T16:39:42.541
next: 2017-02-19T16:39:43.541

Notice the next time is the next day, not 2017-02-18T16:40:00.000.

If I use the Cron schedule 0 */10 * * * *, which was what I wanted to use, the result is correct (although the milliseconds from the from-datetime is carried over, not reset to 0).

[alonsodomin]

Yeah, that's weird, the usage of the * in the seconds field should not affect the day field, specially taking into account that hour field remains stable.

Thanks for posting the test that shows the issue two, I'm going to use it as regression test to find out what wrong thing is going on in there.

Regarding the milliseconds fields that's somewhat expected given implementation since when stepping over the date time, only the fields that belong to the cron expression are modified and those that have no representation in the expression are left untouched. That allows getting expected results when using the subexpressions (i.e.: date or time only).

I understand what you mean though, setting the seconds field to the constant 0, somehow builds the expectation that the milliseconds will set to 0 too... need to give it a thought, on how to implement it though whilst still providing good interaction with other features. Personally, not a fan of building special case behaviours depending on the position of a field, and need to see how this would interact with the implementation of support for the symbol ?, which I'd like to use at the seconds, day of month, and day of week fields.

[larsjaas]

I suppose the milliseconds issue can be a matter of interpretation, but my expectation of it being set to zero comes from the cron expression string itself (the time matching specification).

Let's say a task takes 100ms to perform. I set up "* * * * * *" as the CronExpr. Each time after the task is performed, I ask CronExpr to find the next once-a-second time based on the current time. I would expect to perform the task 60 times a minute, only slipping if the task itself lasted longer than one second. But because of drift caused by the current implementation I guess it would only be performed 54 times?

[alonsodomin]

I see your point, would you mind to open an issue for the milliseconds case? as I think this is a separate concern from that weird behaviour you described before...

Will try to look at during this week. The issue with milliseconds should be easy to solve, but u just got me in the middle of a work trip so I need to find a slot to properly review it.

[larsjaas]

Agreed that they are two completely separate issues.
Opened #51.

[alonsodomin]

The actual problem is in the general stepping algorithm failing to correctly set the nearest neighbour when there is a carry over value of 0 from the seconds field over to the minutes field, which has the */10 constraint.

The change on the day of month field is a consequence of that. The stepping over the date part is recursive to compensate for adjustments in the day of week field relying in whether the final date matches the original expression or not. But in this case it gives up after one recursive step (in which it has increased the day of month field) as it can't find a date that actually can match the expression).

I should be possible to fix by enforcing stricter law-checking of the Enumerated type-class (where the implementation for the field stepping algorithm lives). Will try to get a fixed prototype in the next days.

[alonsodomin]

Both issues have been fixed and a new release has been sent to Maven Central. It will take a couple of hours for it to be available in all regions. It would be great if you could try it at some point.

[larsjaas]

I'm not getting the results I expected now either (with 0.3.1), but at least the date-advance-issue is gone.

    val from = LocalDateTime.parse("2017-02-18T16:39:42.541")
    val next = cron.next(from).get
    println(s"from: ${from}")
    println(s"next: ${next}")
    from.until(next, ChronoUnit.SECONDS) shouldBe 17L // I tightened up this test

gives

from: 2017-02-18T16:39:42.541
next: 2017-02-18T16:40:43
CronExpr
- should not advance to the next day *** FAILED *** (568 milliseconds)
  60 was not equal to 17 (CronExprSpec.scala:14)

I would expect the seconds='*' to cause the 'next' instance to be at 16:40:00, and the next after that to be 16:40:01.

I doubt this is a much used usecase (I don't use it myself even) so it doesn't stop me from using cron4s in practice.

[alonsodomin]

Yeah, sort of aware of this, the seconds field in this case needs to advance up to 0. Same should happen with an expression in the likes of * * */2 * * * and a timestamp 2017-02-18T15:39:42.541 as from, in such a case, the expected result should be 2017-02-18T16:00.

On top that, there is the case that neither * */10 * * * * or * * */2 * * * should be valid cron expressions, they must have a ? in either the day of month or day of week fields, which is not supported yet...

Right now, in a way, the * symbol is mixing himself up with the ?. This is definitively an issue, but probably not easy to solve until I introduce proper support for the ? and therefore properly separate the behaviour of those two.

Sorry for the inconvenience of not being yet fully feature complete ... I'll create a ticket to properly propagate the the stepping effect over the *'s

[larsjaas]

Was not aware of that, and I've always thought * could go in all fields. Is there really a formal cron format specification / standard? I ported from cronutils, which let you set a format type, and there were formats with 5 fields and 6 fields (probably differing on including seconds) and I think different ways of specifying weekdays.

[alonsodomin]

Not really aware of a pure formal Cron specification. The unix cron is 5 fields (does not allow seconds) but a very common one in JVM-land is the Quartz kind, which includes the seconds field by default and even the possibility of a year field (this website renders cron expressions using that format: http://www.cronmaker.com). And yeah, on top of that, there is not strict consensus on how to number weekdays.

I tried to stick myself to the Wikipedia page on Cron, although I did like the additional precission of specifying seconds that Quartz gives:
https://en.wikipedia.org/wiki/Cron

One thing seems to be clear, there is a need for ? for either the day of month or day of week, mostly due to how "entangled" those two fields are to each other. Right now, in cron4s, I have to do a bounded recursive loop to make them match, which would be unnecessary with proper support for ?, reducing the full step algo in just two passes, one applying the field constrains, and the second one applying corrections.