Skip to content

Commit dea5918

Browse files
jhoellerjhoeller
committed
CronTrigger defensively protects itself against accidental re-fires if a task runs too early (SPR-7004)
1 parent 2136b04 commit dea5918

File tree

3 files changed

+59
-37
lines changed

3 files changed

+59
-37
lines changed

‎org.springframework.context/src/main/java/org/springframework/scheduling/support/CronSequenceGenerator.java‎

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,6 @@ public CronSequenceGenerator(String expression, TimeZone timeZone) {
8888
* @return the next value matching the pattern
8989
*/
9090
public Date next(Date date) {
91-
9291
/*
9392
The plan:
9493
@@ -106,11 +105,10 @@ public Date next(Date date) {
106105
4.2 Reset the minutes and seconds and go to 2
107106
108107
...
109-
110-
*/
108+
*/
111109

112110
Calendar calendar = new GregorianCalendar();
113-
calendar.setTimeZone(timeZone);
111+
calendar.setTimeZone(this.timeZone);
114112
calendar.setTime(date);
115113

116114
// Truncate to the next whole second
@@ -136,15 +134,17 @@ private void doNext(Calendar calendar) {
136134
int updateMinute = findNext(this.minutes, minute, calendar, Calendar.MINUTE, Calendar.HOUR_OF_DAY, resets);
137135
if (minute == updateMinute) {
138136
resets.add(Calendar.MINUTE);
139-
} else {
137+
}
138+
else {
140139
doNext(calendar);
141140
}
142141

143142
int hour = calendar.get(Calendar.HOUR_OF_DAY);
144143
int updateHour = findNext(this.hours, hour, calendar, Calendar.HOUR_OF_DAY, Calendar.DAY_OF_WEEK, resets);
145144
if (hour == updateHour) {
146145
resets.add(Calendar.HOUR_OF_DAY);
147-
} else {
146+
}
147+
else {
148148
doNext(calendar);
149149
}
150150

@@ -153,7 +153,8 @@ private void doNext(Calendar calendar) {
153153
int updateDayOfMonth = findNextDay(calendar, this.daysOfMonth, dayOfMonth, daysOfWeek, dayOfWeek, 366, resets);
154154
if (dayOfMonth == updateDayOfMonth) {
155155
resets.add(Calendar.DAY_OF_MONTH);
156-
} else {
156+
}
157+
else {
157158
doNext(calendar);
158159
}
159160

@@ -278,7 +279,8 @@ private void setNumberHits(BitSet bits, String value, int max) {
278279
// Not an incrementer so it must be a range (possibly empty)
279280
int[] range = getRange(field, max);
280281
bits.set(range[0], range[1] + 1);
281-
} else {
282+
}
283+
else {
282284
String[] split = StringUtils.delimitedListToStringArray(field, "/");
283285
if (split.length > 2) {
284286
throw new IllegalArgumentException("Incrementer has more than two fields: " + field);
@@ -304,7 +306,8 @@ private int[] getRange(String field, int max) {
304306
}
305307
if (!field.contains("-")) {
306308
result[0] = result[1] = Integer.valueOf(field);
307-
} else {
309+
}
310+
else {
308311
String[] split = StringUtils.delimitedListToStringArray(field, "-");
309312
if (split.length > 2) {
310313
throw new IllegalArgumentException("Range has more than two fields: " + field);
@@ -324,19 +327,21 @@ public boolean equals(Object obj) {
324327
return false;
325328
}
326329
CronSequenceGenerator cron = (CronSequenceGenerator) obj;
327-
return cron.months.equals(months) && cron.daysOfMonth.equals(daysOfMonth) && cron.daysOfWeek.equals(daysOfWeek)
328-
&& cron.hours.equals(hours) && cron.minutes.equals(minutes) && cron.seconds.equals(seconds);
330+
return cron.months.equals(this.months) && cron.daysOfMonth.equals(this.daysOfMonth) &&
331+
cron.daysOfWeek.equals(this.daysOfWeek) && cron.hours.equals(this.hours) &&
332+
cron.minutes.equals(this.minutes) && cron.seconds.equals(this.seconds);
329333
}
330334

331335
@Override
332336
public int hashCode() {
333-
return 37 + 17 * months.hashCode() + 29 * daysOfMonth.hashCode() + 37 * daysOfWeek.hashCode() + 41
334-
* hours.hashCode() + 53 * minutes.hashCode() + 61 * seconds.hashCode();
337+
return 37 + 17 * this.months.hashCode() + 29 * this.daysOfMonth.hashCode() +
338+
37 * this.daysOfWeek.hashCode() + 41 * this.hours.hashCode() +
339+
53 * this.minutes.hashCode() + 61 * this.seconds.hashCode();
335340
}
336341

337342
@Override
338343
public String toString() {
339-
return getClass().getSimpleName() + ": " + expression;
344+
return getClass().getSimpleName() + ": " + this.expression;
340345
}
341346

342347
}

‎org.springframework.context/src/main/java/org/springframework/scheduling/support/CronTrigger.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,16 @@ public CronTrigger(String cronExpression, TimeZone timeZone) {
5757

5858
public Date nextExecutionTime(TriggerContext triggerContext) {
5959
Date date = triggerContext.lastCompletionTime();
60-
if (date == null) {
60+
if (date != null) {
61+
Date scheduled = triggerContext.lastScheduledExecutionTime();
62+
if (scheduled != null && date.before(scheduled)) {
63+
// Previous task apparently executed too early...
64+
// Let's simply use the last calculated execution time then,
65+
// in order to prevent accidental re-fires in the same second.
66+
date = scheduled;
67+
}
68+
}
69+
else {
6170
date = new Date();
6271
}
6372
return this.sequenceGenerator.next(date);

‎org.springframework.context/src/test/java/org/springframework/scheduling/support/CronTriggerTests.java‎

Lines changed: 30 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2009 the original author or authors.
2+
* Copyright 2002-2010 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -16,25 +16,26 @@
1616

1717
package org.springframework.scheduling.support;
1818

19-
import static org.junit.Assert.assertEquals;
20-
2119
import java.util.ArrayList;
2220
import java.util.Calendar;
2321
import java.util.Date;
2422
import java.util.GregorianCalendar;
2523
import java.util.List;
2624
import java.util.TimeZone;
2725

26+
import static org.junit.Assert.*;
2827
import org.junit.Before;
2928
import org.junit.Test;
3029
import org.junit.runner.RunWith;
3130
import org.junit.runners.Parameterized;
3231
import org.junit.runners.Parameterized.Parameters;
32+
3333
import org.springframework.scheduling.TriggerContext;
3434

3535
/**
3636
* @author Dave Syer
3737
* @author Mark Fisher
38+
* @author Juergen Hoeller
3839
*/
3940
@RunWith(Parameterized.class)
4041
public class CronTriggerTests {
@@ -45,6 +46,7 @@ public class CronTriggerTests {
4546

4647
private final TimeZone timeZone;
4748

49+
4850
public CronTriggerTests(Date date, TimeZone timeZone) {
4951
this.timeZone = timeZone;
5052
this.date = date;
@@ -58,9 +60,6 @@ public static List<Object[]> getParameters() {
5860
return list;
5961
}
6062

61-
/**
62-
* @param calendar
63-
*/
6463
private void roundup(Calendar calendar) {
6564
calendar.add(Calendar.SECOND, 1);
6665
calendar.set(Calendar.MILLISECOND, 0);
@@ -106,6 +105,17 @@ public void testIncrementSecondByOne() throws Exception {
106105
assertEquals(calendar.getTime(), trigger.nextExecutionTime(context));
107106
}
108107

108+
@Test
109+
public void testIncrementSecondWithPreviousExecutionTooEarly() throws Exception {
110+
CronTrigger trigger = new CronTrigger("11 * * * * *", timeZone);
111+
calendar.set(Calendar.SECOND, 11);
112+
SimpleTriggerContext context = new SimpleTriggerContext();
113+
context.update(calendar.getTime(), new Date(calendar.getTimeInMillis() - 100),
114+
new Date(calendar.getTimeInMillis() - 90));
115+
calendar.add(Calendar.MINUTE, 1);
116+
assertEquals(calendar.getTime(), trigger.nextExecutionTime(context));
117+
}
118+
109119
@Test
110120
public void testIncrementSecondAndRollover() throws Exception {
111121
CronTrigger trigger = new CronTrigger("10 * * * * *", timeZone);
@@ -126,28 +136,29 @@ public void testSecondRange() throws Exception {
126136
}
127137

128138
@Test
129-
public void testIncrementMinuteByOne() throws Exception {
130-
CronTrigger trigger = new CronTrigger("0 11 * * * *", timeZone);
139+
public void testIncrementMinute() throws Exception {
140+
CronTrigger trigger = new CronTrigger("0 * * * * *", timeZone);
131141
calendar.set(Calendar.MINUTE, 10);
132142
Date date = calendar.getTime();
133143
calendar.add(Calendar.MINUTE, 1);
134144
calendar.set(Calendar.SECOND, 0);
135-
TriggerContext context = getTriggerContext(date);
136-
assertEquals(calendar.getTime(), trigger.nextExecutionTime(context));
145+
TriggerContext context1 = getTriggerContext(date);
146+
date = trigger.nextExecutionTime(context1);
147+
assertEquals(calendar.getTime(), date);
148+
calendar.add(Calendar.MINUTE, 1);
149+
TriggerContext context2 = getTriggerContext(date);
150+
date = trigger.nextExecutionTime(context2);
151+
assertEquals(calendar.getTime(), date);
137152
}
138153

139154
@Test
140-
public void testIncrementMinute() throws Exception {
141-
CronTrigger trigger = new CronTrigger("0 * * * * *", timeZone);
155+
public void testIncrementMinuteByOne() throws Exception {
156+
CronTrigger trigger = new CronTrigger("0 11 * * * *", timeZone);
142157
calendar.set(Calendar.MINUTE, 10);
143-
Date date = calendar.getTime();
158+
TriggerContext context = getTriggerContext(calendar.getTime());
144159
calendar.add(Calendar.MINUTE, 1);
145160
calendar.set(Calendar.SECOND, 0);
146-
TriggerContext context1 = getTriggerContext(date);
147-
assertEquals(calendar.getTime(), date = trigger.nextExecutionTime(context1));
148-
calendar.add(Calendar.MINUTE, 1);
149-
TriggerContext context2 = getTriggerContext(date);
150-
assertEquals(calendar.getTime(), date = trigger.nextExecutionTime(context2));
161+
assertEquals(calendar.getTime(), trigger.nextExecutionTime(context));
151162
}
152163

153164
@Test
@@ -612,10 +623,7 @@ public void testWhitespace() throws Exception {
612623
assertEquals(trigger1, trigger2);
613624
}
614625

615-
/**
616-
* @param trigger
617-
* @param calendar
618-
*/
626+
619627
private void assertMatchesNextSecond(CronTrigger trigger, Calendar calendar) {
620628
Date date = calendar.getTime();
621629
roundup(calendar);

0 commit comments

Comments
 (0)