CronTrigger defensively protects itself against accidental re-fires if a task runs too early (SPR-7004)

git-svn-id: https://src.springframework.org/svn/spring-framework/trunk@3373 50f2f4bb-b051-0410-bef5-90022cba6387

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

@@ -88,7 +88,6 @@ public class CronSequenceGenerator {
 	 * @return the next value matching the pattern
 	 */
 	public Date next(Date date) {
-
 		/*
 		The plan:
 
@@ -106,11 +105,10 @@ public class CronSequenceGenerator {
 		4.2 Reset the minutes and seconds and go to 2
 		
 		...
-
-		 */
+		*/
 
 		Calendar calendar = new GregorianCalendar();
-		calendar.setTimeZone(timeZone);
+		calendar.setTimeZone(this.timeZone);
 		calendar.setTime(date);
 
 		// Truncate to the next whole second
@@ -136,7 +134,8 @@ public class CronSequenceGenerator {
 		int updateMinute = findNext(this.minutes, minute, calendar, Calendar.MINUTE, Calendar.HOUR_OF_DAY, resets);
 		if (minute == updateMinute) {
 			resets.add(Calendar.MINUTE);
-		} else {
+		}
+		else {
 			doNext(calendar);
 		}
 
@@ -144,7 +143,8 @@ public class CronSequenceGenerator {
 		int updateHour = findNext(this.hours, hour, calendar, Calendar.HOUR_OF_DAY, Calendar.DAY_OF_WEEK, resets);
 		if (hour == updateHour) {
 			resets.add(Calendar.HOUR_OF_DAY);
-		} else {
+		}
+		else {
 			doNext(calendar);
 		}
 
@@ -153,7 +153,8 @@ public class CronSequenceGenerator {
 		int updateDayOfMonth = findNextDay(calendar, this.daysOfMonth, dayOfMonth, daysOfWeek, dayOfWeek, 366, resets);
 		if (dayOfMonth == updateDayOfMonth) {
 			resets.add(Calendar.DAY_OF_MONTH);
-		} else {
+		}
+		else {
 			doNext(calendar);
 		}
 
@@ -278,7 +279,8 @@ public class CronSequenceGenerator {
 				// Not an incrementer so it must be a range (possibly empty)
 				int[] range = getRange(field, max);
 				bits.set(range[0], range[1] + 1);
-			} else {
+			}
+			else {
 				String[] split = StringUtils.delimitedListToStringArray(field, "/");
 				if (split.length > 2) {
 					throw new IllegalArgumentException("Incrementer has more than two fields: " + field);
@@ -304,7 +306,8 @@ public class CronSequenceGenerator {
 		}
 		if (!field.contains("-")) {
 			result[0] = result[1] = Integer.valueOf(field);
-		} else {
+		}
+		else {
 			String[] split = StringUtils.delimitedListToStringArray(field, "-");
 			if (split.length > 2) {
 				throw new IllegalArgumentException("Range has more than two fields: " + field);
@@ -324,19 +327,21 @@ public class CronSequenceGenerator {
 			return false;
 		}
 		CronSequenceGenerator cron = (CronSequenceGenerator) obj;
-		return cron.months.equals(months) && cron.daysOfMonth.equals(daysOfMonth) && cron.daysOfWeek.equals(daysOfWeek)
-				&& cron.hours.equals(hours) && cron.minutes.equals(minutes) && cron.seconds.equals(seconds);
+		return cron.months.equals(this.months) && cron.daysOfMonth.equals(this.daysOfMonth) &&
+				cron.daysOfWeek.equals(this.daysOfWeek) && cron.hours.equals(this.hours) &&
+				cron.minutes.equals(this.minutes) && cron.seconds.equals(this.seconds);
 	}
 
 	@Override
 	public int hashCode() {
-		return 37 + 17 * months.hashCode() + 29 * daysOfMonth.hashCode() + 37 * daysOfWeek.hashCode() + 41
-				* hours.hashCode() + 53 * minutes.hashCode() + 61 * seconds.hashCode();
+		return 37 + 17 * this.months.hashCode() + 29 * this.daysOfMonth.hashCode() +
+				37 * this.daysOfWeek.hashCode() + 41 * this.hours.hashCode() +
+				53 * this.minutes.hashCode() + 61 * this.seconds.hashCode();
 	}
 
 	@Override
 	public String toString() {
-		return getClass().getSimpleName() + ": " + expression;
+		return getClass().getSimpleName() + ": " + this.expression;
 	}
 
 }

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

@@ -57,7 +57,16 @@ public class CronTrigger implements Trigger {
 
 	public Date nextExecutionTime(TriggerContext triggerContext) {
 		Date date = triggerContext.lastCompletionTime();
-		if (date == null) {
+		if (date != null) {
+			Date scheduled = triggerContext.lastScheduledExecutionTime();
+			if (scheduled != null && date.before(scheduled)) {
+				// Previous task apparently executed too early...
+				// Let's simply use the last calculated execution time then,
+				// in order to prevent accidental re-fires in the same second.
+				date = scheduled;
+			}
+		}
+		else {
 			date = new Date();
 		}
 		return this.sequenceGenerator.next(date);

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2009 the original author or authors.
+ * Copyright 2002-2010 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,8 +16,6 @@
 
 package org.springframework.scheduling.support;
 
-import static org.junit.Assert.assertEquals;
-
 import java.util.ArrayList;
 import java.util.Calendar;
 import java.util.Date;
@@ -25,16 +23,19 @@ import java.util.GregorianCalendar;
 import java.util.List;
 import java.util.TimeZone;
 
+import static org.junit.Assert.*;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 import org.junit.runners.Parameterized.Parameters;
+
 import org.springframework.scheduling.TriggerContext;
 
 /**
  * @author Dave Syer
  * @author Mark Fisher
+ * @author Juergen Hoeller
  */
 @RunWith(Parameterized.class)
 public class CronTriggerTests {
@@ -45,6 +46,7 @@ public class CronTriggerTests {
 
 	private final TimeZone timeZone;
 
+
 	public CronTriggerTests(Date date, TimeZone timeZone) {
 		this.timeZone = timeZone;
 		this.date = date;
@@ -58,9 +60,6 @@ public class CronTriggerTests {
 		return list;
 	}
 
-	/**
-	 * @param calendar
-	 */
 	private void roundup(Calendar calendar) {
 		calendar.add(Calendar.SECOND, 1);
 		calendar.set(Calendar.MILLISECOND, 0);
@@ -106,6 +105,17 @@ public class CronTriggerTests {
 		assertEquals(calendar.getTime(), trigger.nextExecutionTime(context));
 	}
 
+	@Test
+	public void testIncrementSecondWithPreviousExecutionTooEarly() throws Exception {
+		CronTrigger trigger = new CronTrigger("11 * * * * *", timeZone);
+		calendar.set(Calendar.SECOND, 11);
+		SimpleTriggerContext context = new SimpleTriggerContext();
+		context.update(calendar.getTime(), new Date(calendar.getTimeInMillis() - 100),
+				new Date(calendar.getTimeInMillis() - 90));
+		calendar.add(Calendar.MINUTE, 1);
+		assertEquals(calendar.getTime(), trigger.nextExecutionTime(context));
+	}
+
 	@Test
 	public void testIncrementSecondAndRollover() throws Exception {
 		CronTrigger trigger = new CronTrigger("10 * * * * *", timeZone);
@@ -126,28 +136,29 @@ public class CronTriggerTests {
 	}
 
 	@Test
-	public void testIncrementMinuteByOne() throws Exception {
-		CronTrigger trigger = new CronTrigger("0 11 * * * *", timeZone);
+	public void testIncrementMinute() throws Exception {
+		CronTrigger trigger = new CronTrigger("0 * * * * *", timeZone);
 		calendar.set(Calendar.MINUTE, 10);
 		Date date = calendar.getTime();
 		calendar.add(Calendar.MINUTE, 1);
 		calendar.set(Calendar.SECOND, 0);
-		TriggerContext context = getTriggerContext(date);
-		assertEquals(calendar.getTime(), trigger.nextExecutionTime(context));
+		TriggerContext context1 = getTriggerContext(date);
+		date = trigger.nextExecutionTime(context1);
+		assertEquals(calendar.getTime(), date);
+		calendar.add(Calendar.MINUTE, 1);
+		TriggerContext context2 = getTriggerContext(date);
+		date = trigger.nextExecutionTime(context2);
+		assertEquals(calendar.getTime(), date);
 	}
 
 	@Test
-	public void testIncrementMinute() throws Exception {
-		CronTrigger trigger = new CronTrigger("0 * * * * *", timeZone);
+	public void testIncrementMinuteByOne() throws Exception {
+		CronTrigger trigger = new CronTrigger("0 11 * * * *", timeZone);
 		calendar.set(Calendar.MINUTE, 10);
-		Date date = calendar.getTime();
+		TriggerContext context = getTriggerContext(calendar.getTime());
 		calendar.add(Calendar.MINUTE, 1);
 		calendar.set(Calendar.SECOND, 0);
-		TriggerContext context1 = getTriggerContext(date);
-		assertEquals(calendar.getTime(), date = trigger.nextExecutionTime(context1));
-		calendar.add(Calendar.MINUTE, 1);
-		TriggerContext context2 = getTriggerContext(date);
-		assertEquals(calendar.getTime(), date = trigger.nextExecutionTime(context2));
+		assertEquals(calendar.getTime(), trigger.nextExecutionTime(context));
 	}
 
 	@Test
@@ -612,10 +623,7 @@ public class CronTriggerTests {
 		assertEquals(trigger1, trigger2);
 	}
 
-	/**
-	 * @param trigger
-	 * @param calendar
-	 */
+
 	private void assertMatchesNextSecond(CronTrigger trigger, Calendar calendar) {
 		Date date = calendar.getTime();
 		roundup(calendar);