From 9aefbf541600ae5278f39b13dd894fa2f37e8f2f Mon Sep 17 00:00:00 2001
From: Akash Kumar <116457960+akashchamp@users.noreply.github.com>
Date: Sat, 26 Sep 2026 00:14:46 +0530
Subject: [PATCH] [SCXML-290] Make JSEvaluator.initGlobalsScript volatile

JSEvaluator#initGlobalsScript is a private static String that caches the
contents of the init_global.js classpath resource. It is written exactly
once, inside the per-instance synchronized initEngine() method, but is
read without any synchronization from getScriptContext() (and by every
other JSEvaluator instance's initEngine() null-check).

Because initEngine() synchronizes on the individual JSEvaluator instance
(this), and separate instances are commonly constructed concurrently
(one per SCXML instance), the write to the static field and a read of it
on another instance do not share a common monitor. The JMM therefore does
not guarantee that a thread will observe the script content published by
another thread's initEngine() call; it may observe a stale or null value.

Marking the field volatile establishes the required happens-before edge
between any write and any subsequent read, independent of which instance
or monitor performed the write, fixing the visibility gap directly at its
source.

Also adds a JSEvaluatorTest that constructs and evaluates many JSEvaluator
instances concurrently, exercising the previously-racy initialization path
under real thread contention.

Generated-by: Claude Sonnet 5 -- https://www.apache.org/legal/generative-tooling.html

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 .../scxml2/env/javascript/JSEvaluator.java    |  2 +-
 .../env/javascript/JSEvaluatorTest.java       | 49 +++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/src/main/java/org/apache/commons/scxml2/env/javascript/JSEvaluator.java b/src/main/java/org/apache/commons/scxml2/env/javascript/JSEvaluator.java
index cde6dc763..4810bb188 100644
--- a/src/main/java/org/apache/commons/scxml2/env/javascript/JSEvaluator.java
+++ b/src/main/java/org/apache/commons/scxml2/env/javascript/JSEvaluator.java
@@ -83,7 +83,7 @@ public String getSupportedDatamodel() {
             + "expression, Context must be a org.apache.commons.scxml2.env.javascript.JSContext";
 
     /** Nashorn Global initialization script, loaded from {@code init_global.js} classpath resource */
-    private static String initGlobalsScript;
+    private static volatile String initGlobalsScript;
 
     /** Nashorn ScriptEngine **/
     private transient ScriptEngine engine;
diff --git a/src/test/java/org/apache/commons/scxml2/env/javascript/JSEvaluatorTest.java b/src/test/java/org/apache/commons/scxml2/env/javascript/JSEvaluatorTest.java
index 0eadd07fd..c997661cd 100644
--- a/src/test/java/org/apache/commons/scxml2/env/javascript/JSEvaluatorTest.java
+++ b/src/test/java/org/apache/commons/scxml2/env/javascript/JSEvaluatorTest.java
@@ -24,7 +24,13 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.StringReader;
+import java.util.List;
 import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 
 import org.apache.commons.scxml2.Context;
 import org.apache.commons.scxml2.Evaluator;
@@ -140,6 +146,49 @@ void testBasic() throws SCXMLExpressionException {
         assertTrue((Boolean) evaluator.eval(context, "1+1 == 2"));
     }
 
+    /**
+     * SCXML-290: {@code JSEvaluator#initGlobalsScript} is a static field that is only ever written
+     * inside the per-instance {@code synchronized initEngine()} method, but was read without any
+     * synchronization elsewhere. Since the write and the read do not share a common monitor, the Java
+     * Memory Model does not guarantee that a thread constructing/using a fresh {@link JSEvaluator}
+     * instance will observe the value published by another instance's {@code initEngine()} call.
+     * <p>
+     * This test exercises many {@link JSEvaluator} instances concurrently performing their first
+     * (lazy) engine initialization and evaluation. It cannot deterministically force the JMM
+     * visibility gap to manifest in a single JVM run, but it does exercise the previously-racy
+     * code path under real concurrency and fails loudly (instead of silently passing) if
+     * initialization throws or produces an incorrect result on any thread.
+     * </p>
+     */
+    @Test
+    void testConcurrentEvaluatorInitialization() throws Exception {
+        final int threadCount = 32;
+        final ExecutorService executor = Executors.newFixedThreadPool(threadCount);
+        final CountDownLatch ready = new CountDownLatch(threadCount);
+        final CountDownLatch start = new CountDownLatch(1);
+        final List<Throwable> failures = new CopyOnWriteArrayList<>();
+        try {
+            for (int i = 0; i < threadCount; i++) {
+                executor.submit(() -> {
+                    ready.countDown();
+                    try {
+                        start.await();
+                        final Evaluator concurrentEvaluator = new JSEvaluator();
+                        assertTrue((Boolean) concurrentEvaluator.eval(context, "1+1 == 2"));
+                    } catch (final Throwable t) {
+                        failures.add(t);
+                    }
+                });
+            }
+            assertTrue(ready.await(5, TimeUnit.SECONDS), "Threads failed to start in time");
+            start.countDown();
+        } finally {
+            executor.shutdown();
+            assertTrue(executor.awaitTermination(10, TimeUnit.SECONDS), "Executor did not terminate in time");
+        }
+        assertTrue(failures.isEmpty(), "Concurrent JSEvaluator initialization failed: " + failures);
+    }
+
     /**
      * Tests evaluation with SCXML data model expressions.
      */