Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix infinite loop caused by yearly with bySetPos #564

Merged
merged 1 commit into from Aug 17, 2022

Conversation

liurxliu
Copy link

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

@codecov
Copy link

codecov bot commented Feb 14, 2022

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%) ⬆️

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.

@phil-davis
Copy link
Contributor

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?

@liurxliu
Copy link
Author

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: #442

@phil-davis
Copy link
Contributor

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 phil-davis merged commit a07ccff into sabre-io:master Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants