[#QUARTZ-445] CronCalendar.getNextIncludedTime() loops forever

Quartz Scheduler

CronCalendar.getNextIncludedTime() loops forever

Created: 16/May/06 04:46 AM Updated: 05/Jul/06 10:27 PM

Component/s:

None

Affects Version/s:

1.5.2

Fix Version/s:

1.6

Issue Links:

Related

This issue relates to:
	QUARTZ-481 Improve CronExpression.getNextInvalid...

Description

« Hide

As subject says. TestCase:

public void testCronCalendar() throws AchvSchedulerException, ParseException
    {
        CronCalendar cronCalendar = new CronCalendar( "name", "* * 0-7,18-23 ? * *" );
        Calendar javaCal = Calendar.getInstance();
        javaCal.set( Calendar.HOUR, 7 );
        javaCal.set( Calendar.MINUTE, 0 );
        javaCal.set( Calendar.SECOND, 0 );
        javaCal.set( Calendar.MILLISECOND, 0 );

assertFalse( cronCalendar.isTimeIncluded( javaCal.getTimeInMillis() ) );
        assertEquals( javaCal.getTimeInMillis() + 1, cronCalendar.getNextIncludedTime( javaCal.getTimeInMillis() ) );
    }

All

Comments

Change History

Sort Order:

[ Permlink | « Hide ]

Christoph Kutzinski - [16/May/06 05:24 AM ]

I think the bug is a missing inversion of the if-clause in line 107 of CronCalendar.
It should read IMO:

while (!isTimeIncluded(nextIncludedTime)) {
...
         if ( !cronExpression.isSatisfiedBy(new Date(nextIncludedTime))) { <=== NOT added
         nextIncludedTime =
         cronExpression.getNextValidTimeAfter(
         new Date(nextIncludedTime)).getTime();
         }
...
}

However I couldn't test it yet, i.e. have no time to figure out how to build Quartz myself.

[ Permlink | « Hide ]

Christoph Kutzinski - [17/May/06 08:33 AM ]

I was wrong with my last comment (was confused by the fact that the calendar should 'include' all time spaces which are 'excluded' by the cron expression)

So a good fix would be to change it to
while (!isTimeIncluded(nextIncludedTime)) {
...
         if ( cronExpression.isSatisfiedBy(new Date(nextIncludedTime))) { <=== NOT added
           nextIncludedTime = cronExpression.getNextInvalidTimeAfter( new Date(nextIncludedTime)).getTime(); <== Note the 'invalid'
         }
...
}

Unfortunately there is no such method in CronExpression. I made a hack in my own subclass of cron calendar which looks like this:
if ( getCronExpression().isSatisfiedBy( new Date( nextIncludedTime ) ) )
{
     do
      {
            nextIncludedTime += 1000;
       }
       while( getCronExpression().isSatisfiedBy( new Date( nextIncludedTime ) ) );
}

This returns correct results (at least for my test case), but is probably very inefficient if the next included time is far in the future.

[ Permlink | « Hide ]

Aaron Craven - [05/Jul/06 10:26 PM ]

Problem is corrected, but with a poorly performing solution. See related JIRA entry for more details.