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. + *

+ * 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. + *

+ */ + @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 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. */