From 1def0a15ed53412af533af017866c595b7b6d092 Mon Sep 17 00:00:00 2001 From: Florian Hotze Date: Wed, 2 Oct 2024 16:04:20 +0200 Subject: [PATCH 1/2] [jsscripting] Synchronize context access in logger initialisation to avoid illegal multi-thread access Fixes #17494. Signed-off-by: Florian Hotze --- .../internal/DebuggingGraalScriptEngine.java | 18 ++++++++-- .../internal/OpenhabGraalJSScriptEngine.java | 35 ++++++++++++++++++- 2 files changed, 49 insertions(+), 4 deletions(-) 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 76f1afb2dd544..09b8824ec4133 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 @@ -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 +class DebuggingGraalScriptEngine extends InvocationInterceptingScriptEngineWithInvocableAndCompilableAndAutoCloseable { private static final int STACK_TRACE_LENGTH = 5; @@ -48,8 +49,19 @@ 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 locker 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 is thrown or not to avoid + // deadlocks + delegate.unlock(); } } 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 cb4dcfd23b58b..94b89666318b0 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 @@ -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 { + extends InvocationInterceptingScriptEngineWithInvocableAndCompilableAndAutoCloseable + 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(); + } } From cefde3645df9a38f1516b634760561d98e36c49c Mon Sep 17 00:00:00 2001 From: Florian Hotze Date: Wed, 2 Oct 2024 19:09:20 +0200 Subject: [PATCH 2/2] Address review Signed-off-by: Florian Hotze --- .../jsscripting/internal/DebuggingGraalScriptEngine.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 09b8824ec4133..8c71899a6880a 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 @@ -52,15 +52,14 @@ protected void beforeInvocation() { // OpenhabGraalJSScriptEngine::beforeInvocation will be executed after // DebuggingGraalScriptEngine::beforeInvocation, because GraalJSScriptEngineFactory::createScriptEngine returns // a DebuggingGraalScriptEngine instance. - // We therefore need to synchronize locker setup here and cannot rely on the synchronization in + // 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 is thrown or not to avoid - // deadlocks + } finally { // Make sure that Lock is unlocked regardless of an exception being thrown or not to avoid deadlocks delegate.unlock(); } }