History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: QUARTZ-557
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Henri Yandell
Reporter: Mana Taghdiri
Votes: 1
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
Quartz Scheduler

Cron/Simple Trigger may return a firing time not included in the calender

Created: 26/Jan/07 12:43 PM   Updated: 14/Jul/08 08:49 AM
Component/s: Triggers
Affects Version/s: 1.6
Fix Version/s: 1.6.1

File Attachments: 1. Text File QUARTZ-557-2.patch (1 kb)
2. Text File QUARTZ-557.patch (0.8 kb)

Issue Links:
Related
This issue relates to:
QUARTZ-678 CronTrigger may return a firing time ... Major Open
 

Flags: Important


 Description  « Hide
The code for the "updateWithNewCalendar" method in "CronTrigger" and "SimpleTrigger" in the 1.06 release doesn't look right. It may set the "nextFireTime" field to something that is NOT included in the passed calendar. The code reads as follows: (I have no idea why anyone may use a loop that clearly iterates only once..) If the
misfire condition is true, the value assigned to the "nextFireTime" won't be checked by the calendar.

    {
        nextFireTime = getFireTimeAfter(previousFireTime);
        
        Date now = new Date();
        do {
            while (nextFireTime != null && calendar != null
                    && !calendar.isTimeIncluded(nextFireTime.getTime())) {
                nextFireTime = getFireTimeAfter(nextFireTime);
            }
            
            if(nextFireTime != null && nextFireTime.before(now)) {
                long diff = now.getTime() - nextFireTime.getTime();
                if(diff >= misfireThreshold) {
                    nextFireTime = getFireTimeAfter(nextFireTime);
                    continue;
                }
            }
        }while(false);
    }


 All   Comments   Change History      Sort Order:
Henri Yandell - [05/Feb/07 08:02 PM ]
Seems that the fix is to surround the getFireTimeAfter inside the diff >= misfireThreshold block with a copy of the while loop above.

That would be the 4th time this while loop appears in the file, so making it a private helper method seems tempting. Setting up a unit test for this seems non-trivial.

Henri Yandell - [05/Mar/07 03:51 PM ]
Patch that does the simplest part of the my suggestion. Untested though - I imagine visual inspection would be used as the test.

Iwan Memruk - [16/Mar/07 03:09 AM ]
Something like that happens for me as well.. (version 1.6.0)

java.lang.NullPointerException
at org.quartz.simpl.RAMJobStore.applyMisfire(RAMJobStore.java:1141)
at org.quartz.simpl.RAMJobStore.resumeTrigger(RAMJobStore.java:1000)
at org.quartz.simpl.RAMJobStore.resumeJobGroup(RAMJobStore.java:1077)
at org.quartz.core.QuartzScheduler.resumeJobGroup(QuartzScheduler.java:1149)
at org.quartz.impl.StdScheduler.resumeJobGroup(StdScheduler.java:461)
....

The Quartz code goes like..
        java.util.Date tnft = tw.trigger.getNextFireTime();
        if (tnft.getTime() > misfireTime) { return false; }
And tnft is suddenly null. It does not happen always though. I could not determine the exact circumstances when it becomes null.

The trigger is a SimpleTrigger. I've tried different misfire settings without any result.

Michael Day - [23/Mar/07 07:19 PM ]
I'm experiencing the same issue as the last commenter. In my particular case, I'm trying to resume a trigger that has no next fire time. applyMisfire() should check to ensure tnft is not null before trying to use it.

Henri Yandell - [03/May/07 12:35 PM ]
The latter two comments appear to be a different issue to this one.

Re-reading the first one, I suspect the aim was for the 'continue' to iterate the loop a second time, letting the calendar get set then. Unfortunately continue doesn't skip the while clause in a do-while so as the reporter points out, this is a single-iteration loop.


Henri Yandell - [03/May/07 12:36 PM ]
A second attempt at a patch - this time I've rewritten the method to be simpler while (I think) retaining the originally intended logic. Eyeballs welcome.

Henri Yandell - [03/May/07 12:41 PM ]
I've created a new issue based on the two NPE comments: QUARTZ-581

Henri Yandell - [06/Sep/07 02:16 PM ]
svn ci -m "Applying second patch from QUARTZ-557" src/java/org/quartz/SimpleTrigger.java
Sending src/java/org/quartz/SimpleTrigger.java
Transmitting file data .
Committed revision 720.

svn ci -m "Applying second patch from QUARTZ-557" src/java/org/quartz/SimpleTrigger.java
Sending src/java/org/quartz/SimpleTrigger.java
Transmitting file data .
Committed revision 721.

Wessel van Norel - [14/Jul/08 08:36 AM ]
The original remark said:

"CronTrigger" and "SimpleTrigger"

but it only seems to be fixed in the SimpleTrigger and not yet in the CronTrigger. Since the ticket is closed I can't attach a new patch.

Furthermore the documentation of the misfireThreshold parameter in Trigger.java is a bit unclear about what the parameter means. That's why I was looking in the code of both example triggers.

Wessel van Norel - [14/Jul/08 08:49 AM ]
Linking the two issue's to each other. Created QUARTZ-678 since this ticked (557) was closed a long time ago while "only" 1/2 of the ticket was actually solved