[#2060] drop exceptions thrown by @Every jobs so that they get rescheduled

framework/src/play/jobs/Job.java

@@ -164,7 +164,19 @@ public void every(String delay) {
      *            time in seconds
      */
     public void every(int seconds) {
-        JobsPlugin.executor.scheduleWithFixedDelay(this, seconds, seconds, TimeUnit.SECONDS);
+        // Wrap this job in a Runnable that catches and ignores all thrown exception.
+        // If there's an unhandled exception, then the executor would suppress all
+        // further executions, violating the contract.  Note that this does
+        // not interfere with the normal handling of onException().
+        Runnable neverThrowingRunnable = new Runnable() {
+            public void run() {
+                try {
+                    Job.this.run();
+                } catch (Exception exception) {
+                }
+            }
+        };
+        JobsPlugin.executor.scheduleWithFixedDelay(neverThrowingRunnable, seconds, seconds, TimeUnit.SECONDS);
         JobsPlugin.scheduledJobs.add(this);
     }
 

framework/src/play/jobs/JobsPlugin.java

@@ -157,14 +157,15 @@ public void afterApplicationStart() {
             // @Every
             if (clazz.isAnnotationPresent(Every.class)) {
                 try {
-                    Job job = createJob(clazz);
+                    Job<?> job = (Job<?>) clazz.newInstance();
+
                     String value = job.getClass().getAnnotation(Every.class).value();
                     if (value.startsWith("cron.")) {
                         value = Play.configuration.getProperty(value);
                     }
                     value = Expression.evaluate(value, value).toString();
                     if (!"never".equalsIgnoreCase(value)) {
-                        executor.scheduleWithFixedDelay(job, Time.parseDuration(value), Time.parseDuration(value), TimeUnit.SECONDS);
+                        job.every(value);
                     }
                 } catch (InstantiationException | IllegalAccessException ex) {
                     throw new UnexpectedException("Cannot instantiate Job " + clazz.getName(), ex);

samples-and-tests/just-test-cases/test/JobTest.java

@@ -0,0 +1,139 @@
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.junit.Test;
+
+import play.jobs.Every;
+import play.jobs.Job;
+import play.test.UnitTest;
+
+/**
+ * Unit tests for the {@link Job} class.
+ */
+public class JobTest extends UnitTest {
+
+    /**
+     * A Job class that is annotated with {@link Every} to run every second.
+     */
+    public static class TestJob extends Job {
+        private final AtomicLong totalRuns = new AtomicLong(0);
+        private volatile boolean throwException = false;
+
+        @Override
+        public void doJob() throws Exception {
+            totalRuns.getAndIncrement();
+
+            // To avoid logging an error every second, we only throw an
+            // exception when the relevant test case is running.
+            if (throwException) {
+                throwException = false;
+                throw new Exception("Throwing an exception");
+            }
+        }
+    }
+
+    /**
+     * A Job class that is annotated with {@link Every} to run every second.
+     */
+    @Every("1s")
+    public static class RunEverySecond extends Job {
+        private static final AtomicLong totalRuns = new AtomicLong(0);
+        private static volatile boolean throwException = false;
+
+        @Override
+        public void doJob() throws Exception {
+            totalRuns.getAndIncrement();
+
+            // To avoid logging an error every second, we only throw an
+            // exception when the relevant test case is running.
+            if (throwException) {
+                throwException = false;
+                throw new Exception("Throwing an exception");
+            }
+        }
+    }
+
+    /**
+     * A Job class that is annotated with {@link Every} to never run.
+     */
+    @Every("never")
+    public static class RunNever extends Job {
+        private static final AtomicLong totalRuns = new AtomicLong(0);
+
+        @Override
+        public void doJob() throws Exception {
+            totalRuns.getAndIncrement();
+        }
+    }
+
+    /**
+     * A Job class that is annotated with {@link Every} to never run (using mixed case)
+     */
+    @Every("NeveR")
+    public static class RunNeverMixedCase extends Job {
+        private static final AtomicLong totalRuns = new AtomicLong(0);
+
+        @Override
+        public void doJob() throws Exception {
+            totalRuns.getAndIncrement();
+        }
+    }
+
+    /**
+     * Tests that a job which is annotated to run every second does, indeed, run every second.
+     */
+    @Test
+    public void testEverySecond() {
+        long beforeRuns = RunEverySecond.totalRuns.get();
+        pause(1500); // wait long enough for the job to have run at least once
+        long afterRuns = RunEverySecond.totalRuns.get();
+        assertTrue("RunEverySecond job was never run", beforeRuns < afterRuns);
+        assertTrue("RunEverySecond job was run too many times", afterRuns - beforeRuns <= 2);
+    }
+
+    /**
+     * Tests that jobs which are annotated to never run have never been run.
+     */
+    @Test
+    public void testEveryNever() {
+        assertEquals(0, RunNever.totalRuns.get());
+        assertEquals(0, RunNeverMixedCase.totalRuns.get());
+    }
+
+    /**
+     * Tests that throwing an exception does not halt the periodic scheduling of an {@code @Every} annotation.
+     * This is a regression test for Lighthouse [#2060]
+     */
+    @Test
+    public void testExceptionDoesNotHaltReschedulingWithEveryAnnotation() {
+
+        // Configure RunEverySecond to throw an exception.
+        RunEverySecond.throwException = true;
+        long beforeRuns = RunEverySecond.totalRuns.get();
+
+        pause(2500); // wait long enough for the job to have run at least twice
+
+        // Make sure it threw an exception and ran multiple times.
+        long afterRuns = RunEverySecond.totalRuns.get();
+        assertFalse("RunEverySecond job never ran", RunEverySecond.throwException);
+        assertTrue("RunEverySecond job was not run after throwing an exception", 2 <= afterRuns - beforeRuns);
+    }
+
+    /**
+     * Tests that throwing an exception does not halt the periodic scheduling of the {@link Job#every(int)} method.
+     * This is a regression test for Lighthouse [#2060]
+     */
+    @Test
+    public void testExceptionDoesNotHaltReschedulingEveryMethod() {
+
+        // Schedule a job to run every second.
+        TestJob job = new TestJob();
+        job.throwException = true;
+        job.every("1s");
+
+        pause(2500); // wait long enough for the job to have run at least twice
+
+        // Make sure it threw an exception and ran multiple times.
+        assertFalse("TestJob job never ran", job.throwException);
+        assertTrue("TestJob job was not run after throwing an exception", 2 <= job.totalRuns.get());
+    }
+}