[jsscripting] Synchronize context access in logger initialization (#1…

…7496) * [jsscripting] Synchronize context access in logger initialisation to avoid illegal multi-thread access Fixes #17494. Signed-off-by: Florian Hotze <[email protected]>
openhab · Oct 2, 2024 · 856d526 · 856d526
1 parent 82ba5da
commit 856d526
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 4 deletions.
diff --git a/...src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java b/...src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java
@@ -15,6 +15,7 @@
 import static org.openhab.core.automation.module.script.ScriptTransformationService.OPENHAB_TRANSFORMATION_SCRIPT;
 
 import java.util.Arrays;
+import java.util.concurrent.locks.Lock;
 import java.util.stream.Collectors;
 
 import javax.script.Compilable;
@@ -34,7 +35,7 @@
  * @author Jonathan Gilbert - Initial contribution
  * @author Florian Hotze - Improve logger name, Fix memory leak caused by exception logging
  */
-class DebuggingGraalScriptEngine<T extends ScriptEngine & Invocable & AutoCloseable & Compilable>
+class DebuggingGraalScriptEngine<T extends ScriptEngine & Invocable & AutoCloseable & Compilable & Lock>
         extends InvocationInterceptingScriptEngineWithInvocableAndCompilableAndAutoCloseable<T> {
 
     private static final int STACK_TRACE_LENGTH = 5;
@@ -48,8 +49,18 @@ public DebuggingGraalScriptEngine(T delegate) {
     @Override
     protected void beforeInvocation() {
         super.beforeInvocation();
-        if (logger == null) {
-            initializeLogger();
+        // OpenhabGraalJSScriptEngine::beforeInvocation will be executed after
+        // DebuggingGraalScriptEngine::beforeInvocation, because GraalJSScriptEngineFactory::createScriptEngine returns
+        // a DebuggingGraalScriptEngine instance.
+        // We therefore need to synchronize logger setup here and cannot rely on the synchronization in
+        // OpenhabGraalJSScriptEngine.
+        delegate.lock();
+        try {
+            if (logger == null) {
+                initializeLogger();
+            }
+        } finally { // Make sure that Lock is unlocked regardless of an exception being thrown or not to avoid deadlocks
+            delegate.unlock();
         }
     }
 

diff --git a/...src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java b/...src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java
@@ -32,6 +32,8 @@
 import java.time.ZonedDateTime;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
@@ -69,7 +71,8 @@
  *         {@link Lock} for multi-thread synchronization; globals and openhab-js injection code caching
  */
 public class OpenhabGraalJSScriptEngine
-        extends InvocationInterceptingScriptEngineWithInvocableAndCompilableAndAutoCloseable<GraalJSScriptEngine> {
+        extends InvocationInterceptingScriptEngineWithInvocableAndCompilableAndAutoCloseable<GraalJSScriptEngine>
+        implements Lock {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(OpenhabGraalJSScriptEngine.class);
     private static final Source GLOBAL_SOURCE;
@@ -346,4 +349,34 @@ private static Reader getFileAsReader(String fileName) throws IOException {
 
         return new InputStreamReader(ioStream);
     }
+
+    @Override
+    public void lock() {
+        lock.lock();
+    }
+
+    @Override
+    public void lockInterruptibly() throws InterruptedException {
+        lock.lockInterruptibly();
+    }
+
+    @Override
+    public boolean tryLock() {
+        return lock.tryLock();
+    }
+
+    @Override
+    public boolean tryLock(long l, TimeUnit timeUnit) throws InterruptedException {
+        return lock.tryLock(l, timeUnit);
+    }
+
+    @Override
+    public void unlock() {
+        lock.unlock();
+    }
+
+    @Override
+    public Condition newCondition() {
+        return lock.newCondition();
+    }
 }