vobject icon indicating copy to clipboard operation
vobject copied to clipboard

Fix infinite loop caused by yearly with bySetPos

Open liurxliu opened this issue 3 years ago • 3 comments

Consider the rrule: FREQ=YEARLY;BYMONTH=5;BYSETPOS=3;BYMONTHDAY=3

It will not generate any occurrence, but the RRuleIterator will stuck in the infinite loop

liurxliu avatar Feb 14 '22 10:02 liurxliu

Codecov Report

Merging #564 (1c66700) into master (06feff3) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #564   +/-   ##
=========================================
  Coverage     98.34%   98.34%           
- Complexity     1884     1885    +1     
=========================================
  Files            71       71           
  Lines          4536     4539    +3     
=========================================
+ Hits           4461     4464    +3     
  Misses           75       75           
Impacted Files Coverage Δ
lib/Recur/RRuleIterator.php 99.44% <100.00%> (+<0.01%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 06feff3...1c66700. Read the comment docs.

codecov[bot] avatar Feb 14 '22 11:02 codecov[bot]

FREQ=YEARLY;BYMONTH=5;BYSETPOS=3;BYMONTHDAY=3

I think that means that the event happens yearly, on the 5th month and the 3rd day - for example 2022-05-03, just one event in a year. But then BYSETPOS=3 means, of all the events in the year, take only the 3rd event. But there is only ever one event in any year, so the BYSETPOS=3 filter means that no events ever match the criteria and we get an infinite loop that tries to find a "next" event in the future, but there is never an event.

It seems "a good thing" to "forcibly" stop the infinite loop at some point, to catch all these sorts of cases where the rule iterator will never get a match.

But it would also be nice to be able to analyse/parse rules "up front" and deduce that the rule can, mathematically, never generate any match, and so it is pointless wasting the effort of looping from now to year 9999.

Maybe there are a lot more rule cases that result in there never being a match for some yearly/monthly/weekly... rule? And so there might be infinite loops waiting in monthly/weekly etc processing?

phil-davis avatar Feb 14 '22 14:02 phil-davis

You are right! It will be more efficient to be able to detect rrule that never generate any occurrence. The fix was took from the monthly case here: https://github.com/sabre-io/vobject/issues/442

liurxliu avatar Feb 15 '22 05:02 liurxliu

Current CI passes in my test PR #564 so this works OK with the current code base and CI.

Let's take this fix - it fixes the problem in the same way as the existing code added in #443 - so we might as well take the same approach for now. I will merge.

phil-davis avatar Aug 17 '22 13:08 phil-davis