From 52e83f107c353cc6931d4896d5429b00cdca28b8 Mon Sep 17 00:00:00 2001 From: Manyanda Chitimbo Date: Thu, 5 May 2022 23:59:15 +0200 Subject: [PATCH] fix(quartz): make sure that disabled jobs are paused Fixes https://github.com/quarkusio/quarkus/issues/25214 Disabled methods whose jobs details and triggers were already in the database were still getting triggered because Quartz invokes all the jobs in the database that are not in the paused State. This is to make sure that the disabled methods are effectively not triggered by Quartz in any way. Furthermore, I do not believe the full scheduled method implementing the Application custom logic was called because of the checks in the extensions which meant the App business logic trigger was not full invoked but pausing the triggers via Quartz we are sure that the trigger won't be invoked when it is disabled. Previously this was done via checks in the extension which mean that only the Application logic of the scheduled method was not called. - https://github.com/quarkusio/quarkus/blob/4df39b60a0b3d132713e6d60087a1054588d4a5f/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java#L215-L218 - https://github.com/quarkusio/quarkus/blob/4df39b60a0b3d132713e6d60087a1054588d4a5f/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java#L168-L170 which meant that the trigger was not stored in known business triggers at https://github.com/quarkusio/quarkus/blob/4df39b60a0b3d132713e6d60087a1054588d4a5f/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java#L302-L303 meaning that upon the execution invocation of the method in https://github.com/quarkusio/quarkus/blob/4df39b60a0b3d132713e6d60087a1054588d4a5f/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java#L560 the business logic wasn't triggered but a NPE (same as https://github.com/quarkusio/quarkus/issues/24931) could have been raised with the code in main branch --- .../quartz/runtime/QuartzScheduler.java | 2 ++ .../it/quartz/DisabledMethodResource.java | 28 +++++++++++++++++++ .../it/quartz/DisabledScheduledMethods.java | 24 ++++++++++++++++ .../src/main/resources/application.properties | 3 ++ .../io/quarkus/it/quartz/QuartzTestCase.java | 18 ++++++++++++ 5 files changed, 75 insertions(+) create mode 100644 integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledMethodResource.java create mode 100644 integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledScheduledMethods.java diff --git a/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java b/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java index 22e65c5c55b3f..8b31854cd1ab5 100644 --- a/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java +++ b/extensions/quartz/runtime/src/main/java/io/quarkus/quartz/runtime/QuartzScheduler.java @@ -166,6 +166,7 @@ public QuartzScheduler(SchedulerContext context, QuartzSupport quartzSupport, Sc String cron = SchedulerUtils.lookUpPropertyValue(scheduled.cron()); if (!cron.isEmpty()) { if (SchedulerUtils.isOff(cron)) { + this.pause(identity); continue; } if (!CronType.QUARTZ.equals(cronType)) { @@ -214,6 +215,7 @@ public QuartzScheduler(SchedulerContext context, QuartzSupport quartzSupport, Sc } else if (!scheduled.every().isEmpty()) { OptionalLong everyMillis = SchedulerUtils.parseEveryAsMillis(scheduled); if (!everyMillis.isPresent()) { + this.pause(identity); continue; } SimpleScheduleBuilder simpleScheduleBuilder = SimpleScheduleBuilder.simpleSchedule() diff --git a/integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledMethodResource.java b/integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledMethodResource.java new file mode 100644 index 0000000000000..e962c1723da5f --- /dev/null +++ b/integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledMethodResource.java @@ -0,0 +1,28 @@ +package io.quarkus.it.quartz; + +import javax.inject.Inject; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.Produces; +import javax.ws.rs.core.MediaType; + +@Path("/scheduler/disabled") +public class DisabledMethodResource { + + @Inject + DisabledScheduledMethods disabledScheduledMethods; + + @GET + @Path("cron") + @Produces(MediaType.TEXT_PLAIN) + public String getCronCount() { + return disabledScheduledMethods.valueSetByCronScheduledMethod; + } + + @GET + @Path("every") + @Produces(MediaType.TEXT_PLAIN) + public String getEveryCount() { + return disabledScheduledMethods.valueSetByEveryScheduledMethod; + } +} diff --git a/integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledScheduledMethods.java b/integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledScheduledMethods.java new file mode 100644 index 0000000000000..4b1b5d938925a --- /dev/null +++ b/integration-tests/quartz/src/main/java/io/quarkus/it/quartz/DisabledScheduledMethods.java @@ -0,0 +1,24 @@ +package io.quarkus.it.quartz; + +import javax.enterprise.context.ApplicationScoped; + +import io.quarkus.scheduler.Scheduled; + +@ApplicationScoped +public class DisabledScheduledMethods { + volatile static String valueSetByCronScheduledMethod = ""; + volatile static String valueSetByEveryScheduledMethod = ""; + + // This should never be called as the job is disabled + @Scheduled(cron = "${disabled}", identity = "disabled-cron-counter") + void setValueByCron() { + valueSetByCronScheduledMethod = "cron"; + } + + // This should never be called as the job is turned off + @Scheduled(every = "${off}", identity = "disabled-every-counter") + void setValueByEvery() { + valueSetByEveryScheduledMethod = "every"; + } + +} diff --git a/integration-tests/quartz/src/main/resources/application.properties b/integration-tests/quartz/src/main/resources/application.properties index c99453a9c63bc..84cc04b240877 100644 --- a/integration-tests/quartz/src/main/resources/application.properties +++ b/integration-tests/quartz/src/main/resources/application.properties @@ -18,3 +18,6 @@ quarkus.flyway.baseline-description=Quartz # Register de LoggingJobHistoryPlugin for testing quarkus.quartz.plugins.jobHistory.class=org.quartz.plugins.history.LoggingJobHistoryPlugin quarkus.quartz.plugins.jobHistory.properties.jobSuccessMessage=Job [{1}.{0}] execution complete and reports: {8} + +disabled=disabled +off=off diff --git a/integration-tests/quartz/src/test/java/io/quarkus/it/quartz/QuartzTestCase.java b/integration-tests/quartz/src/test/java/io/quarkus/it/quartz/QuartzTestCase.java index b962bfc8b14ba..e5fd18363e7cf 100644 --- a/integration-tests/quartz/src/test/java/io/quarkus/it/quartz/QuartzTestCase.java +++ b/integration-tests/quartz/src/test/java/io/quarkus/it/quartz/QuartzTestCase.java @@ -1,6 +1,7 @@ package io.quarkus.it.quartz; import static io.restassured.RestAssured.given; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; @@ -19,6 +20,14 @@ public void testCount() throws InterruptedException { assertCounter("/scheduler/count/fix-8555"); } + @Test + public void testDisabledMethodsShouldNeverBeExecuted() throws InterruptedException { + // Wait at least 1 second + Thread.sleep(1000); + assertEmptyValueForDisabledMethod("/scheduler/disabled/cron"); + assertEmptyValueForDisabledMethod("/scheduler/disabled/every"); + } + private void assertCounter(String counterPath) { Response response = given().when().get(counterPath); String body = response.asString(); @@ -29,4 +38,13 @@ private void assertCounter(String counterPath) { .statusCode(200); } + private void assertEmptyValueForDisabledMethod(String path) { + Response response = given().when().get(path); + String body = response.asString(); + assertEquals("", body); + response + .then() + .statusCode(200); + } + }