[jsscripting] Extend synchronization to common ScriptEngine methods (o…

…penhab#13924)

* [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.

* Revert "[jsscripting] Extend synchronization to common ScriptEngine methods"

This reverts commit aadd21e.

* [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.

* [jsscripting] Reduce compiler warnings
* [jsscripting] Replace finally blocks & Wrap returns in afterInvocation
* [jsscripting] Fix deadlock caused by NoSuchMethodException in Invocable interface methods

During testing my latest changes, I noticed that there is a deadlock when invokeFunction or invokeMethod are called on a non-existing method.
This happens because the NoSuchMethodException keeps afterInvocation from running and therefore the lock never gets released.

* [jsscripting] Also rethrow NPE & Fix PMD warnings/errors
* [jsscripting] Wrap and rethrow other exceptions instead of returning them
* [jsscripting] Address review comment from @jpg0

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>

bundles/org.openhab.automation.jsscripting/pom.xml

@@ -101,6 +101,13 @@
  </execution>
  </executions>
  </plugin>
+ <plugin>
+ <groupId>org.openhab.tools.sat</groupId>
+ <artifactId>sat-plugin</artifactId>
+ <configuration>
+ <pmdFilter>${project.basedir}/suppressions.properties</pmdFilter>
+ </configuration>
+ </plugin>
  </plugins>
  </build>
 

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

@@ -15,7 +15,6 @@
 
 import javax.script.Invocable;
 import javax.script.ScriptEngine;
-import javax.script.ScriptException;
 
 import org.graalvm.polyglot.PolyglotException;
 import org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable;
@@ -38,11 +37,11 @@ public DebuggingGraalScriptEngine(T delegate) {
  }
 
  @Override
- public ScriptException afterThrowsInvocation(ScriptException se) {
- Throwable cause = se.getCause();
+ public Exception afterThrowsInvocation(Exception e) {
+ Throwable cause = e.getCause();
  if (cause instanceof PolyglotException) {
  STACK_LOGGER.error("Failed to execute script:", cause);
  }
- return se;
+ return e;
  }
 }

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

@@ -17,6 +17,7 @@
 
 import javax.script.ScriptEngine;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.automation.jsscripting.internal.fs.watch.JSDependencyTracker;
 import org.openhab.core.automation.module.script.ScriptDependencyTracker;
@@ -37,6 +38,7 @@
 @Component(service = ScriptEngineFactory.class, configurationPid = "org.openhab.jsscripting", property = Constants.SERVICE_PID
  + "=org.openhab.jsscripting")
 @ConfigurableService(category = "automation", label = "JS Scripting", description_uri = "automation:jsscripting")
+@NonNullByDefault
 public final class GraalJSScriptEngineFactory implements ScriptEngineFactory {
  private static final String CFG_INJECTION_ENABLED = "injectionEnabled";
  private static final String INJECTION_CODE = "Object.assign(this, require('openhab'));";
@@ -80,7 +82,7 @@ public void scopeValues(ScriptEngine scriptEngine, Map<String, Object> scopeValu
  }
 
  @Override
- public ScriptEngine createScriptEngine(String scriptType) {
+ public @Nullable ScriptEngine createScriptEngine(String scriptType) {
  return new DebuggingGraalScriptEngine<>(
  new OpenhabGraalJSScriptEngine(injectionEnabled ? INJECTION_CODE : null, jsScriptServiceUtil));
  }

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> {
@@ -83,13 +86,13 @@ public class OpenhabGraalJSScriptEngine
  }, HostAccess.TargetMappingPrecedence.LOW)
  .build();
 
- /** Shared lock object for synchronization of multi-thread access */
- private final Object lock = new Object();
+ /** {@link Lock} synchronization of multi-thread access */
+ private final Lock lock = new ReentrantLock();
  private final JSRuntimeFeatures jsRuntimeFeatures;
 
  // these fields start as null because they are populated on first use
  private String engineIdentifier;
- private Consumer<String> scriptDependencyListener;
+ private @Nullable Consumer<String> scriptDependencyListener;
 
  private boolean initialized = false;
  private final String globalScript;
@@ -119,8 +122,9 @@ public OpenhabGraalJSScriptEngine(@Nullable String injectionCode, JSScriptServic
  @Override
  public SeekableByteChannel newByteChannel(Path path, Set<? extends OpenOption> options,
  FileAttribute<?>... attrs) throws IOException {
- if (scriptDependencyListener != null) {
- scriptDependencyListener.accept(path.toString());
+ Consumer<String> localScriptDependencyListener = scriptDependencyListener;
+ if (localScriptDependencyListener != null) {
+ localScriptDependencyListener.accept(path.toString());
  }
 
  if (path.toString().endsWith(".js")) {
@@ -174,6 +178,8 @@ public Path toRealPath(Path path, LinkOption... linkOptions) throws IOException
 
  @Override
  protected void beforeInvocation() {
+ lock.lock();
+
  if (initialized) {
  return;
  }
@@ -227,11 +233,15 @@ 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 Object afterInvocation(Object obj) {
+ lock.unlock();
+ return obj;
+ }
+
+ @Override
+ protected Exception afterThrowsInvocation(Exception e) {
+ lock.unlock();
+ return e;
  }
 
  @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/scope/AbstractScriptExtensionProvider.java

@@ -13,7 +13,11 @@
 
 package org.openhab.automation.jsscripting.internal.scope;
 
-import java.util.*;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Function;
 
@@ -28,7 +32,7 @@
  * @author Jonathan Gilbert - Initial contribution
  */
 public abstract class AbstractScriptExtensionProvider implements ScriptExtensionProvider {
- private Map<String, Function<String, Object>> types;
+ private Map<String, Function<String, Object>> types = new HashMap<>();
  private Map<String, Map<String, Object>> idToTypes = new ConcurrentHashMap<>();
 
  protected abstract String getPresetName();
@@ -41,7 +45,7 @@ protected void addType(String name, Function<String, Object> value) {
 
  @Activate
  public void activate(final BundleContext context) {
- types = new HashMap<>();
+ types.clear();
  initializeTypes(context);
  }
 

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/ScriptDisposalAwareScriptExtensionProvider.java

@@ -13,7 +13,11 @@
 
 package org.openhab.automation.jsscripting.internal.scope;
 
-import java.util.*;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Function;