[jsscripting] Extend synchronization to common ScriptEngine methods &…

… Switch to ReentrantLock

This extends the multi-thread synchronization to "eval" and "invokeMethod" and moves synchronization for "invokeFunction" to the InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable class.
The synchronization mechanism changed from using synchronized to using a ReentrantLock together with catch_finally to avoid having deadlocks when an exception is thrown.
Fixes the multi-thread access requested warnings described in the community (https://community.openhab.org/t/openhab-3-4-milestone-discussion/138093/130) and related to openhab/openhab-core#3180.

Signed-off-by: Florian Hotze <[email protected]>

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSRuntimeFeatures.java

@@ -14,6 +14,7 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.locks.Lock;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.openhab.automation.jsscripting.internal.threading.ThreadsafeTimers;
@@ -31,7 +32,7 @@ public class JSRuntimeFeatures {
     private final Map<String, Object> features = new HashMap<>();
     public final ThreadsafeTimers threadsafeTimers;
 
-    JSRuntimeFeatures(Object lock, JSScriptServiceUtil jsScriptServiceUtil) {
+    JSRuntimeFeatures(Lock lock, JSScriptServiceUtil jsScriptServiceUtil) {
         this.threadsafeTimers = new ThreadsafeTimers(lock, jsScriptServiceUtil.getScriptExecution(),
                 jsScriptServiceUtil.getScheduler());
 

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSScriptServiceUtil.java

@@ -12,6 +12,8 @@
  */
 package org.openhab.automation.jsscripting.internal;
 
+import java.util.concurrent.locks.Lock;
+
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.openhab.core.automation.module.script.action.ScriptExecution;
 import org.openhab.core.scheduler.Scheduler;
@@ -44,7 +46,7 @@ public ScriptExecution getScriptExecution() {
         return scriptExecution;
     }
 
-    public JSRuntimeFeatures getJSRuntimeFeatures(Object lock) {
+    public JSRuntimeFeatures getJSRuntimeFeatures(Lock lock) {
         return new JSRuntimeFeatures(lock, this);
     }
 }

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java

@@ -30,6 +30,8 @@
 import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
 import java.util.function.Function;
 
@@ -58,7 +60,8 @@
  * @author Jonathan Gilbert - Initial contribution
  * @author Dan Cunningham - Script injections
  * @author Florian Hotze - Create lock object for multi-thread synchronization; Inject the {@link JSRuntimeFeatures}
- *         into the JS context; Fix memory leak caused by HostObject by making HostAccess reference static
+ *         into the JS context; Fix memory leak caused by HostObject by making HostAccess reference static; Switch to
+ *         {@link Lock} for multi-thread synchronization
  */
 public class OpenhabGraalJSScriptEngine
         extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable<GraalJSScriptEngine> {
@@ -84,7 +87,7 @@ public class OpenhabGraalJSScriptEngine
             .build();
 
     /** Shared lock object for synchronization of multi-thread access */
-    private final Object lock = new Object();
+    private final Lock lock = new ReentrantLock();
     private final JSRuntimeFeatures jsRuntimeFeatures;
 
     // these fields start as null because they are populated on first use
@@ -174,6 +177,8 @@ public Path toRealPath(Path path, LinkOption... linkOptions) throws IOException
 
     @Override
     protected void beforeInvocation() {
+        lock.lock();
+
         if (initialized) {
             return;
         }
@@ -227,11 +232,8 @@ protected void beforeInvocation() {
     }
 
     @Override
-    public Object invokeFunction(String s, Object... objects) throws ScriptException, NoSuchMethodException {
-        // Synchronize multi-thread access to avoid exceptions when reloading a script file while the script is running
-        synchronized (lock) {
-            return super.invokeFunction(s, objects);
-        }
+    protected void afterInvocation() {
+        lock.unlock();
     }
 
     @Override

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/ScriptExtensionModuleProvider.java

@@ -16,6 +16,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.locks.Lock;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.graalvm.polyglot.Context;
@@ -29,19 +30,20 @@
  * Class providing script extensions via CommonJS modules.
  *
  * @author Jonathan Gilbert - Initial contribution
- * @author Florian Hotze - Pass in lock object for multi-thread synchronization
+ * @author Florian Hotze - Pass in lock object for multi-thread synchronization; Switch to {@link Lock} for multi-thread
+ *         synchronization
  */
 
 @NonNullByDefault
 public class ScriptExtensionModuleProvider {
 
     private static final String RUNTIME_MODULE_PREFIX = "@runtime";
     private static final String DEFAULT_MODULE_NAME = "Defaults";
-    private final Object lock;
+    private final Lock lock;
 
     private final ScriptExtensionAccessor scriptExtensionAccessor;
 
-    public ScriptExtensionModuleProvider(ScriptExtensionAccessor scriptExtensionAccessor, Object lock) {
+    public ScriptExtensionModuleProvider(ScriptExtensionAccessor scriptExtensionAccessor, Lock lock) {
         this.scriptExtensionAccessor = scriptExtensionAccessor;
         this.lock = lock;
     }

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.java

@@ -38,6 +38,9 @@ public InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable(T delegat
     protected void beforeInvocation() {
     }
 
+    protected void afterInvocation() {
+    }
+
     protected ScriptException afterThrowsInvocation(ScriptException se) {
         return se;
     }
@@ -49,6 +52,8 @@ public Object eval(String s, ScriptContext scriptContext) throws ScriptException
             return super.eval(s, scriptContext);
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            afterInvocation();
         }
     }
 
@@ -59,6 +64,8 @@ public Object eval(Reader reader, ScriptContext scriptContext) throws ScriptExce
             return super.eval(reader, scriptContext);
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            afterInvocation();
         }
     }
 
@@ -69,6 +76,8 @@ public Object eval(String s) throws ScriptException {
             return super.eval(s);
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            afterInvocation();
         }
     }
 
@@ -79,6 +88,8 @@ public Object eval(Reader reader) throws ScriptException {
             return super.eval(reader);
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            afterInvocation();
         }
     }
 
@@ -89,6 +100,8 @@ public Object eval(String s, Bindings bindings) throws ScriptException {
             return super.eval(s, bindings);
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            afterInvocation();
         }
     }
 
@@ -99,6 +112,8 @@ public Object eval(Reader reader, Bindings bindings) throws ScriptException {
             return super.eval(reader, bindings);
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            afterInvocation();
         }
     }
 
@@ -109,6 +124,8 @@ public Object invokeMethod(Object o, String s, Object... objects) throws ScriptE
             return super.invokeMethod(o, s, objects);
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            afterInvocation();
         }
     }
 
@@ -119,6 +136,8 @@ public Object invokeFunction(String s, Object... objects) throws ScriptException
             return super.invokeFunction(s, objects);
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            afterInvocation();
         }
     }
 }

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeSimpleRuleDelegate.java

@@ -15,6 +15,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.locks.Lock;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -38,7 +39,7 @@
 @NonNullByDefault
 class ThreadsafeSimpleRuleDelegate implements Rule, SimpleRuleActionHandler {
 
-    private final Object lock;
+    private final Lock lock;
     private final SimpleRule delegate;
 
     /**
@@ -47,16 +48,19 @@ class ThreadsafeSimpleRuleDelegate implements Rule, SimpleRuleActionHandler {
      * @param lock rule executions will synchronize on this object
      * @param delegate the delegate to forward invocations to
      */
-    ThreadsafeSimpleRuleDelegate(Object lock, SimpleRule delegate) {
+    ThreadsafeSimpleRuleDelegate(Lock lock, SimpleRule delegate) {
         this.lock = lock;
         this.delegate = delegate;
     }
 
     @Override
     @NonNullByDefault({})
     public Object execute(Action module, Map<String, ?> inputs) {
-        synchronized (lock) {
+        try {
+            lock.lock();
             return delegate.execute(module, inputs);
+        } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
+            lock.unlock();
         }
     }
 

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeTimers.java

@@ -19,6 +19,7 @@
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.Lock;
 
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.core.automation.module.script.action.ScriptExecution;
@@ -35,15 +36,15 @@
  *         Threadsafe reimplementation of the timer creation methods of {@link ScriptExecution}
  */
 public class ThreadsafeTimers {
-    private final Object lock;
+    private final Lock lock;
     private final Scheduler scheduler;
     private final ScriptExecution scriptExecution;
     // Mapping of positive, non-zero integer values (used as timeoutID or intervalID) and the Scheduler
     private final Map<Long, ScheduledCompletableFuture<Object>> idSchedulerMapping = new ConcurrentHashMap<>();
     private AtomicLong lastId = new AtomicLong();
     private String identifier = "noIdentifier";
 
-    public ThreadsafeTimers(Object lock, ScriptExecution scriptExecution, Scheduler scheduler) {
+    public ThreadsafeTimers(Lock lock, ScriptExecution scriptExecution, Scheduler scheduler) {
         this.lock = lock;
         this.scheduler = scheduler;
         this.scriptExecution = scriptExecution;
@@ -79,8 +80,12 @@ public Timer createTimer(ZonedDateTime instant, Runnable closure) {
      */
     public Timer createTimer(@Nullable String identifier, ZonedDateTime instant, Runnable closure) {
         return scriptExecution.createTimer(identifier, instant, () -> {
-            synchronized (lock) {
+            try {
+                lock.lock();
                 closure.run();
+            } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid
+                        // deadlocks
+                lock.unlock();
             }
         });
     }
@@ -111,9 +116,13 @@ public long setTimeout(Runnable callback, Long delay) {
     public long setTimeout(Runnable callback, Long delay, Object... args) {
         long id = lastId.incrementAndGet();
         ScheduledCompletableFuture<Object> future = scheduler.schedule(() -> {
-            synchronized (lock) {
+            try {
+                lock.lock();
                 callback.run();
                 idSchedulerMapping.remove(id);
+            } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid
+                        // deadlocks
+                lock.unlock();
             }
         }, identifier + ".timeout." + id, Instant.now().plusMillis(delay));
         idSchedulerMapping.put(id, future);
@@ -160,8 +169,12 @@ public long setInterval(Runnable callback, Long delay) {
     public long setInterval(Runnable callback, Long delay, Object... args) {
         long id = lastId.incrementAndGet();
         ScheduledCompletableFuture<Object> future = scheduler.schedule(() -> {
-            synchronized (lock) {
+            try {
+                lock.lock();
                 callback.run();
+            } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid
+                        // deadlocks
+                lock.unlock();
             }
         }, identifier + ".interval." + id, new LoopingAdjuster(Duration.ofMillis(delay)));
         idSchedulerMapping.put(id, future);

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeWrappingScriptedAutomationManagerDelegate.java

@@ -13,6 +13,8 @@
 
 package org.openhab.automation.jsscripting.internal.threading;
 
+import java.util.concurrent.locks.Lock;
+
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.openhab.core.automation.Rule;
 import org.openhab.core.automation.module.script.rulesupport.shared.ScriptedAutomationManager;
@@ -31,15 +33,16 @@
  * instance of this class that they are registered with.
  *
  * @author Jonathan Gilbert - Initial contribution
- * @author Florian Hotze - Pass in lock object for multi-thread synchronization
+ * @author Florian Hotze - Pass in lock object for multi-thread synchronization; Switch to {@link Lock} for multi-thread
+ *         synchronization
  */
 @NonNullByDefault
 public class ThreadsafeWrappingScriptedAutomationManagerDelegate {
 
     private ScriptedAutomationManager delegate;
-    private final Object lock;
+    private final Lock lock;
 
-    public ThreadsafeWrappingScriptedAutomationManagerDelegate(ScriptedAutomationManager delegate, Object lock) {
+    public ThreadsafeWrappingScriptedAutomationManagerDelegate(ScriptedAutomationManager delegate, Lock lock) {
         this.delegate = delegate;
         this.lock = lock;
     }