From aadd21e45879c10aad29bf279ddbb0afd789b0aa Mon Sep 17 00:00:00 2001 From: Florian Hotze Date: Mon, 12 Dec 2022 18:48:13 +0100 Subject: [PATCH] [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 https://github.com/openhab/openhab-core/pull/3180. Signed-off-by: Florian Hotze --- .../internal/DebuggingGraalScriptEngine.java | 4 +- .../internal/OpenhabGraalJSScriptEngine.java | 14 +--- ...leAndAutocloseableAndSynchronization.java} | 74 ++++++++++++++++--- ...leAndAutoCloseableAndSynchronization.java} | 6 +- 4 files changed, 69 insertions(+), 29 deletions(-) rename bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/{DelegatingScriptEngineWithInvocableAndAutocloseable.java => DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization.java} (64%) rename bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/{InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.java => InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization.java} (95%) diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java index 4855f02c2f7fd..5aa252b06f66e 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java +++ b/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 - extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable { + extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization { private static final Logger STACK_LOGGER = LoggerFactory .getLogger("org.openhab.automation.script.javascript.stack"); diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java index 92dfd5b7374b6..4fb7e62e43268 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java +++ b/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 { + extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization { 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(); diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseable.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization.java similarity index 64% rename from bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseable.java rename to bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization.java index f96ce621db4d1..c4e11edb242ad 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseable.java +++ b/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 +public abstract class DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization 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 diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization.java similarity index 95% rename from bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.java rename to bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization.java index a538bea09a36a..4bd67da0c66fe 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization.java @@ -28,10 +28,10 @@ * @param The delegate class * @author Jonathan Gilbert - Initial contribution */ -public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable - extends DelegatingScriptEngineWithInvocableAndAutocloseable { +public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization + extends DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization { - public InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable(T delegate) { + public InvocationInterceptingScriptEngineWithInvocableAndAutoCloseableAndSynchronization(T delegate) { super(delegate); }