Improve cron exception handling

Signed-off-by: Jimmy Tanagra <[email protected]>
openhab · Jan 11, 2025 · 3883c08 · 3883c08
1 parent 6dffaa6
commit 3883c08
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 11 deletions.
diff --git a/...n/java/org/openhab/core/automation/internal/module/handler/GenericCronTriggerHandler.java b/...n/java/org/openhab/core/automation/internal/module/handler/GenericCronTriggerHandler.java
@@ -68,9 +68,13 @@ public synchronized void setCallback(ModuleHandlerCallback callback) {
     }
 
     private void scheduleJob() {
-        schedule = scheduler.schedule(this, expression);
-        logger.debug("Scheduled cron job '{}' for trigger '{}'.", module.getConfiguration().get(CFG_CRON_EXPRESSION),
-                module.getId());
+        try {
+            schedule = scheduler.schedule(this, expression);
+            logger.debug("Scheduled cron job '{}' for trigger '{}'.",
+                    module.getConfiguration().get(CFG_CRON_EXPRESSION), module.getId());
+        } catch (IllegalArgumentException e) { // Catch exception from CronAdjuster
+            logger.warn("Failed to schedule job for trigger '{}'. {}", module.getId(), e.getMessage());
+        }
     }
 
     @Override

diff --git a/bundles/org.openhab.core/src/main/java/org/openhab/core/scheduler/CronAdjuster.java b/bundles/org.openhab.core/src/main/java/org/openhab/core/scheduler/CronAdjuster.java
@@ -12,6 +12,7 @@
  */
 package org.openhab.core.scheduler;
 
+import java.time.DateTimeException;
 import java.time.DayOfWeek;
 import java.time.temporal.ChronoField;
 import java.time.temporal.ChronoUnit;
@@ -71,7 +72,6 @@ interface Checker {
             .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
 
     private final List<Field> fields = new ArrayList<>(7);
-    private String cronExpression;
     private final Map<String, String> environmentMap;
     private final boolean reboot;
 
@@ -83,7 +83,6 @@ public CronAdjuster(final String specification) {
         environmentMap = parseEnvironment(entries);
 
         String cronExpression = entries[entries.length - 1].trim();
-        this.cronExpression = cronExpression;
 
         reboot = "@reboot".equals(cronExpression);
 
@@ -108,6 +107,14 @@ public CronAdjuster(final String specification) {
         parseAndAdd(cronExpression, parts[2], ChronoField.HOUR_OF_DAY);
         parseAndAdd(cronExpression, parts[1], ChronoField.MINUTE_OF_HOUR);
         parseAndAdd(cronExpression, parts[0], ChronoField.SECOND_OF_MINUTE);
+
+        try {
+            // Test the cron expression in action to make sure it won't cause too many restarts
+            adjustInto(java.time.ZonedDateTime.now());
+        } catch (final DateTimeException e) {
+            throw new IllegalArgumentException(
+                    String.format("Invalid cron expression '%s': %s", cronExpression, e.getMessage()));
+        }
     }
 
     /**
@@ -529,8 +536,7 @@ public Temporal adjustInto(@Nullable final Temporal temporal) {
                 index++;
             } else {
                 if (restarts++ > 1000) {
-                    throw new IllegalArgumentException(
-                            String.format("Cron expression '%s' is not valid, too many restarts", cronExpression));
+                    throw new DateTimeException("Conditions not satisfied.");
                 }
                 ret = out;
                 index = 0;

diff --git a/.../org.openhab.core/src/test/java/org/openhab/core/internal/scheduler/CronAdjusterTest.java b/.../org.openhab.core/src/test/java/org/openhab/core/internal/scheduler/CronAdjusterTest.java
@@ -203,9 +203,6 @@ public void testCronExpression(String in, String cron, String[] outs) {
     @ParameterizedTest
     @ValueSource(strings = { "0 0 0 31 2 *", "* * *", "80 * * * * *" })
     public void testInvalidCronExpression(String cron) {
-        assertThrows(IllegalArgumentException.class, () -> {
-            final CronAdjuster cronAdjuster = new CronAdjuster(cron);
-            cronAdjuster.adjustInto(java.time.ZonedDateTime.now());
-        });
+        assertThrows(IllegalArgumentException.class, () -> new CronAdjuster(cron));
     }
 }