This is an automated email from the ASF dual-hosted git repository.
ashishvijaywargiya pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new 485496ad3e Cut testIntegration runtime(from ~86s to ~57s) and fix a
service ECA cache race (#1805)
485496ad3e is described below
commit 485496ad3e1aca1775c191b3b884a001c82804e0
Author: Ashish Vijaywargiya <[email protected]>
AuthorDate: Fri Aug 28 19:47:32 2026 +0530
Cut testIntegration runtime(from ~86s to ~57s) and fix a service ECA cache
race (#1805)
---
.../apache/ofbiz/entity/test/EntityTestSuite.java | 13 ++++--
framework/service/servicedef/secas_test_se.xml | 17 +++++--
.../apache/ofbiz/service/eca/ServiceEcaUtil.java | 53 ++++++++++++++++++----
3 files changed, 68 insertions(+), 15 deletions(-)
diff --git
a/framework/entity/src/test/java/org/apache/ofbiz/entity/test/EntityTestSuite.java
b/framework/entity/src/test/java/org/apache/ofbiz/entity/test/EntityTestSuite.java
index 6cdeb5823f..44c7f74466 100644
---
a/framework/entity/src/test/java/org/apache/ofbiz/entity/test/EntityTestSuite.java
+++
b/framework/entity/src/test/java/org/apache/ofbiz/entity/test/EntityTestSuite.java
@@ -951,9 +951,12 @@ public class EntityTestSuite implements JupiterTestHelper {
GenericTransactionException caught = null;
try {
GenericValue testValue = delegator.makeValue("Testing",
"testingId", "timeout-test");
- boolean transBegin = TransactionUtil.begin(10); // timeout set to
10 seconds
+ // Timeout and sleep only need to keep the same 2x margin the
timeout is exceeded by;
+ // shrinking both by the same factor (was 10s/20s) keeps that
margin while cutting
+ // this test's real wall-clock cost, which used to dominate the
testIntegration run.
+ boolean transBegin = TransactionUtil.begin(2); // timeout set to 2
seconds
delegator.create(testValue);
- Thread.sleep(20 * 1000);
+ Thread.sleep(4 * 1000);
TransactionUtil.commit(transBegin);
} catch (GenericTransactionException e) {
caught = e;
@@ -974,9 +977,11 @@ public class EntityTestSuite implements JupiterTestHelper {
try {
GenericValue testValue = delegator.makeValue("Testing",
"testingId", "timeout-test");
boolean transBegin = TransactionUtil.begin();
- TransactionUtil.setTransactionTimeout(20); // now set timeout to
20 seconds
+ // Same 2x-under-the-timeout margin as before (was 20s/10s),
scaled down for the same
+ // reason as testTransactionUtilMoreThanTimeout above.
+ TransactionUtil.setTransactionTimeout(4); // now set timeout to 4
seconds
delegator.create(testValue);
- Thread.sleep(10 * 1000);
+ Thread.sleep(2 * 1000);
TransactionUtil.commit(transBegin);
} finally {
delegator.removeByAnd("Testing", "testingId", "timeout-test");
diff --git a/framework/service/servicedef/secas_test_se.xml
b/framework/service/servicedef/secas_test_se.xml
index be428bcac5..7bc52ce010 100644
--- a/framework/service/servicedef/secas_test_se.xml
+++ b/framework/service/servicedef/secas_test_se.xml
@@ -27,10 +27,21 @@ under the License.
<action service="testServiceEcaGlobalEventExecOnRollback" mode="sync"/>
</eca>
- <!-- Note: This eca is used only to allow time for the global ecas above
to
- complete before the xml assertion test is run -->
+ <!-- Note: This eca is used only to allow time for the global ecas above to
+ complete before the xml assertion test is run. The global
commit/rollback listeners
+ above run async off the transaction's afterCompletion callback and
typically finish
+ in a few ms, so 500ms is a wide margin - it used to be 5000ms,
which by itself was a
+ large share of the whole testIntegration run.
+ This used to be unsafe to shorten: doing so let the main thread
reach WorkEffortTests
+ while those listener threads were still in flight, and their
access to ServiceEcaUtil's
+ then non-thread-safe ECA-rules cache raced with createWorkEffort's
own rule lookup on
+ the main thread, throwing a NullPointerException from
java.util.LinkedList$ListItr.next()
+ inside ServiceEcaUtil.evalRules. That race is fixed at the source
in ServiceEcaUtil.java
+ (CONFIG_LOCK plus ConcurrentHashMap/CopyOnWriteArrayList) rather
than by waiting it out
+ here - see that file's CONFIG_LOCK javadoc for the full mechanism
- and this value is
+ verified safe at 500ms across repeated testIntegration runs. -->
<eca service="testServiceEcaGlobalEventExec" event="return">
- <set field-name="duration" value="5000" format="long"/>
+ <set field-name="duration" value="500" format="long"/>
<action service="blockingTestScv" mode="sync"/>
</eca>
</service-eca>
diff --git
a/framework/service/src/main/java/org/apache/ofbiz/service/eca/ServiceEcaUtil.java
b/framework/service/src/main/java/org/apache/ofbiz/service/eca/ServiceEcaUtil.java
index 8b6e4b8133..f45e741531 100644
---
a/framework/service/src/main/java/org/apache/ofbiz/service/eca/ServiceEcaUtil.java
+++
b/framework/service/src/main/java/org/apache/ofbiz/service/eca/ServiceEcaUtil.java
@@ -19,7 +19,6 @@
package org.apache.ofbiz.service.eca;
import java.util.Collection;
-import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
@@ -27,6 +26,7 @@ import java.util.Set;
import java.util.TreeSet;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.Future;
import org.apache.ofbiz.base.component.ComponentConfig;
@@ -54,19 +54,49 @@ public final class ServiceEcaUtil {
// new UtilCache("service.ServiceECAs", 0, 0, false);
private static Map<String, Map<String, List<ServiceEcaRule>>> ecaCache =
new ConcurrentHashMap<>();
+ // Guards every write to ecaCache
(readConfig/reloadConfig/addEcaDefinitions, via mergeEcaDefinitions).
+ // ServiceDispatcher's constructor calls readConfig() unconditionally on
every single dispatcher it
+ // creates, with no coordination between callers; every test suite creates
its own dispatcher(s), so
+ // dozens of ServiceDispatcher constructions can land within the same few
milliseconds at startup,
+ // all on different threads. Before this lock, readConfig()'s "if
(isNotEmpty(ecaCache)) return;"
+ // guard was a plain check-then-act race: several of those threads could
all observe an empty cache
+ // at once and all proceed to rebuild it concurrently.
mergeEcaDefinitions() mutates ecaCache's
+ // per-service HashMap and per-event LinkedList in place (remove-then-add,
and HashMap.put on a
+ // shared eventMap instance); with two threads doing that at once on the
same List/Map, a third
+ // thread concurrently iterating that same List in evalRules() (any
in-flight service call
+ // evaluating its ECA rules, which happens on essentially every service
invocation) can observe a
+ // corrupted node link and throw a NullPointerException out of
LinkedList$ListItr.next() - confirmed
+ // in a testIntegration run where one secas.xml file was independently
reloaded 4 times, on 4
+ // different threads, within a ~300ms startup window, racing a live
evalRules() call and crashing it.
+ private static final Object CONFIG_LOCK = new Object();
+
private ServiceEcaUtil() { }
public static void reloadConfig() {
- ecaCache.clear();
- readConfig();
+ synchronized (CONFIG_LOCK) {
+ ecaCache.clear();
+ readConfigInternal();
+ }
}
public static void readConfig() {
- // Only proceed if the cache hasn't already been populated, caller
should be using reloadConfig() in that situation
+ // Fast path: avoid the lock once startup has finished and every
caller hits this on every
+ // dispatcher construction. Safe to read unlocked - ecaCache is a
ConcurrentHashMap and this is
+ // only ever used to decide whether to (re)acquire the lock and check
again below.
if (UtilValidate.isNotEmpty(ecaCache)) {
return;
}
+ synchronized (CONFIG_LOCK) {
+ // Re-check under the lock: another thread may have already
populated ecaCache while this
+ // one was waiting to enter this block, in which case there is
nothing left to do.
+ if (UtilValidate.isNotEmpty(ecaCache)) {
+ return;
+ }
+ readConfigInternal();
+ }
+ }
+ private static void readConfigInternal() {
List<Future<List<ServiceEcaRule>>> futures = new LinkedList<>();
List<ServiceEcas> serviceEcasList = null;
try {
@@ -98,7 +128,9 @@ public final class ServiceEcaUtil {
public static void addEcaDefinitions(ResourceHandler handler) {
List<ServiceEcaRule> handlerRules = getEcaDefinitions(handler);
- mergeEcaDefinitions(handlerRules);
+ synchronized (CONFIG_LOCK) {
+ mergeEcaDefinitions(handlerRules);
+ }
}
private static List<ServiceEcaRule> getEcaDefinitions(ResourceHandler
handler) {
@@ -134,14 +166,19 @@ public final class ServiceEcaUtil {
List<ServiceEcaRule> rules = null;
if (eventMap == null) {
- eventMap = new HashMap<>();
- rules = new LinkedList<>();
+ // ConcurrentHashMap/CopyOnWriteArrayList, not
HashMap/LinkedList: evalRules() below
+ // reads these without taking CONFIG_LOCK (it runs on
essentially every service call,
+ // so it can't pay lock overhead the way the rare writes above
can), so the collections
+ // themselves - not just serializing the writers against each
other - have to tolerate a
+ // reader iterating one of these while CONFIG_LOCK's holder
concurrently mutates it.
+ eventMap = new ConcurrentHashMap<>();
+ rules = new CopyOnWriteArrayList<>();
ecaCache.put(serviceName, eventMap);
eventMap.put(eventName, rules);
} else {
rules = eventMap.get(eventName);
if (rules == null) {
- rules = new LinkedList<>();
+ rules = new CopyOnWriteArrayList<>();
eventMap.put(eventName, rules);
}
}
