taichino / croniter

croniter is a python module to provide iteration for datetime object.
http://github.com/taichino/croniter
387 stars 105 forks source link

"* * R/0 * *" causes ZeroDivisionError #164

Closed cuu508 closed 2 years ago

cuu508 commented 3 years ago

Test case:

Python 3.8.5 (default, Jan 27 2021, 15:41:15) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from datetime import datetime
>>> from croniter import croniter
>>> 
>>> croniter("* * R/0 * *", datetime)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "(...)/croniter.py", line 150, in __init__
    self.expanded, self.nth_weekday_of_month = self.expand(expr_format, hash_id=hash_id)
  File "(...)/croniter.py", line 755, in expand
    return cls._expand(expr_format, hash_id=hash_id)
  File "(...)/croniter.py", line 614, in _expand
    expr = expander(cls).expand(efl, i, expr, hash_id=hash_id)
  File "(...)/croniter.py", line 896, in expand
    self.do(
  File "(...)/croniter.py", line 854, in do
    return ((crc >> idx) % (range_end - range_begin + 1)) + range_begin
ZeroDivisionError: integer division or modulo by zero
>>> 
cuu508 commented 3 years ago

Another one: "0 0 H(0-0)/0 * *"

I'm looking at HashExpander.expand() and the two places where divisor gets passed as range_end to the HashExpander.do(). I suspect these are both incorrect but I don't know what the original author's intent was.

For example, consider this snippet:

from datetime import datetime
from croniter import croniter
from croniter.croniter import HashExpander

pattern = "R/5 * * * *"
c = croniter(pattern, datetime)
h = HashExpander(c)

for i in range(0, 10):
    print(h.expand(pattern, 0, "r/5", "r"))

It prints out:

2-59/5
5-59/5
4-59/5
4-59/5
0-59/5
5-59/5
3-59/5
2-59/5
3-59/5
1-59/5

In the expanded expression, the range start is never above 5. That's because divisor (5) was passed in as range_end to HashExpander.do().

kiorky commented 3 years ago

You ll have to ask @rfinnie which is the original author !

cuu508 commented 3 years ago

I'm looking at examples in Jenkins docs –

image

And using divisor as the start of the range starts to make sense. I'll stare some more at the code :-)

cuu508 commented 3 years ago

I submitted a PR which adds a range check. The net result is * * R/0 * * now triggers a CroniterBadCronError instead of ZeroDivisionError.

There's one more thing I'm not sure about, @rfinnie perhaps you can chime in. One of the above Jenkins examples is "H(0-29)/10". Its description is "every ten minutes in the first half of every hour (three times, perhaps at :04, :14, :24)".

If I feed "H(0-29)/10" to HashExpander, it will give one of the:

0-29/10
1-29/10
2-29/10
3-29/10
4-29/10
5-29/10
6-29/10
7-29/10
8-29/10
9-29/10
10-29/10

All excpressions except the last one would match three times. For example, 3-29/10 would match :03, :13 and :23. However, 10-29/10 would match only two times: :10 and :20. Is this consistent with how Jenkins handles the H symbol?

kiorky commented 3 years ago

please @rfinnie ?

rfinnie commented 3 years ago

I submitted a PR which adds a range check. The net result is * * R/0 * * now triggers a CroniterBadCronError instead of ZeroDivisionError.

There's one more thing I'm not sure about, @rfinnie perhaps you can chime in. One of the above Jenkins examples is "H(0-29)/10". Its description is "every ten minutes in the first half of every hour (three times, perhaps at :04, :14, :24)".

If I feed "H(0-29)/10" to HashExpander, it will give one of the:

0-29/10
1-29/10
2-29/10
3-29/10
4-29/10
5-29/10
6-29/10
7-29/10
8-29/10
9-29/10
10-29/10

All excpressions except the last one would match three times. For example, 3-29/10 would match :03, :13 and :23. However, 10-29/10 would match only two times: :10 and :20. Is this consistent with how Jenkins handles the H symbol?

Oops, sorry I didn't notice being highlighted. I think the behavior is correct and the Jenkins example is simply wrong -- if you want a hashed distribution where each would guarantee to be run 3 times within a 30 minute period, I think H(1-30)/10 would be the only expression which would satisfy those requirements. But I'll see if I can verify this weekend.

kiorky commented 3 years ago

Oops, sorry I didn't notice being highlighted. I think the behavior is correct and the Jenkins example is simply wrong -- if you want a hashed distribution where each would guarantee to be run 3 times within a 30 minute period, I think H(1-30)/10 would be the only expression which would satisfy those requirements. But I'll see if I can verify this weekend.

When you have your test running, add it in a mr as a unit test than we can refer at a later time, i tend to be a goldfish !

cuu508 commented 2 years ago

I think the behavior is correct and the Jenkins example is simply wrong

– good enough for me :-), closing.