From 7f2dddf1ea2dbe99d98e5e8ab45a11c3f0c6bf93 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Fri, 28 Aug 2026 21:57:47 +0530 Subject: [PATCH 1/2] Add regression test for the ServiceEcaUtil cache race Drives addEcaDefinitions() writers and an evalRules() reader concurrently against the same service/event, using duplicate-equal rules so every write hits the remove-then-add dedup path. Verified this fails against the pre-fix ServiceEcaUtil.java (bf5e6bb4ff) with a ConcurrentModificationException, and passes against the fix from PR #1805. --- .../eca/ServiceEcaUtilConcurrencyTest.groovy | 183 ++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 framework/service/src/test/groovy/org/apache/ofbiz/service/eca/ServiceEcaUtilConcurrencyTest.groovy diff --git a/framework/service/src/test/groovy/org/apache/ofbiz/service/eca/ServiceEcaUtilConcurrencyTest.groovy b/framework/service/src/test/groovy/org/apache/ofbiz/service/eca/ServiceEcaUtilConcurrencyTest.groovy new file mode 100644 index 0000000000..9d33547fe6 --- /dev/null +++ b/framework/service/src/test/groovy/org/apache/ofbiz/service/eca/ServiceEcaUtilConcurrencyTest.groovy @@ -0,0 +1,183 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + *******************************************************************************/ +package org.apache.ofbiz.service.eca + +import org.apache.ofbiz.base.config.GenericConfigException +import org.apache.ofbiz.base.config.ResourceHandler +import org.apache.ofbiz.base.util.UtilXml +import org.junit.jupiter.api.Assertions +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.Timeout +import org.w3c.dom.Document + +import java.util.concurrent.CountDownLatch +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.Future +import java.util.concurrent.ThreadFactory +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference + +/** + * Regression test for the ServiceEcaUtil cache race documented in + * plugins/supporting-docs/2026-08-28-testintegration-runtime-and-eca-race-fix.md + * and fixed by https://github.com/apache/ofbiz-framework/pull/1805. + * + * Before that fix, ServiceEcaUtil#mergeEcaDefinitions() mutated a plain + * HashMap/LinkedList cache with no synchronization at all between writers + * (readConfig()/reloadConfig()/addEcaDefinitions(), racing each other every + * time several ServiceDispatchers are constructed at once) and readers + * (evalRules(), which runs on essentially every service call and + * deliberately stays lock-free). A reader iterating a rule list while a + * writer concurrently did remove()-then-add() on that same LinkedList could + * observe a corrupted node link and throw a NullPointerException out of + * LinkedList$ListItr.next() - exactly the crash seen in 8 unrelated + * testIntegration suites once the artificial ECA startup delay was cut. + * + * This drives addEcaDefinitions() (writers) and evalRules() (a reader) + * against the very same service/event concurrently, using duplicate-equal + * rules so every write does the racy remove-then-add. Against the pre-fix + * code this reliably throws; against the fix (CONFIG_LOCK serializing + * writers, plus ConcurrentHashMap/CopyOnWriteArrayList so the lock-free + * reader tolerates a concurrent writer) it must not. + */ +class ServiceEcaUtilConcurrencyTest { + + private static final int WRITER_THREADS = 8 + private static final int READER_THREADS = 4 + private static final int MERGES_PER_WRITER = 3000 + + @Test + // SEPARATE_THREAD so a genuine hang (e.g. a corrupted, cyclic rule list spinning + // evalRules()/mergeEcaDefinitions() forever instead of throwing) still fails this test at + // the deadline, rather than blocking until someone kills the build. + @Timeout(value = 30, threadMode = Timeout.ThreadMode.SEPARATE_THREAD) + void evalRulesToleratesConcurrentAddEcaDefinitions() { + String serviceName = "raceRegressionTestService${UUID.randomUUID()}" + String eventName = 'raceRegressionTestEvent' + String ruleXml = "" + + // Overrides of a Java interface's accessor methods; they can't be turned into Groovy + // properties without breaking the @Override contract they satisfy. + ResourceHandler handler = new ResourceHandler() { + + @Override + String getLoaderName() { return 'main' } // codenarc-disable GetterMethodCouldBeProperty + + @Override + String getLocation() { return 'ServiceEcaUtilConcurrencyTest-in-memory-secas.xml' } // codenarc-disable GetterMethodCouldBeProperty + + @Override + Document getDocument() { return UtilXml.readXmlDocument(ruleXml, false) } + + @Override + InputStream getStream() { return null } + + @Override + URL getURL() throws GenericConfigException { + // No backing resource for this in-memory handler; addEcaDefinitions() catches this + // and falls back to getLocation(), same as it does for any resource without a URL. + throw new GenericConfigException('no URL for in-memory test resource') + } + + @Override + boolean isFileResource() { return false } + + @Override + String getFullLocation() { return getLocation() } // codenarc-disable GetterMethodCouldBeProperty + + } + + CountDownLatch startLatch = new CountDownLatch(1) + AtomicBoolean stopReaders = new AtomicBoolean(false) + AtomicReference readerFailure = new AtomicReference<>() + AtomicReference writerFailure = new AtomicReference<>() + + List readers = (1..READER_THREADS).collect { + Thread reader = new Thread({ + Map context = [:] + Map result = [:] + startLatch.await() + while (!stopReaders.get()) { + try { + ServiceEcaUtil.evalRules(serviceName, null, eventName, null, context, result, false, false) + // Catching Throwable deliberately: a corrupted list under this exact race can throw + // an Error (e.g. StackOverflowError from a cyclic node) as easily as an Exception. + } catch (Throwable t) { // codenarc-disable CatchThrowable + readerFailure.compareAndSet(null, t) + return + } + } + } as Runnable) + reader.daemon = true + reader + } + readers.each { it.start() } + + // Daemon threads: if a corrupted collection ever makes a writer spin forever instead of + // throwing, shutdownNow() below can't force a tight, non-interruptible loop to stop, so + // non-daemon pool threads would otherwise keep the whole test JVM alive past the timeout. + ThreadFactory daemonThreadFactory = { Runnable r -> + Thread t = Executors.defaultThreadFactory().newThread(r) + t.daemon = true + t + } as ThreadFactory + ExecutorService writers = Executors.newFixedThreadPool(WRITER_THREADS, daemonThreadFactory) + try { + List> futures = (1..WRITER_THREADS).collect { + writers.submit({ + startLatch.await() + for (int j = 0; j < MERGES_PER_WRITER; j++) { + try { + ServiceEcaUtil.addEcaDefinitions(handler) + // See the matching note on the reader's catch above. + } catch (Throwable t) { // codenarc-disable CatchThrowable + writerFailure.compareAndSet(null, t) + return + } + } + } as Runnable) + } + + startLatch.countDown() + futures.each { it.get(20, TimeUnit.SECONDS) } + } finally { + writers.shutdownNow() + writers.awaitTermination(5, TimeUnit.SECONDS) + stopReaders.set(true) + readers.each { it.join(5000) } + } + + if (writerFailure.get() != null) { + Assertions.fail("addEcaDefinitions() threw under concurrent load: ${writerFailure.get()}", writerFailure.get()) + } + if (readerFailure.get() != null) { + Assertions.fail("evalRules() threw while racing concurrent addEcaDefinitions(): ${readerFailure.get()}", readerFailure.get()) + } + + // All WRITER_THREADS * MERGES_PER_WRITER merges loaded content-equal rules for the same + // service/event, so they should have collapsed via the remove-old/add-new dedup path down + // to exactly one rule - not accumulated duplicates and not lost entirely. + List finalRules = ServiceEcaUtil.getServiceEventRules(serviceName, eventName) + Assertions.assertEquals(1, finalRules?.size(), + "Expected exactly one deduplicated rule for ${serviceName}:${eventName}, got: ${finalRules}") + } + +} From ae8f85137209567f2a647c7be50e7e7f22c7efff Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Fri, 28 Aug 2026 23:38:52 +0530 Subject: [PATCH 2/2] Widen ServiceEcaUtilConcurrencyTest timeouts for CI headroom The 20s per-future wait and 30s overall @Timeout were sized against a fast, idle dev machine. The test method took ~12s locally on 10 idle cores with all writers serialized on CONFIG_LOCK, leaving little margin for GitHub Actions' shared/throttled runners, which timed out the check on both build (17) and build (21) in PR #1807 with no corruption or assertion failure logged - a false-positive timeout, not a reproduction of the race. Bump the per-future wait to 90s and the outer timeout to 120s so a slow-but-healthy CI run isn't misreported as a failure, while still catching a genuine hang. The workload (writer/reader thread counts, merges per writer) is unchanged. --- .../service/eca/ServiceEcaUtilConcurrencyTest.groovy | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/framework/service/src/test/groovy/org/apache/ofbiz/service/eca/ServiceEcaUtilConcurrencyTest.groovy b/framework/service/src/test/groovy/org/apache/ofbiz/service/eca/ServiceEcaUtilConcurrencyTest.groovy index 9d33547fe6..3f8ed10e58 100644 --- a/framework/service/src/test/groovy/org/apache/ofbiz/service/eca/ServiceEcaUtilConcurrencyTest.groovy +++ b/framework/service/src/test/groovy/org/apache/ofbiz/service/eca/ServiceEcaUtilConcurrencyTest.groovy @@ -68,7 +68,12 @@ class ServiceEcaUtilConcurrencyTest { // SEPARATE_THREAD so a genuine hang (e.g. a corrupted, cyclic rule list spinning // evalRules()/mergeEcaDefinitions() forever instead of throwing) still fails this test at // the deadline, rather than blocking until someone kills the build. - @Timeout(value = 30, threadMode = Timeout.ThreadMode.SEPARATE_THREAD) + // + // 120s gives generous headroom over CI: all writers serialize on CONFIG_LOCK, so the + // 24 000 addEcaDefinitions() calls are effectively single-threaded XML-parsing work that + // took ~12s on an idle 10-core dev machine - comfortably longer on a shared/throttled + // GitHub Actions runner. This bound exists to catch a genuine hang, not to pace the workload. + @Timeout(value = 120, threadMode = Timeout.ThreadMode.SEPARATE_THREAD) void evalRulesToleratesConcurrentAddEcaDefinitions() { String serviceName = "raceRegressionTestService${UUID.randomUUID()}" String eventName = 'raceRegressionTestEvent' @@ -157,7 +162,9 @@ class ServiceEcaUtilConcurrencyTest { } startLatch.countDown() - futures.each { it.get(20, TimeUnit.SECONDS) } + // See the @Timeout comment above for why 90s: this bound only needs to be well clear + // of the observed ~12s local runtime, with room for slower/shared CI hardware. + futures.each { it.get(90, TimeUnit.SECONDS) } } finally { writers.shutdownNow() writers.awaitTermination(5, TimeUnit.SECONDS)