nextExecution not working for '0/30 * * * * *' about cron-utils HOT 19 CLOSED

@strmer15 Thank you for reporting this! This is a bug. I am digging into the code and looking for a solution. Will keep you updated on changes!

from cron-utils.

So I've run into this issue as well and can reproduce it with a unix cron expression for minutes or hours:

0/15 * * * *
0 0/2 * * *

Both of those have the problem.

The problem appears to be in either ExecutionTime or TimeNode. The root cause is that we have a current time that is the last time in the list. So for the minutes case above, the list of minute values contains 0, 15, 30, and 45. As ExecutionTime.nextClosestMatch runs it finds that value in the list and that reference value is 45. Logic says it rounds up the input time to be the next whole minute. So something like 16:45:01 becomes 16:46:00. That happens in the if statement for seconds. Good so far.

It then recurses with the new date. So minute 46 is not found in the list of minute values. At this point, there is a call to minutes.getNextValue(date.getMinuteOfHour(), 0). This class/method is where the logic problem is evident (though I am not positive it's the only logic problem). So there in the TimeNode class we end up the getNearestForwardValue method. The input value of 46 is not in the list of values (0,15,30,45) so it tries to find a value in the list that is greater. Obviously not there.

The logic then is to increment shiftsToApply, with an intention of getting the next value past the index. The code actually does that, but its the wrong value. The method will return 15 as the next value past 46, when 0 is the proper value. That is due to the shiftsToApply having a value of 1, so the loop at the end skips past the 0 value (which is the current value) and instead picks the next value of 15.

Occam's razor says to just not increment the shiftsToApply value, which I tried, but that causes a couple of the test cases to fail during compile. So that's why I'm not sure the logic bug is in here. I think it is because the next value should be 0 from this method.

I'm going to continue to debug and look for a fix since I picked up this library for a scheduler and need it to handle these cases in particular. Reporting here in case it might be enough for someone more familiar with the library to know where it should be fixed.

I do suspect this same logic bug applies to some of the other open issues as well.

from cron-utils.

I think this is a two part fix. Part one is not incrementing shiftsToApply in the TimeNode method as I mentioned before. Part 2 is allowing for time wrap in the ExecutionTime.nextClosestMatch method. So here is my modified check for months:

        if(!months.getValues().contains(date.getMonthOfYear())) {
            log.debug(" == checking months ==");
            nearestValue = months.getNextValue(date.getMonthOfYear(), 0);
            int nextMonths = nearestValue.getValue();
            if(nearestValue.getShifts()>0){
                newDate =
                        new DateTime(date.getYear(), 1, 1, 0, 0, 0).plusYears(nearestValue.getShifts());
                return nextClosestMatch(newDate);
            }
            if (nearestValue.getValue() < date.getMonthOfYear()) {
                System.out.println("Adding 1 year. near = [" + nearestValue.getValue() + "]" );
                date = date.plusYears(1);
            }
            log.debug(" == months ==");
            return initDateTime(date.getYear(), nextMonths, lowestDay, lowestHour, lowestMinute, lowestSecond);
        }

I've left my debug in there but essentially that last if statement I added recognizes that time has wrapped to the next increment, in this case, next year, and then adds 1 year to it. So this handles the case where the current month is 12 and the next month to execute is 1 (which means next year). Similar cases for the other date components are needed.

However, this still leaves another bug. In the test class ExecutionTimeCron4jIntegrationTest and method testDayOfWeekOverridesAlwaysAtDayOfMonth with the EVERY_MONDAY_AT_18 expression, the calculated days of the month is wrong. With today being 2015-10-07, it only lists day 5 as a Monday. It misses days 12, 19, and 26 in the list. So everything is thrown off.

That's next on my list to track down.

from cron-utils.

@paulhethmon thank you for the insight and proposed partial fix! The diagnose is correct - I fully agree with it. I also agree this fix solves a couple of reported issues.
There is a bug in the logic about how the shifts and counts are applied. Apart from considering each field on its own; we may find helpful to add more detailed tests on TimeNode shifts behavior. These may give us some insight on cases we are not considering right now; and that propagate the error when calculating values for specific fields.

from cron-utils.

@jmrozanec The bug for days of week calculation is in the new method OnDayOfWeekValueGenerator.generateNoneValues(). The return statement with division using integers does not return successive values. I haven't delved into the issues for why you added this method back in August, but its definitely part of the problem. I think I can come up with a fix for this as well as the other things I've found. I will create a pull request once I have but I do have concerns that some of my fixes will impact other things I don't understand.

from cron-utils.

@paulhethmon thank you! I will be happy to review the pull request and check how may affect the rest of the codebase before merging it into master.
The changes introduced in August were aiming to replace a previous version of how to compute next/previous executions, which had more issues and was harder to understand.

from cron-utils.

@jmrozanec Once I figure out how to actually create a pull request, I will do so. I have some cases fixed, but I know of some that are not, and some edge cases my fixes don't handle.

from cron-utils.

@paulhethmon let me know if I can help you some way. If necessary, we can chat at gitter.im

from cron-utils.

@jmrozanec I think I've created the pull request for issues 37 and 38. It might also help out some of the others. One of the biggest changes was adding a new class BetweenDayOfWeekValueGenerator which handles the day of week range calculations. That class does not handle all cases though. Some of the abstract methods are not implemented as I was not exactly sure about what to do. What I have done does pass several new test cases that I had as well as what was in the issue.

from cron-utils.

@paulhethmon I will review it, and merge into trunk soon. Thank you!

from cron-utils.

Hi, this fix would be most welcome for me as well. We are getting nextExecution times in the past with a variety of cron definitions, most recently: */10 * * * *
Thanks!

from cron-utils.

@paulhethmon Thank you for providing the fix! I tested it against some other open issues, and seems to fix this and other reported bugs. Will merge into master and release soon a new version with this changes.

from cron-utils.

@danielsteenblik we considered the case you expose and verified is solved by the new fix we are introducing. Before weekend a new version of cron-utils will be released to Maven central. Thank you for exposing new cases!

from cron-utils.

@strmer15 the issue was fixed! Will release a new version of cron-utils before weekend.

from cron-utils.

Great, thanks!

On Wednesday, October 14, 2015, jmrozanec [email protected] wrote:

@danielsteenblik https://github.com/danielsteenblik we considered the
case you expose and verified is solved by the new fix we are introducing.
Before weekend a new version of cron-utils will be released to Maven
central. Thank you for exposing new cases!

—
Reply to this email directly or view it on GitHub
#37 (comment)
.

from cron-utils.

Hi @jmrozanec, were you able to make any progress toward a cron-utils
release with these updates? Thanks! -daniel

On Wed, Oct 14, 2015 at 9:28 PM, jmrozanec [email protected] wrote:

@strmer15 https://github.com/strmer15 the issue was fixed! Will release
a new version of cron-utils before weekend.

—
Reply to this email directly or view it on GitHub
#37 (comment)
.

from cron-utils.

@paulhethmon @strmer15 @danielsteenblik Version 3.1.1 was released including this fix.
@paulhethmon thank you for solving this issue!

from cron-utils.

Thank you!

On Mon, Oct 19, 2015 at 5:48 PM, jmrozanec [email protected] wrote:

@paulhethmon https://github.com/paulhethmon @strmer15
https://github.com/strmer15 @danielsteenblik
https://github.com/danielsteenblik Version 3.1.1
https://github.com/jmrozanec/cron-utils/releases/tag/3.1.1 was released
including this fix.
@paulhethmon https://github.com/paulhethmon thank you for solving this
issue!

—
Reply to this email directly or view it on GitHub
#37 (comment)
.

from cron-utils.

@danielsteenblik you are welcome! Join us on Gitter, to stay tuned about updates on future releases!

from cron-utils.