[jsscripting] Extend synchronization to common ScriptEngine methods

This extends the multi-thread synchronization to "eval" and "invokeMethod" and moves synchronization for "invokeFunction" to the DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization class. 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/DebuggingGraalScriptEngine.java

@@ -18,7 +18,7 @@
 import javax.script.ScriptException;
 
 import org.graalvm.polyglot.PolyglotException;
-import org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable;
+import org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -28,7 +28,7 @@
  * @author Jonathan Gilbert - Initial contribution
  */
 class DebuggingGraalScriptEngine<T extends ScriptEngine & Invocable & AutoCloseable>
-        extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable<T> {
+        extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization<T> {
 
     private static final Logger STACK_LOGGER = LoggerFactory
             .getLogger("org.openhab.automation.script.javascript.stack");

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

@@ -45,7 +45,7 @@
 import org.openhab.automation.jsscripting.internal.fs.PrefixedSeekableByteChannel;
 import org.openhab.automation.jsscripting.internal.fs.ReadOnlySeekableByteArrayChannel;
 import org.openhab.automation.jsscripting.internal.fs.watch.JSDependencyTracker;
-import org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable;
+import org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization;
 import org.openhab.core.automation.module.script.ScriptExtensionAccessor;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -61,7 +61,7 @@
  *         into the JS context; Fix memory leak caused by HostObject by making HostAccess reference static
  */
 public class OpenhabGraalJSScriptEngine
-        extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable<GraalJSScriptEngine> {
+        extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization<GraalJSScriptEngine> {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(OpenhabGraalJSScriptEngine.class);
     private static final String GLOBAL_REQUIRE = "require(\"@jsscripting-globals\");";
@@ -83,8 +83,6 @@ public class OpenhabGraalJSScriptEngine
                     }, HostAccess.TargetMappingPrecedence.LOW)
             .build();
 
-    /** Shared lock object for synchronization of multi-thread access */
-    private final Object lock = new Object();
     private final JSRuntimeFeatures jsRuntimeFeatures;
 
     // these fields start as null because they are populated on first use
@@ -226,14 +224,6 @@ 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);
-        }
-    }
-
     @Override
     public void close() {
         jsRuntimeFeatures.close();

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseable.java → bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization.java

@@ -26,47 +26,85 @@
 import org.eclipse.jdt.annotation.Nullable;
 
 /**
- * {@link ScriptEngine} implementation that delegates to a supplied ScriptEngine instance. Allows overriding specific
- * methods.
+ * {@link ScriptEngine} implementation that delegates to a supplied ScriptEngine instance and provides multi-thread
+ * synchronization. Allows overriding specific methods.
  *
  * @author Jonathan Gilbert - Initial contribution
  */
-public abstract class DelegatingScriptEngineWithInvocableAndAutocloseable<T extends ScriptEngine & Invocable & AutoCloseable>
+public abstract class DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization<T extends ScriptEngine & Invocable & AutoCloseable>
         implements ScriptEngine, Invocable, AutoCloseable {
     protected T delegate;
+    /** Shared lock object for synchronization of multi-thread access */
+    protected final Object lock = new Object();
 
-    public DelegatingScriptEngineWithInvocableAndAutocloseable(T delegate) {
+    public DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization(T delegate) {
         this.delegate = delegate;
     }
 
     @Override
     public @Nullable Object eval(String s, ScriptContext scriptContext) throws ScriptException {
-        return Objects.nonNull(delegate) ? delegate.eval(s, scriptContext) : null;
+        if (Objects.nonNull(delegate)) {
+            synchronized (lock) {
+                return delegate.eval(s, scriptContext);
+            }
+        } else {
+            return null;
+        }
     }
 
     @Override
     public @Nullable Object eval(Reader reader, ScriptContext scriptContext) throws ScriptException {
-        return Objects.nonNull(delegate) ? delegate.eval(reader, scriptContext) : null;
+        if (Objects.nonNull(delegate)) {
+            synchronized (lock) {
+                return delegate.eval(reader, scriptContext);
+            }
+        } else {
+            return null;
+        }
     }
 
     @Override
     public @Nullable Object eval(String s) throws ScriptException {
-        return Objects.nonNull(delegate) ? delegate.eval(s) : null;
+        if (Objects.nonNull(delegate)) {
+            synchronized (lock) {
+                return delegate.eval(s);
+            }
+        } else {
+            return null;
+        }
     }
 
     @Override
     public @Nullable Object eval(Reader reader) throws ScriptException {
-        return Objects.nonNull(delegate) ? delegate.eval(reader) : null;
+        if (Objects.nonNull(delegate)) {
+            synchronized (lock) {
+                return delegate.eval(reader);
+            }
+        } else {
+            return null;
+        }
     }
 
     @Override
     public @Nullable Object eval(String s, Bindings bindings) throws ScriptException {
-        return Objects.nonNull(delegate) ? delegate.eval(s, bindings) : null;
+        if (Objects.nonNull(delegate)) {
+            synchronized (lock) {
+                return delegate.eval(s, bindings);
+            }
+        } else {
+            return null;
+        }
     }
 
     @Override
     public @Nullable Object eval(Reader reader, Bindings bindings) throws ScriptException {
-        return Objects.nonNull(delegate) ? delegate.eval(reader, bindings) : null;
+        if (Objects.nonNull(delegate)) {
+            synchronized (lock) {
+                return delegate.eval(reader, bindings);
+            }
+        } else {
+            return null;
+        }
     }
 
     @Override
@@ -115,12 +153,24 @@ public void setContext(ScriptContext scriptContext) {
     @Override
     public @Nullable Object invokeMethod(Object o, String s, Object... objects)
             throws ScriptException, NoSuchMethodException {
-        return Objects.nonNull(delegate) ? delegate.invokeMethod(o, s, objects) : null;
+        if (Objects.nonNull(delegate)) {
+            synchronized (lock) {
+                return delegate.invokeMethod(o, s, objects);
+            }
+        } else {
+            return null;
+        }
     }
 
     @Override
     public @Nullable Object invokeFunction(String s, Object... objects) throws ScriptException, NoSuchMethodException {
-        return Objects.nonNull(delegate) ? delegate.invokeFunction(s, objects) : null;
+        if (Objects.nonNull(delegate)) {
+            synchronized (lock) {
+                return delegate.invokeFunction(s, objects);
+            }
+        } else {
+            return null;
+        }
     }
 
     @Override

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

@@ -28,10 +28,10 @@
  * @param <T> The delegate class
  * @author Jonathan Gilbert - Initial contribution
  */
-public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable<T extends ScriptEngine & Invocable & AutoCloseable>
-        extends DelegatingScriptEngineWithInvocableAndAutocloseable<T> {
+public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization<T extends ScriptEngine & Invocable & AutoCloseable>
+        extends DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization<T> {
 
-    public InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable(T delegate) {
+    public InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization(T delegate) {
         super(delegate);
     }