From 66e8d7dc944eff6096eb337e408b407254785490 Mon Sep 17 00:00:00 2001
From: Florian Hotze <florianh_dev@icloud.com>
Date: Wed, 27 Mar 2024 21:43:07 +0100
Subject: [PATCH] [jsscripting] Fix memory leak on script execution failure
 (#16578)

Make engineIdentifier a instance field to ease debugging.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
---
 .../internal/DebuggingGraalScriptEngine.java  | 24 +++++++++++++++----
 .../internal/OpenhabGraalJSScriptEngine.java  |  4 +++-
 .../suppressions.properties                   |  1 +
 3 files changed, 24 insertions(+), 5 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 6342c6848b..508e9cf26a 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
@@ -12,6 +12,9 @@
  */
 package org.openhab.automation.jsscripting.internal;
 
+import java.util.Arrays;
+import java.util.stream.Collectors;
+
 import javax.script.Invocable;
 import javax.script.ScriptContext;
 import javax.script.ScriptEngine;
@@ -26,11 +29,13 @@ import org.slf4j.LoggerFactory;
  * Wraps ScriptEngines provided by Graal to provide error messages and stack traces for scripts.
  *
  * @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>
         extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable<T> {
 
     private static final String SCRIPT_TRANSFORMATION_ENGINE_IDENTIFIER = "openhab-transformation-script-";
+    private static final int STACK_TRACE_LENGTH = 5;
 
     private @Nullable Logger logger;
 
@@ -49,15 +54,26 @@ class DebuggingGraalScriptEngine<T extends ScriptEngine & Invocable & AutoClosea
     @Override
     public Exception afterThrowsInvocation(Exception e) {
         Throwable cause = e.getCause();
+        // OPS4J Pax Logging holds a reference to the exception, which causes the OpenhabGraalJSScriptEngine to not be
+        // removed from heap by garbage collection and causing a memory leak.
+        // Therefore, don't pass the exceptions itself to the logger, but only their message!
         if (cause instanceof IllegalArgumentException) {
-            logger.error("Failed to execute script:", e);
-        }
-        if (cause instanceof PolyglotException) {
-            logger.error("Failed to execute script:", cause);
+            logger.error("Failed to execute script: {}", stringifyThrowable(cause));
+        } else if (cause instanceof PolyglotException) {
+            logger.error("Failed to execute script: {}", stringifyThrowable(cause));
         }
         return e;
     }
 
+    private String stringifyThrowable(Throwable throwable) {
+        String message = throwable.getMessage();
+        StackTraceElement[] stackTraceElements = throwable.getStackTrace();
+        String stackTrace = Arrays.stream(stackTraceElements).limit(STACK_TRACE_LENGTH)
+                .map(t -> "        at " + t.toString()).collect(Collectors.joining(System.lineSeparator()))
+                + System.lineSeparator() + "        ... " + stackTraceElements.length + " more";
+        return (message != null) ? message + System.lineSeparator() + stackTrace : stackTrace;
+    }
+
     /**
      * Initializes the logger.
      * This cannot be done on script engine creation because the context variables are not yet initialized.
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 ce9450a3a3..e4d324a8a7 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
@@ -135,6 +135,7 @@ public class OpenhabGraalJSScriptEngine
 
     // these fields start as null because they are populated on first use
     private @Nullable Consumer<String> scriptDependencyListener;
+    private String engineIdentifier; // this field is very helpful for debugging, please do not remove it
 
     private boolean initialized = false;
     private final boolean injectionEnabled;
@@ -242,6 +243,7 @@ public class OpenhabGraalJSScriptEngine
         if (localEngineIdentifier == null) {
             throw new IllegalStateException("Failed to retrieve engine identifier from engine bindings");
         }
+        this.engineIdentifier = localEngineIdentifier;
 
         ScriptExtensionAccessor scriptExtensionAccessor = (ScriptExtensionAccessor) ctx
                 .getAttribute(CONTEXT_KEY_EXTENSION_ACCESSOR);
@@ -250,7 +252,7 @@ public class OpenhabGraalJSScriptEngine
         }
 
         Consumer<String> localScriptDependencyListener = (Consumer<String>) ctx
-                .getAttribute("oh.dependency-listener"/* CONTEXT_KEY_DEPENDENCY_LISTENER */);
+                .getAttribute(CONTEXT_KEY_DEPENDENCY_LISTENER);
         if (localScriptDependencyListener == null) {
             LOGGER.warn(
                     "Failed to retrieve script script dependency listener from engine bindings. Script dependency tracking will be disabled.");
diff --git a/bundles/org.openhab.automation.jsscripting/suppressions.properties b/bundles/org.openhab.automation.jsscripting/suppressions.properties
index f57fe0e511..38e9aa4cf1 100644
--- a/bundles/org.openhab.automation.jsscripting/suppressions.properties
+++ b/bundles/org.openhab.automation.jsscripting/suppressions.properties
@@ -1,2 +1,3 @@
 # Please check here how to add suppressions https://maven.apache.org/plugins/maven-pmd-plugin/examples/violation-exclusions.html
+org.openhab.automation.jsscripting.internal.OpenhabGraalJSScriptEngine=UnusedPrivateField
 org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable=AvoidThrowingNullPointerException,AvoidCatchingNPE
-- 
2.47.3