Revert "[jsscripting] Don't close the shared GraalJS engine on factory deacti…" (#21054)

This reverts commit a4bef92da4.

Signed-off-by: Florian Hotze <dev@florianhotze.com>

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

@@ -30,7 +30,6 @@ import org.openhab.core.automation.module.script.ScriptDependencyTracker;
 import org.openhab.core.automation.module.script.ScriptEngineFactory;
 import org.openhab.core.config.core.ConfigurableService;
 import org.osgi.framework.Constants;
-import org.osgi.service.component.ComponentConstants;
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
 import org.osgi.service.component.annotations.Deactivate;
@@ -44,7 +43,7 @@ import org.slf4j.event.Level;
  * An implementation of {@link ScriptEngineFactory} with customizations for GraalJS ScriptEngines.
  *
  * @author Jonathan Gilbert - Initial contribution
- * @author Dan Cunningham - Script injections; Scope the shared Engine to the bundle lifetime
+ * @author Dan Cunningham - Script injections
  * @author Florian Hotze - Debugger support
  */
 @Component(service = ScriptEngineFactory.class, configurationPid = "org.openhab.jsscripting", property = Constants.SERVICE_PID
@@ -67,16 +66,10 @@ public class GraalJSScriptEngineFactory implements ScriptEngineFactory {
 
     private final Logger logger = LoggerFactory.getLogger(GraalJSScriptEngineFactory.class);
     private final GraalJSScriptEngineConfiguration configuration;
-
     /**
-     * Shared Polyglot {@link Engine} used by all {@link OpenhabGraalJSScriptEngine} instances.
+     * Shared Polyglot {@link Engine} instance to be used by all instances of {@link OpenhabGraalJSScriptEngine}.
-     * <p>
-     * Kept static and never closed on deactivation. Script contexts created from it can outlive a component restart
-     * (e.g. when an OSGi reference is rebound), and closing the engine would break every loaded rule with
-     * "The Context is already closed". So we create it once and reuse it for the life of the bundle.
      */
-    private static volatile @Nullable Engine engine;
+    private final Engine engine;
-    private static final Object ENGINE_LOCK = new Object();
 
     private final JSScriptServiceUtil jsScriptServiceUtil;
     private final JSDependencyTracker jsDependencyTracker;
@@ -90,53 +83,35 @@ public class GraalJSScriptEngineFactory implements ScriptEngineFactory {
         this.jsScriptServiceUtil = jsScriptServiceUtil;
         this.configuration = new GraalJSScriptEngineConfiguration(config);
 
-        if (getEngine(configuration, logger) == null || getLanguage() == null) {
+        if (configuration.isDebuggerEnabled()) {
+            Engine.Builder engineBuilder = createEngineBuilder();
+            engineBuilder //
+                    .option("inspect", "0.0.0.0:" + configuration.getDebuggerPort()) //
+                    .option("inspect.Suspend", "false") // Don't pause at startup waiting for debugger to attach
+                    .option("inspect.WaitAttached", "false") // Don't block code execution waiting for debugger to
+                                                             // attach
+                    .option("inspect.Secure", "false"); // Disable TLS
+            Engine engine;
+            try {
+                engine = engineBuilder.build();
+            } catch (RuntimeException e) {
+                logger.error(
+                        "Failed to initialize Graal JavaScript engine with debugger support. Continuing without debugger support.",
+                        e);
+                engine = createEngineBuilder().build();
+            }
+            logger.info("Debugger support is enabled for JavaScript Scripting.");
+            this.engine = engine;
+        } else {
+            this.engine = createEngineBuilder().build();
+        }
+
+        if (getLanguage() == null) {
             logger.error(LANG_NOT_INITIALIZED_MSG);
         }
     }
 
-    /**
+    private Engine.Builder createEngineBuilder() {
-     * Creates the shared {@link Engine} on first use and reuses it afterwards. Debugger settings are read once here,
-     * so changing them only takes effect after an openHAB restart.
-     *
-     * @return the shared {@link Engine}, or {@code null} if it could not be created
-     */
-    private static @Nullable Engine getEngine(GraalJSScriptEngineConfiguration configuration, Logger logger) {
-        synchronized (ENGINE_LOCK) {
-            Engine localEngine = engine;
-            if (localEngine != null) {
-                return localEngine;
-            }
-            try {
-                if (configuration.isDebuggerEnabled()) {
-                    Engine.Builder engineBuilder = createEngineBuilder() //
-                            .option("inspect", "0.0.0.0:" + configuration.getDebuggerPort()) //
-                            .option("inspect.Suspend", "false") // Don't pause at startup waiting for debugger to attach
-                            .option("inspect.WaitAttached", "false") // Don't block code execution waiting for debugger
-                                                                     // to attach
-                            .option("inspect.Secure", "false"); // Disable TLS
-                    try {
-                        localEngine = engineBuilder.build();
-                    } catch (RuntimeException e) {
-                        logger.error(
-                                "Failed to initialize Graal JavaScript engine with debugger support. Continuing without debugger support.",
-                                e);
-                        localEngine = createEngineBuilder().build();
-                    }
-                    logger.info("Debugger support is enabled for JavaScript Scripting.");
-                } else {
-                    localEngine = createEngineBuilder().build();
-                }
-            } catch (RuntimeException e) {
-                logger.error("Failed to initialize Graal JavaScript engine.", e);
-                return null;
-            }
-            engine = localEngine;
-            return localEngine;
-        }
-    }
-
-    private static Engine.Builder createEngineBuilder() {
         Logger engineLogger = LoggerFactory
                 .getLogger(GraalJSScriptEngineFactory.class.getPackageName() + ".org.graalvm.polyglot.Engine");
         return Engine.newBuilder().allowExperimentalOptions(true) //
@@ -148,30 +123,9 @@ public class GraalJSScriptEngineFactory implements ScriptEngineFactory {
                 .err(new ThreadLocalSlf4jOutputStream(engineLogger, Level.DEBUG));
     }
 
-    /**
-     * Releases the shared {@link Engine}, but only when the bundle is actually stopping. The reason is checked so that
-     * ordinary component restarts (e.g. an OSGi reference rebind) do not close the engine and break running scripts -
-     * that is exactly what this fix prevents. Only a real bundle stop releases the engine.
-     */
     @Deactivate
-    protected void deactivate(int reason) {
-        if (reason == ComponentConstants.DEACTIVATION_REASON_BUNDLE_STOPPED) {
-            dispose();
-        }
-    }
-
-    /**
-     * Closes the shared {@link Engine} and clears it so it is re-created on next use. Called on bundle stop and from
-     * tests.
-     */
     public void dispose() {
-        synchronized (ENGINE_LOCK) {
+        this.engine.close();
-            Engine localEngine = engine;
-            if (localEngine != null) {
-                localEngine.close();
-                engine = null;
-            }
-        }
     }
 
     @Modified
@@ -194,13 +148,12 @@ public class GraalJSScriptEngineFactory implements ScriptEngineFactory {
         if (!SCRIPT_TYPES.contains(scriptType)) {
             return null;
         }
-        Engine localEngine = getEngine(configuration, logger);
+        if (getLanguage() == null) {
-        if (localEngine == null || getLanguage() == null) {
             logger.error(LANG_NOT_INITIALIZED_MSG);
             return null;
         }
         return new DebuggingGraalScriptEngine<>(
-                new OpenhabGraalJSScriptEngine(configuration, localEngine, jsScriptServiceUtil, jsDependencyTracker));
+                new OpenhabGraalJSScriptEngine(configuration, engine, jsScriptServiceUtil, jsDependencyTracker));
     }
 
     @Override
@@ -214,7 +167,6 @@ public class GraalJSScriptEngineFactory implements ScriptEngineFactory {
      * @return the Graal language of {@link OpenhabGraalJSScriptEngine} or {@code null} if not available
      */
     private @Nullable Language getLanguage() {
-        Engine localEngine = engine;
+        return engine.getLanguages().get(OpenhabGraalJSScriptEngine.LANGUAGE_ID);
-        return localEngine == null ? null : localEngine.getLanguages().get(OpenhabGraalJSScriptEngine.LANGUAGE_ID);
     }
 }