From 251659642d5752de01e2a51f758932a496f96505 Mon Sep 17 00:00:00 2001 From: David Costanzo Date: Thu, 1 Jun 2017 13:55:36 -0700 Subject: [PATCH 1/2] [#2060] Discard exceptions thrown by @Every jobs to that they continue to be re-scheduled --- framework/src/play/jobs/Job.java | 14 +- framework/src/play/jobs/JobsPlugin.java | 5 +- .../just-test-cases/test/JobTest.java | 139 ++++++++++++++++++ 3 files changed, 155 insertions(+), 3 deletions(-) create mode 100644 samples-and-tests/just-test-cases/test/JobTest.java diff --git a/framework/src/play/jobs/Job.java b/framework/src/play/jobs/Job.java index 901ce062b8..24a680262d 100644 --- a/framework/src/play/jobs/Job.java +++ b/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); } diff --git a/framework/src/play/jobs/JobsPlugin.java b/framework/src/play/jobs/JobsPlugin.java index 12d07c4ca1..8c2764964c 100644 --- a/framework/src/play/jobs/JobsPlugin.java +++ b/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); diff --git a/samples-and-tests/just-test-cases/test/JobTest.java b/samples-and-tests/just-test-cases/test/JobTest.java new file mode 100644 index 0000000000..a7e34ac4dd --- /dev/null +++ b/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()); + } +} From 58baeacf3022219e1e2672f9bb461268213ff2b8 Mon Sep 17 00:00:00 2001 From: David Costanzo Date: Mon, 12 Jun 2017 11:13:47 -0700 Subject: [PATCH 2/2] [#2060] Only throw exceptions for jobs scheduled with .in() or .now() --- framework/src/play/jobs/Job.java | 24 ++--- framework/src/play/jobs/JobsPlugin.java | 5 +- .../just-test-cases/test/JobTest.java | 99 +++++++++++++++++++ 3 files changed, 110 insertions(+), 18 deletions(-) diff --git a/framework/src/play/jobs/Job.java b/framework/src/play/jobs/Job.java index 24a680262d..fbc5e9d25c 100644 --- a/framework/src/play/jobs/Job.java +++ b/framework/src/play/jobs/Job.java @@ -36,7 +36,8 @@ public class Job extends Invoker.Invocation implements Callable { protected long lastRun = 0; protected boolean wasError = false; protected Throwable lastException = null; - + protected boolean propagateException = false; + Date nextPlannedExecution = null; @Override @@ -128,6 +129,9 @@ public Promise in(int seconds) { } private Callable getJobCallingCallable(final Promise smartFuture) { + // Propogate exceptions thrown by Job.call() to smartFuture. + propagateException = true; + return new Callable() { @Override public V call() throws Exception { @@ -164,19 +168,7 @@ public void every(String delay) { * time in seconds */ public void every(int 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.executor.scheduleWithFixedDelay(this, seconds, seconds, TimeUnit.SECONDS); JobsPlugin.scheduledJobs.add(this); } @@ -189,7 +181,9 @@ public void onException(Throwable e) { super.onException(e); } catch (Throwable ex) { Logger.error(ex, "Error during job execution (%s)", this); - throw new UnexpectedException(unwrap(e)); + if (propagateException) { + throw new UnexpectedException(unwrap(e)); + } } } diff --git a/framework/src/play/jobs/JobsPlugin.java b/framework/src/play/jobs/JobsPlugin.java index 8c2764964c..12d07c4ca1 100644 --- a/framework/src/play/jobs/JobsPlugin.java +++ b/framework/src/play/jobs/JobsPlugin.java @@ -157,15 +157,14 @@ public void afterApplicationStart() { // @Every if (clazz.isAnnotationPresent(Every.class)) { try { - Job job = (Job) clazz.newInstance(); - + Job job = createJob(clazz); 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)) { - job.every(value); + executor.scheduleWithFixedDelay(job, Time.parseDuration(value), Time.parseDuration(value), TimeUnit.SECONDS); } } catch (InstantiationException | IllegalAccessException ex) { throw new UnexpectedException("Cannot instantiate Job " + clazz.getName(), ex); diff --git a/samples-and-tests/just-test-cases/test/JobTest.java b/samples-and-tests/just-test-cases/test/JobTest.java index a7e34ac4dd..c526ec5c8d 100644 --- a/samples-and-tests/just-test-cases/test/JobTest.java +++ b/samples-and-tests/just-test-cases/test/JobTest.java @@ -1,9 +1,12 @@ +import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicLong; import org.junit.Test; import play.jobs.Every; import play.jobs.Job; +import play.jobs.On; +import play.libs.F.Promise; import play.test.UnitTest; /** @@ -52,6 +55,27 @@ public void doJob() throws Exception { } } + /** + * A Job class that is annotated with {@link On} to run every second. + */ + @On("* * * * * ? *") + public static class RunOnSecond 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. */ @@ -78,6 +102,63 @@ public void doJob() throws Exception { } } + /** + * Tests that if a Job's execution throws an exception when scheduled from {@link Job#now}, that the exception is + * thrown from the promise. + */ + @Test + public void testCanCollectExceptionFromJobNow() throws Exception { + + // Schedule a job that throws an exception to run now. + Job job = new Job() { + @Override + public void doJob() throws Exception { + throw new Exception("Intentional exception"); + } + }; + Promise promise = job.now(); + + try { + promise.get(); + fail("Calling Promise.get() for a job result that threw an exception did not throw the exception"); + } catch (ExecutionException ex) { + // The exception that gets thrown must be unwrapped to get to the original. + // Here, we unwrap twice (instead of once) because the Job infrastructure + // wraps it twice. This might be a bug, but since programs may depend on it, + // it's the behavior we test for. + assertEquals("Intentional exception", ex.getCause().getCause().getMessage()); + } + } + + /** + * Tests that if a Job's execution throws an exception when scheduled from {@link Job#in(int)}, that the exception + * is thrown from the promise. + */ + @Test + public void testCanCollectExceptionFromJobIn() throws Exception { + + // Schedule a job that throws an exception to run now. + Job job = new Job() { + @Override + public void doJob() throws Exception { + throw new Exception("Intentional exception"); + } + }; + // Schedule the job to run in 0 seconds (that is, now). + Promise promise = job.in(0); + + try { + promise.get(); + fail("Calling Promise.get() for a job result that threw an exception did not throw the exception"); + } catch (ExecutionException ex) { + // The exception that gets thrown must be unwrapped to get to the original. + // Here, we unwrap twice (instead of once) because the Job infrastructure + // wraps it twice. This might be a bug, but since programs may depend on it, + // it's the behavior we test for. + assertEquals("Intentional exception", ex.getCause().getCause().getMessage()); + } + } + /** * Tests that a job which is annotated to run every second does, indeed, run every second. */ @@ -136,4 +217,22 @@ public void testExceptionDoesNotHaltReschedulingEveryMethod() { assertFalse("TestJob job never ran", job.throwException); assertTrue("TestJob job was not run after throwing an exception", 2 <= job.totalRuns.get()); } + + /** + * Tests that throwing an exception does not halt the periodic scheduling of an {@code @On} annotation. + */ + @Test + public void testExceptionDoesNotHaltReschedulingWithOnAnnotation() { + + // Configure RunEverySecond to throw an exception. + RunOnSecond.throwException = true; + long beforeRuns = RunOnSecond.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 = RunOnSecond.totalRuns.get(); + assertFalse("RunOnSecond job never ran", RunEverySecond.throwException); + assertTrue("RunOnSecond job was not run after throwing an exception", 2 <= afterRuns - beforeRuns); + } }