[astro] Refactor event scheduling (#20078)

* Refactor event scheduling to handle if the event already exists

Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>

Author: Nadahar

bundles/org.openhab.binding.astro/src/main/java/org/openhab/binding/astro/internal/AstroBindingConstants.java

@@ -163,4 +163,11 @@ private AstroBindingConstants() {
     public static final String EVENT_CHANNEL_ID_CIVIL_DUSK = "civilDusk#event";
     public static final String EVENT_CHANNEL_ID_EVENING_NIGHT = "eveningNight#event";
     public static final String EVENT_CHANNEL_ID_DAYLIGHT = "daylight#event";
+
+    // job identifiers
+
+    public static final String DAILY_JOB = "daily";
+    public static final String POSITIONAL_JOB = "positional";
+    public static final String PUBLISH_ZODIAC_JOB = "publishZodiac";
+    public static final String PUBLISH_SEASON_JOB = "publishSeason";
 }

bundles/org.openhab.binding.astro/src/main/java/org/openhab/binding/astro/internal/handler/AstroThingHandler.java

@@ -28,11 +28,11 @@
 import java.util.Calendar;
 import java.util.Collection;
 import java.util.GregorianCalendar;
-import java.util.HashSet;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
-import java.util.Set;
+import java.util.Map;
 import java.util.TimeZone;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
@@ -41,6 +41,7 @@
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
+import org.openhab.binding.astro.internal.AstroBindingConstants;
 import org.openhab.binding.astro.internal.action.AstroActions;
 import org.openhab.binding.astro.internal.config.AstroChannelConfig;
 import org.openhab.binding.astro.internal.config.AstroThingConfig;
@@ -55,7 +56,6 @@
 import org.openhab.core.library.types.DecimalType;
 import org.openhab.core.library.types.StringType;
 import org.openhab.core.scheduler.CronScheduler;
-import org.openhab.core.scheduler.ScheduledCompletableFuture;
 import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
 import org.openhab.core.thing.Thing;
@@ -71,13 +71,73 @@
 /**
  * Base ThingHandler for all Astro handlers.
  *
+ * @implNote
+ *           The scheduling of events allows graceful handling of scheduling an event that is already scheduled,
+ *           to allow scheduling to take place at any time of day, even when while the rescheduling itself takes place.
+ *           <p>
+ *           This is achieved by storing all scheduled events in {@link #scheduledFutures}, which identifies "which
+ *           event" it is using a string identifier.
+ *           <p>
+ *           If an event is being scheduled and the same event already exists in {@link #scheduledFutures}, the
+ *           following
+ *           logic applies:
+ *           <ul>
+ *           <li>If the existing/old event is {@link ScheduledFuture#isDone()}, which means either completed or
+ *           cancelled,
+ *           the new event will be scheduled without further due.</li>
+ *           <li>If the existing/old event hasn't yet fired, it will be decided whether the existing/old event should
+ *           remain
+ *           scheduled, or if the new one should be scheduled. Under no circumstances will both be allowed. Which one
+ *           "wins"
+ *           is decided by the following logic:</li>
+ *           <ul>
+ *           <li>If the existing/old event is scheduled to fire at or later than {@link #MIN_TIME_TO_SCHEDULE_MS} from
+ *           the moment
+ *           the evaluation takes place, the existing/old event will be cancelled and the new one scheduled in its
+ *           place.</li>
+ *           <li>If the existing/old event is scheduled to fire in less than {@link #MIN_TIME_TO_SCHEDULE_MS} <i>and</i>
+ *           the
+ *           difference in scheduled time for the old and the new event is less than or equal to
+ *           {@link #MAX_SCHEDULE_DIFFERENCE_MS},
+ *           the existing/old event is allowed to remain scheduled and the new event is discarded. This is to make sure
+ *           that the
+ *           event doesn't fire twice in case the cancellation can't be executed in time to prevent execution. But, if
+ *           the
+ *           difference between the two schedules is too large, the existing/old schedule might be scheduled too
+ *           inaccurately,
+ *           in which case a (desperate) attempt it made at cancelling the old one regardless. This is an extremely
+ *           unlikely
+ *           scenario to actually occur.</li>
+ *           <li>In any other case, the existing/old event is cancelled and the new one scheduled instead.</li>
+ *           </ul>
+ *           </ul>
+ *
  * @author Gerhard Riegler - Initial contribution
  * @author Amit Kumar Mondal - Implementation to be compliant with ESH Scheduler
+ * @author Ravi Nadahar - Refactored scheduling
  */
 @NonNullByDefault
 public abstract class AstroThingHandler extends BaseThingHandler {
     private static final String DAILY_MIDNIGHT = "30 0 0 * * ? *";
 
+    /**
+     * Minimum delay (in milliseconds) that must remain until a job is executed before it will be
+     * scheduled. This prevents attempting to schedule jobs that would effectively execute "now" or
+     * in the past due to clock skew, rounding, or scheduler latency. The value is intentionally
+     * very small compared to astro event intervals, while still providing a safety margin over
+     * typical scheduler/resolution jitter.
+     */
+    private static final long MIN_TIME_TO_SCHEDULE_MS = 10L;
+
+    /**
+     * Maximum time difference (in milliseconds) between two candidate execution times for them to
+     * be considered equivalent in the schedule deduplication logic. If two schedules differ by at
+     * most this amount, they are treated as the same schedule and a new job is not created. This
+     * tolerance compensates for small rounding differences and minor time calculation jitter
+     * without affecting the semantics of astro events, where relevant time spans are much larger.
+     */
+    private static final long MAX_SCHEDULE_DIFFERENCE_MS = 20L;
+
     /** Logger Instance */
     private final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
     private final SimpleDateFormat loggerFormatter = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS", Locale.ROOT);
@@ -92,16 +152,13 @@ public abstract class AstroThingHandler extends BaseThingHandler {
     private final Lock monitor = new ReentrantLock();
 
     // All access must be guarded by "monitor"
-    private final Set<ScheduledFuture<?>> scheduledFutures = new HashSet<>();
+    private final Map<String, ScheduledFuture<?>> scheduledFutures = new HashMap<>();
 
     // All access must be guarded by "monitor"
     private boolean linkedPositionalChannels;
 
     protected AstroThingConfig thingConfig = new AstroThingConfig();
 
-    // All access must be guarded by "monitor"
-    private @Nullable ScheduledCompletableFuture<?> dailyJob;
-
     /** The source of the current time */
     protected final InstantSource instantSource;
 
@@ -230,8 +287,8 @@ private void restartJobs() {
                 Locale locale = localeProvider.getLocale();
                 // Daily Job
                 Job runnable = getDailyJob(zone, locale);
-                dailyJob = cronScheduler.schedule(runnable, DAILY_MIDNIGHT);
-                logger.debug("Scheduled {} at midnight", dailyJob);
+                scheduledFutures.put(AstroBindingConstants.DAILY_JOB, cronScheduler.schedule(runnable, DAILY_MIDNIGHT));
+                logger.debug("Scheduled daily '{}' job at midnight", getThing().getUID());
                 // Execute daily startup job immediately
                 runnable.run();
 
@@ -242,7 +299,7 @@ private void restartJobs() {
                     Job positionalJob = new PositionalJob(this);
                     ScheduledFuture<?> future = scheduler.scheduleAtFixedRate(positionalJob, 0, thingConfig.interval,
                             TimeUnit.SECONDS);
-                    scheduledFutures.add(future);
+                    scheduledFutures.put(AstroBindingConstants.POSITIONAL_JOB, future);
                     logger.info("Scheduled {} every {} seconds", positionalJob, thingConfig.interval);
                 }
             }
@@ -258,12 +315,7 @@ private void stopJobs() {
         logger.debug("Stopping scheduled jobs for thing {}", getThing().getUID());
         monitor.lock();
         try {
-            ScheduledCompletableFuture<?> job = dailyJob;
-            if (job != null) {
-                job.cancel(true);
-            }
-            dailyJob = null;
-            for (ScheduledFuture<?> future : scheduledFutures) {
+            for (ScheduledFuture<?> future : scheduledFutures.values()) {
                 if (!future.isDone()) {
                     future.cancel(true);
                 }
@@ -335,12 +387,24 @@ public void triggerEvent(String channelId, String event) {
     /**
      * Adds the provided {@link Job} to the queue (cannot be {@code null})
      */
-    private void schedule(Job job, long sleepTimeMs) {
+    private void schedule(String identifier, Job job, long sleepTimeMs) {
         monitor.lock();
         try {
             tidyScheduledFutures();
-            ScheduledFuture<?> future = scheduler.schedule(job, sleepTimeMs, TimeUnit.MILLISECONDS);
-            scheduledFutures.add(future);
+            ScheduledFuture<?> future = scheduledFutures.get(identifier);
+            if (future != null && !future.isDone()) {
+                // The event is already scheduled
+                long delay;
+                if ((delay = future.getDelay(TimeUnit.MILLISECONDS)) < MIN_TIME_TO_SCHEDULE_MS
+                        && Math.abs(delay - sleepTimeMs) <= MAX_SCHEDULE_DIFFERENCE_MS) {
+                    // if the previously scheduled event is about to run very soon and their schedules are similar,
+                    // we don't know if we can cancel it in time, so we let it run and don't schedule the new one.
+                    return;
+                }
+                future.cancel(true);
+            }
+            future = scheduler.schedule(job, sleepTimeMs, TimeUnit.MILLISECONDS);
+            scheduledFutures.put(identifier, future);
         } finally {
             monitor.unlock();
         }
@@ -349,11 +413,11 @@ private void schedule(Job job, long sleepTimeMs) {
     /**
      * Adds the provided {@link Job} to the queue (cannot be {@code null})
      */
-    public void schedule(Job job, Calendar eventAt) {
+    public void schedule(String identifier, Job job, Calendar eventAt) {
         // We don't use instantSource here, because we always want to schedule relative to the system clock
         long sleepTime = eventAt.getTimeInMillis() - System.currentTimeMillis();
         if (sleepTime >= 0L) {
-            schedule(job, sleepTime);
+            schedule(identifier, job, sleepTime);
             if (logger.isDebugEnabled()) {
                 final String formattedDate = this.loggerFormatter.format(eventAt.getTime());
                 logger.debug("Scheduled {} in {}ms (at {})", job, sleepTime, formattedDate);
@@ -364,11 +428,11 @@ public void schedule(Job job, Calendar eventAt) {
         }
     }
 
-    public void schedule(Job job, Instant eventAt) {
+    public void schedule(String identifier, Job job, Instant eventAt) {
         // We don't use instantSource here, because we always want to schedule relative to the system clock
         long sleepTime = eventAt.toEpochMilli() + 1L - System.currentTimeMillis();
         if (sleepTime >= 0L) {
-            schedule(job, sleepTime);
+            schedule(identifier, job, sleepTime);
             if (logger.isDebugEnabled()) {
                 logger.debug("Scheduled {} in {}ms (at {})", job, sleepTime, eventAt.atZone(ZoneId.systemDefault()));
             }
@@ -381,8 +445,9 @@ public void schedule(Job job, Instant eventAt) {
     private void tidyScheduledFutures() {
         monitor.lock();
         try {
-            for (Iterator<ScheduledFuture<?>> iterator = scheduledFutures.iterator(); iterator.hasNext();) {
-                ScheduledFuture<?> future = iterator.next();
+            ScheduledFuture<?> future;
+            for (Iterator<ScheduledFuture<?>> iterator = scheduledFutures.values().iterator(); iterator.hasNext();) {
+                future = iterator.next();
                 if (future.isDone()) {
                     logger.trace("Tidying up done future {}", future);
                     iterator.remove();