fix repeating timers (#133)

Thanks @querdenker2k , solves #132
seaside1 · Feb 18, 2023 · 0ac526c · 0ac526c
1 parent 1bc72b6
commit 0ac526c
Show file tree

Hide file tree

Showing 10 changed files with 116 additions and 49 deletions.
diff --git a/src/main/java/org/openhab/automation/jrule/internal/handler/JRuleTimerHandler.java b/src/main/java/org/openhab/automation/jrule/internal/handler/JRuleTimerHandler.java
@@ -126,6 +126,8 @@ public synchronized JRuleTimer createRepeatingTimer(@Nullable String timerName,
         JRuleTimer timer = new JRuleTimer(newTimerName, newTimers, context != null ? context : getCurrentContext(),
                 delay);
         timers.add(timer);
+        logger.trace("added repeating timers '{}': {}", newTimerName, newTimers.size());
+        getTimers(newTimerName);
 
         newTimers.forEach(
                 future -> future.thenAccept(s -> executorService.submit(() -> invokeTimerInternal(timer, function))));
@@ -148,7 +150,10 @@ private static JRuleExecutionContext getCurrentContext() {
     }
 
     private synchronized List<JRuleTimer> getTimers(String timerName) {
-        return timers.stream().filter(timer -> timer.name.equals(timerName)).collect(Collectors.toList());
+        List<JRuleTimer> list = timers.stream().filter(timer -> timer.name.equals(timerName))
+                .collect(Collectors.toList());
+        logger.trace("timers for name '{}': {}", timerName, list.size());
+        return list;
     }
 
     private void invokeTimerInternal(JRuleTimer timer, Runnable runnable) {
@@ -170,12 +175,15 @@ private void invokeTimerInternal(JRuleTimer timer, Runnable runnable) {
             MDC.remove(JRuleEngine.MDC_KEY_RULE);
             MDC.remove(JRuleEngine.MDC_KEY_TIMER);
             logger.debug("Removing thread local after rule completion");
-            timers.remove(timer);
+            if (timer.isDone()) {
+                removeTimer(timer.name);
+            }
             JRule.JRULE_EXECUTION_CONTEXT.remove();
         }
     }
 
     private synchronized void removeTimer(String timerName) {
+        logger.trace("remove timer: '{}'", timerName);
         timers.removeIf(timer -> timer.name.equals(timerName));
     }
 
@@ -212,7 +220,9 @@ public JRuleTimer(String name, List<CompletableFuture<?>> futures, JRuleExecutio
         }
 
         public void cancel() {
+            logger.debug("before: {}", futures);
             this.futures.forEach(future -> future.cancel(false));
+            logger.debug("after: {}", futures);
         }
 
         public String getLogName() {

diff --git a/src/main/java/org/openhab/automation/jrule/internal/test/JRuleMockedEventBus.java b/src/main/java/org/openhab/automation/jrule/internal/test/JRuleMockedEventBus.java
@@ -12,8 +12,9 @@
  */
 package org.openhab.automation.jrule.internal.test;
 
-import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.Executor;
+import java.util.concurrent.Executors;
 
 import org.openhab.automation.jrule.internal.events.JRuleEventSubscriber;
 import org.openhab.core.events.Event;
@@ -24,23 +25,29 @@
  * @author Joseph (Seaside) Hagberg - Initial contribution
  */
 public class JRuleMockedEventBus extends JRuleEventSubscriber {
+    private final Executor executor = Executors.newFixedThreadPool(10);
 
-    private final List<Event> eventList;
+    public JRuleMockedEventBus() {
+    }
 
-    public JRuleMockedEventBus(String eventBusResourceName) {
-        super();
-        JRuleTestEventLogParser parser = new JRuleTestEventLogParser(eventBusResourceName);
-        eventList = parser.parse();
+    public void start() {
+        startSubscriber();
     }
 
-    public JRuleMockedEventBus(List<Event> events) {
-        eventList = new ArrayList<>();
-        eventList.addAll(events);
+    public void fire(boolean async, List<Event> events) {
+        if (async) {
+            events.forEach(event -> executor.execute(() -> super.receive(event)));
+        } else {
+            events.forEach(super::receive);
+        }
     }
 
-    public void start() {
-        startSubscriber();
-        eventList.forEach(super::receive);
+    public void fire(String eventBusResourceName) {
+        JRuleTestEventLogParser parser = new JRuleTestEventLogParser(eventBusResourceName);
+        fire(false, parser.parse());
+    }
+
+    public void stop() {
         stopSubscriber();
     }
 }
diff --git a/src/test/java/org/openhab/automation/jrule/rules/user/TestRules.java b/src/test/java/org/openhab/automation/jrule/rules/user/TestRules.java
@@ -341,7 +341,7 @@ public void timers(JRuleItemEvent event) throws JRuleExecutionException {
 
         // repeating timer
         AtomicInteger counter = new AtomicInteger(0);
-        createRepeatingTimer(Duration.ofMillis(1), 200,
+        createRepeatingTimer(Duration.ofMillis(10), 20,
                 () -> logInfo("TIMER-REPEATING: '{}'", counter.getAndIncrement()));
 
         // cancel normal timer

diff --git a/src/test/java/org/openhab/binding/jrule/internal/rules/JRuleAbstractTest.java b/src/test/java/org/openhab/binding/jrule/internal/rules/JRuleAbstractTest.java
@@ -18,6 +18,7 @@
 import java.util.List;
 import java.util.Map;
 
+import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.TestInstance;
 import org.mockito.Mockito;
@@ -43,6 +44,12 @@ public abstract class JRuleAbstractTest {
 
     protected ItemRegistry itemRegistry;
     protected CollectingEventPublisher eventPublisher;
+    private JRuleMockedEventBus eventBus = new JRuleMockedEventBus();
+
+    @AfterAll
+    protected void shutdown() {
+        eventBus.stop();
+    }
 
     @BeforeAll
     protected void initEngine() {
@@ -60,6 +67,8 @@ protected void initEngine() {
 
         eventPublisher = new CollectingEventPublisher();
         JRuleEventHandler.get().setEventPublisher(eventPublisher);
+
+        eventBus.start();
     }
 
     protected <T extends JRule> T initRule(Class<T> rule) {
@@ -69,9 +78,8 @@ protected <T extends JRule> T initRule(Class<T> rule) {
         return spyRule;
     }
 
-    protected void fireEvents(List<Event> events) {
-        JRuleMockedEventBus eventBus = new JRuleMockedEventBus(events);
-        eventBus.start();
+    protected void fireEvents(boolean async, List<Event> events) {
+        eventBus.fire(async, events);
     }
 
     protected void setState(GenericItem item, State state) throws ItemNotFoundException {

diff --git a/src/test/java/org/openhab/binding/jrule/internal/rules/delayed/JRuleDelayedTest.java b/src/test/java/org/openhab/binding/jrule/internal/rules/delayed/JRuleDelayedTest.java
@@ -12,7 +12,6 @@
  */
 package org.openhab.binding.jrule.internal.rules.delayed;
 
-import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
@@ -54,7 +53,7 @@ public void testDelayed() throws ItemNotFoundException, InterruptedException {
 
         JRuleItemRegistry.get(JRuleDelayedTestRules.TARGET_ITEM, TargetItem.class);
         ZonedDateTime fired = ZonedDateTime.now();
-        fireEvents(List.of(itemChangeEvent(JRuleDelayedTestRules.TRIGGER_ITEM, "nothing", "2s")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleDelayedTestRules.TRIGGER_ITEM, "nothing", "2s")));
         Thread.sleep(3000); // Wait for delayed execution
         verify(rule, times(1)).test2s();
         List<CollectingEventPublisher.Container> events = eventPublisher
@@ -71,7 +70,7 @@ public void testDelayedNotWaiting() throws ItemNotFoundException, InterruptedExc
 
         JRuleItemRegistry.get(JRuleDelayedTestRules.TARGET_ITEM, TargetItem.class);
         ZonedDateTime fired = ZonedDateTime.now();
-        fireEvents(List.of(itemChangeEvent(JRuleDelayedTestRules.TRIGGER_ITEM, "nothing", "2s")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleDelayedTestRules.TRIGGER_ITEM, "nothing", "2s")));
         verify(rule, times(0)).test2s();
     }
 

diff --git a/src/test/java/org/openhab/binding/jrule/internal/rules/timers/JRuleTimerTest.java b/src/test/java/org/openhab/binding/jrule/internal/rules/timers/JRuleTimerTest.java
@@ -23,6 +23,7 @@
 import org.openhab.binding.jrule.internal.rules.JRuleAbstractTest;
 import org.openhab.core.events.Event;
 import org.openhab.core.items.ItemNotFoundException;
+import org.openhab.core.items.events.ItemCommandEvent;
 import org.openhab.core.items.events.ItemEventFactory;
 import org.openhab.core.library.items.StringItem;
 import org.openhab.core.library.types.StringType;
@@ -42,7 +43,7 @@ public void testTimer() throws ItemNotFoundException, InterruptedException {
         setState(new StringItem(JRuleTimerTestRules.TARGET_ITEM), UnDefType.UNDEF);
 
         JRuleItemRegistry.get(JRuleTimerTestRules.TARGET_ITEM, TargetItem.class);
-        fireEvents(List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "timers")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "timers")));
         verify(rule, times(1)).testTimers();
         Thread.sleep(3000); // Wait for timer inside rule to execute
         assertTrue(eventPublisher.hasCommandEvent(JRuleTimerTestRules.TARGET_ITEM, "command"));
@@ -66,7 +67,7 @@ public void testRepeatingTimer() throws ItemNotFoundException, InterruptedExcept
         JRuleItemRegistry.get(JRuleTimerTestRules.TARGET_ITEM_REPEATING, TargetItem.class);
         JRuleItemRegistry.get(JRuleTimerTestRules.TARGET_ITEM_REPEATING_WITH_NAME, TargetItem.class);
         JRuleItemRegistry.get(JRuleTimerTestRules.TARGET_ITEM_REPEATING_WITH_NAME_REPLACED, TargetItem.class);
-        fireEvents(List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "timers-repeating")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "timers-repeating")));
         verify(rule, times(1)).testRepeatingTimers();
         Thread.sleep(3000); // Wait for timer inside rule to execute
         assertTrue(eventPublisher.isLastCommandEvent(JRuleTimerTestRules.TARGET_ITEM_REPEATING_WITH_NAME,
@@ -76,14 +77,33 @@ public void testRepeatingTimer() throws ItemNotFoundException, InterruptedExcept
         assertTrue(eventPublisher.isLastCommandEvent(JRuleTimerTestRules.TARGET_ITEM_REPEATING, "repeating-10"));
     }
 
+    @Test
+    public void testRepeatingTimerComplex() throws ItemNotFoundException, InterruptedException {
+        JRuleTimerTestRules rule = initRule(JRuleTimerTestRules.class);
+        // Set item state in ItemRegistry
+        setState(new StringItem(JRuleTimerTestRules.TARGET_ITEM_REPEATING_WITH_NAME_REPLACED), UnDefType.UNDEF);
+
+        JRuleItemRegistry.get(JRuleTimerTestRules.TARGET_ITEM_REPEATING_WITH_NAME_REPLACED, TargetItem.class);
+        fireEvents(true, List.of(itemCommandEvent(JRuleTimerTestRules.TRIGGER_ITEM, "timers-repeating-complex")));
+        Thread.sleep(500);
+        fireEvents(true, List.of(itemCommandEvent(JRuleTimerTestRules.TRIGGER_ITEM, "timers-repeating-complex")));
+        Thread.sleep(3000); // Wait for timer inside rule to execute
+        verify(rule, times(2)).testRepeatingTimersComplex();
+        assertEquals(1,
+                eventPublisher.getCommandEvents(JRuleTimerTestRules.TARGET_ITEM_REPEATING_WITH_NAME_REPLACED).stream()
+                        .filter(c -> ((ItemCommandEvent) c.getEvent()).getItemCommand().toString()
+                                .equals("repeating-with-name-replaced-5"))
+                        .count());
+    }
+
     @Test
     public void testGetTimedLock() throws ItemNotFoundException, InterruptedException {
         JRuleTimerTestRules rule = initRule(JRuleTimerTestRules.class);
         // Set item state in ItemRegistry
         setState(new StringItem(JRuleTimerTestRules.TARGET_ITEM), UnDefType.UNDEF);
 
         JRuleItemRegistry.get(JRuleTimerTestRules.TARGET_ITEM, TargetItem.class);
-        fireEvents(List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "locks")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "locks")));
         verify(rule, times(1)).testLocks();
         Thread.sleep(3000); // Wait for timer inside rule to execute
         assertTrue(eventPublisher.hasCommandEvent(JRuleTimerTestRules.TARGET_ITEM, "first: true"));
@@ -98,11 +118,11 @@ public void testDebounce() throws ItemNotFoundException, InterruptedException {
         setState(new StringItem(JRuleTimerTestRules.TARGET_ITEM), UnDefType.UNDEF);
 
         JRuleItemRegistry.get(JRuleTimerTestRules.TARGET_ITEM, TargetItem.class);
-        fireEvents(List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "debounce")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "debounce")));
         Thread.sleep(600);
-        fireEvents(List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "debounce")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "debounce")));
         Thread.sleep(600);
-        fireEvents(List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "debounce")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleTimerTestRules.TRIGGER_ITEM, "nothing", "debounce")));
         verify(rule, times(1)).testDebounce();
         Thread.sleep(3000); // Wait for timer inside rule to execute
         assertEquals(1, eventPublisher.countCommandEvent(JRuleTimerTestRules.TARGET_ITEM, "no debounce"));
@@ -111,4 +131,8 @@ public void testDebounce() throws ItemNotFoundException, InterruptedException {
     private Event itemChangeEvent(String item, String from, String to) {
         return ItemEventFactory.createStateChangedEvent(item, new StringType(to), new StringType(from));
     }
+
+    private Event itemCommandEvent(String item, String to) {
+        return ItemEventFactory.createCommandEvent(item, new StringType(to));
+    }
 }
diff --git a/src/test/java/org/openhab/binding/jrule/internal/rules/timers/JRuleTimerTestRules.java b/src/test/java/org/openhab/binding/jrule/internal/rules/timers/JRuleTimerTestRules.java
@@ -96,6 +96,19 @@ public void testRepeatingTimers() {
                 () -> repeatingItem.sendCommand("repeating-" + repeatingCounter.incrementAndGet()));
     }
 
+    @JRuleName("Repeating Timers")
+    @JRuleWhenItemReceivedCommand(item = TRIGGER_ITEM, command = "timers-repeating-complex")
+    public void testRepeatingTimersComplex() {
+        final AtomicInteger counter = new AtomicInteger(0);
+
+        JRuleStringItem repeatingWithNameReplacedItem = JRuleStringItem
+                .forName(TARGET_ITEM_REPEATING_WITH_NAME_REPLACED);
+
+        final String TIMER_NAME = "repeating-with-name-replaced";
+        createOrReplaceRepeatingTimer(TIMER_NAME, Duration.ofMillis(200), 5,
+                () -> repeatingWithNameReplacedItem.sendCommand(TIMER_NAME + "-" + counter.incrementAndGet()));
+    }
+
     @JRuleName("Rule name")
     @JRuleLogName("Rule log name")
     @JRuleWhenItemChange(item = TRIGGER_ITEM, to = "locks")

diff --git a/...ava/org/openhab/binding/jrule/internal/rules/triggers/itemchange/JRuleItemChangeTest.java b/...ava/org/openhab/binding/jrule/internal/rules/triggers/itemchange/JRuleItemChangeTest.java
@@ -50,7 +50,7 @@ public void initTestClass() throws ItemNotFoundException {
     public void testItemChange_no_from_to() {
         JRuleItemChangeRules rule = initRule(JRuleItemChangeRules.class);
         // Only last event should trigger rule method
-        fireEvents(
+        fireEvents(false,
                 List.of(itemChangeEvent("other_item", "2", "1"), itemChangeEvent(JRuleItemChangeRules.ITEM, "2", "1")));
         verify(rule, times(1)).itemChange(Mockito.any(JRuleEvent.class));
     }
@@ -59,7 +59,7 @@ public void testItemChange_no_from_to() {
     public void testItemChange_from() {
         JRuleItemChangeRules rule = initRule(JRuleItemChangeRules.class);
         // Only last event should trigger rule method
-        fireEvents(List.of(itemChangeEvent(JRuleItemChangeRules.ITEM_FROM, "2", "1"),
+        fireEvents(false, List.of(itemChangeEvent(JRuleItemChangeRules.ITEM_FROM, "2", "1"),
                 itemChangeEvent(JRuleItemChangeRules.ITEM_FROM, "1", "2")));
         verify(rule, times(1)).itemChangeFrom(Mockito.any(JRuleEvent.class));
     }
@@ -68,7 +68,7 @@ public void testItemChange_from() {
     public void testItemChange_to() {
         JRuleItemChangeRules rule = initRule(JRuleItemChangeRules.class);
         // Only last event should trigger rule method
-        fireEvents(List.of(itemChangeEvent(JRuleItemChangeRules.ITEM_TO, "1", "2"),
+        fireEvents(false, List.of(itemChangeEvent(JRuleItemChangeRules.ITEM_TO, "1", "2"),
                 itemChangeEvent(JRuleItemChangeRules.ITEM_TO, "2", "1")));
         verify(rule, times(1)).itemChangeTo(Mockito.any(JRuleEvent.class));
     }
@@ -77,16 +77,17 @@ public void testItemChange_to() {
     public void testItemChange_from_to() {
         JRuleItemChangeRules rule = initRule(JRuleItemChangeRules.class);
         // Only last event should trigger rule method
-        fireEvents(List.of(itemChangeEvent(JRuleItemChangeRules.ITEM_FROM_TO, "2", "1"),
-                itemChangeEvent(JRuleItemChangeRules.ITEM_FROM_TO, "3", "2"),
-                itemChangeEvent(JRuleItemChangeRules.ITEM_FROM_TO, "1", "2")));
+        fireEvents(false,
+                List.of(itemChangeEvent(JRuleItemChangeRules.ITEM_FROM_TO, "2", "1"),
+                        itemChangeEvent(JRuleItemChangeRules.ITEM_FROM_TO, "3", "2"),
+                        itemChangeEvent(JRuleItemChangeRules.ITEM_FROM_TO, "1", "2")));
         verify(rule, times(1)).itemChangeFromTo(Mockito.any(JRuleEvent.class));
     }
 
     @Test
     public void testItemChange_multipleMatchingContexts() {
         JRuleItemChangeRules rule = initRule(JRuleItemChangeRules.class);
-        fireEvents(List.of(itemChangeEvent(JRuleItemChangeRules.ITEM_DUPLICATE, "2", "1")));
+        fireEvents(false, List.of(itemChangeEvent(JRuleItemChangeRules.ITEM_DUPLICATE, "2", "1")));
         verify(rule, times(1)).duplicateMatchingWhen(Mockito.any(JRuleEvent.class));
     }
 

diff --git a/...nding/jrule/internal/rules/triggers/itemchangecondition/JRuleItemChangeConditionTest.java b/...nding/jrule/internal/rules/triggers/itemchangecondition/JRuleItemChangeConditionTest.java
@@ -50,17 +50,18 @@ public void initTestClass() throws ItemNotFoundException {
     public void testItemChange_from_to() {
         JRuleItemChangeConditionRules rule = initRule(JRuleItemChangeConditionRules.class);
         // Only last event should trigger rule method
-        fireEvents(List.of(itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO, "12", "13"),
-                itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO, "10", "11"),
-                itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO, "11", "12")));
+        fireEvents(false,
+                List.of(itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO, "12", "13"),
+                        itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO, "10", "11"),
+                        itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO, "11", "12")));
         verify(rule, times(1)).itemChangeFromTo(Mockito.any(JRuleEvent.class));
     }
 
     @Test
     public void testItemChange_from_to_2() {
         JRuleItemChangeConditionRules rule = initRule(JRuleItemChangeConditionRules.class);
         // Only last event should trigger rule method
-        fireEvents(List.of(itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO_2, "10", "20"),
+        fireEvents(false, List.of(itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO_2, "10", "20"),
                 itemChangeEvent(JRuleItemChangeConditionRules.ITEM_FROM_TO_2, "9", "21")));
         verify(rule, times(1)).itemChangeFromTo2(Mockito.any(JRuleEvent.class));
     }