This is an automated email from the ASF dual-hosted git repository.
tkobayas pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/incubator-kie-drools.git
The following commit(s) were added to refs/heads/main by this push:
new d617a0dcec [incubator-kie-drools-6445] Problem in calling
RuleUnitProviderImpl l… (#6520)
d617a0dcec is described below
commit d617a0dcecd768fd88f0e707da7853ad0332ada5
Author: Toshiya Kobayashi <[email protected]>
AuthorDate: Fri Nov 21 19:05:09 2025 +0900
[incubator-kie-drools-6445] Problem in calling RuleUnitProviderImpl l…
(#6520)
* [incubator-kie-drools-6445] Problem in calling RuleUnitProviderImpl
loadRuleUnits from a multithreaded context
* Add comments for double-get-check and suppress warning for unchecked cast
* replace synchronized with ReentrantLock
---
.../ruleunits/impl/RuleUnitProviderImpl.java | 60 +++++++++++-----
.../impl/RuleUnitProviderImplConcurrencyTest.java | 80 ++++++++++++++++++++++
drools-ruleunits/pom.xml | 28 ++++++++
3 files changed, 150 insertions(+), 18 deletions(-)
diff --git
a/drools-ruleunits/drools-ruleunits-impl/src/main/java/org/drools/ruleunits/impl/RuleUnitProviderImpl.java
b/drools-ruleunits/drools-ruleunits-impl/src/main/java/org/drools/ruleunits/impl/RuleUnitProviderImpl.java
index e464ed95d8..1c38af11a4 100644
---
a/drools-ruleunits/drools-ruleunits-impl/src/main/java/org/drools/ruleunits/impl/RuleUnitProviderImpl.java
+++
b/drools-ruleunits/drools-ruleunits-impl/src/main/java/org/drools/ruleunits/impl/RuleUnitProviderImpl.java
@@ -32,6 +32,10 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.ServiceLoader;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import java.util.stream.Collectors;
@@ -66,21 +70,36 @@ public class RuleUnitProviderImpl implements
RuleUnitProvider {
private static final boolean USE_EXEC_MODEL = true;
- private final Map<String, RuleUnit> ruleUnitMap;
+ private final ConcurrentMap<String, RuleUnit> ruleUnitMap;
+ private final Lock generationLock = new ReentrantLock();
public RuleUnitProviderImpl() {
- this.ruleUnitMap =
loadRuleUnits(Thread.currentThread().getContextClassLoader());
+ this.ruleUnitMap = new
ConcurrentHashMap<>(loadRuleUnits(Thread.currentThread().getContextClassLoader()));
}
@Override
+ @SuppressWarnings("unchecked") // safe because ruleUnitMap keys are the
canonical names of their matching RuleUnitData types
public <T extends RuleUnitData> RuleUnit<T> getRuleUnit(T ruleUnitData) {
String ruleUnitName = getRuleUnitName(ruleUnitData);
- RuleUnit<T> ruleUnit = ruleUnitMap.get(ruleUnitName);
+ RuleUnit<T> ruleUnit = (RuleUnit<T>) ruleUnitMap.get(ruleUnitName);
if (ruleUnit != null) {
return ruleUnit;
}
- ruleUnitMap.putAll(generateRuleUnit(ruleUnitData));
- return ruleUnitMap.get(ruleUnitName);
+ // double-check inside the lock so concurrent cache misses don't
re-generate the same unit twice.
+ // computeIfAbsent cannot express this because invalidateRuleUnits may
remove entries while another thread is computing;
+ // we need the whole read-check-generate-publish sequence under one
lock.
+ generationLock.lock();
+ try {
+ ruleUnit = (RuleUnit<T>) ruleUnitMap.get(ruleUnitName);
+ if (ruleUnit == null) {
+ Map<String, RuleUnit> generated =
generateRuleUnit(ruleUnitData);
+ ruleUnitMap.putAll(generated);
+ ruleUnit = (RuleUnit<T>) generated.get(ruleUnitName);
+ }
+ } finally {
+ generationLock.unlock();
+ }
+ return ruleUnit;
}
protected <T extends RuleUnitData> Map<String, RuleUnit>
generateRuleUnit(T ruleUnitData) {
@@ -249,19 +268,24 @@ public class RuleUnitProviderImpl implements
RuleUnitProvider {
@Override
public <T extends RuleUnitData> int invalidateRuleUnits(Class<T>
ruleUnitDataClass) {
- if (NamedRuleUnitData.class.isAssignableFrom(ruleUnitDataClass)) {
- // NamedRuleUnitData may create multiple RuleUnits
- List<String> invalidateKeys = ruleUnitMap.entrySet()
- .stream()
- .filter(entry ->
hasSameRuleUnitDataClass(entry.getValue(), ruleUnitDataClass))
- .map(Map.Entry::getKey)
- .collect(Collectors.toList());
- invalidateKeys.forEach(ruleUnitMap::remove);
- return invalidateKeys.size();
- } else {
- String ruleUnitName = getRuleUnitName(ruleUnitDataClass);
- RuleUnit remove = ruleUnitMap.remove(ruleUnitName);
- return remove == null ? 0 : 1;
+ generationLock.lock();
+ try {
+ if (NamedRuleUnitData.class.isAssignableFrom(ruleUnitDataClass)) {
+ // NamedRuleUnitData may create multiple RuleUnits
+ List<String> invalidateKeys = ruleUnitMap.entrySet()
+ .stream()
+ .filter(entry ->
hasSameRuleUnitDataClass(entry.getValue(), ruleUnitDataClass))
+ .map(Map.Entry::getKey)
+ .collect(Collectors.toList());
+ invalidateKeys.forEach(ruleUnitMap::remove);
+ return invalidateKeys.size();
+ } else {
+ String ruleUnitName = getRuleUnitName(ruleUnitDataClass);
+ RuleUnit remove = ruleUnitMap.remove(ruleUnitName);
+ return remove == null ? 0 : 1;
+ }
+ } finally {
+ generationLock.unlock();
}
}
diff --git
a/drools-ruleunits/drools-ruleunits-impl/src/test/java/org/drools/ruleunits/impl/RuleUnitProviderImplConcurrencyTest.java
b/drools-ruleunits/drools-ruleunits-impl/src/test/java/org/drools/ruleunits/impl/RuleUnitProviderImplConcurrencyTest.java
new file mode 100644
index 0000000000..49d543f8e5
--- /dev/null
+++
b/drools-ruleunits/drools-ruleunits-impl/src/test/java/org/drools/ruleunits/impl/RuleUnitProviderImplConcurrencyTest.java
@@ -0,0 +1,80 @@
+/*
+ * 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.drools.ruleunits.impl;
+
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import org.drools.ruleunits.api.RuleUnit;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.EnabledIfSystemProperty;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class RuleUnitProviderImplConcurrencyTest {
+
+ @EnabledIfSystemProperty(named = "runTurtleTests", matches = "true")
+ @Test
+ void loadRuleUnitsFromMultipleThreads() throws InterruptedException {
+ // https://github.com/apache/incubator-kie-drools/issues/6445
+ for (int n = 0; n < 100; n++) {
+ loadRuleUnitsFromMultipleThreadsOnce();
+ }
+ }
+
+ private void loadRuleUnitsFromMultipleThreadsOnce() throws
InterruptedException {
+ RuleUnitProviderImpl provider = new RuleUnitProviderImpl();
+ provider.invalidateRuleUnits(HelloWorldUnit.class);
+
+ int threadCount = 3; // too many threads don't seem to trigger the
issue more often
+ ExecutorService executorService =
Executors.newFixedThreadPool(threadCount);
+ CyclicBarrier startBarrier = new CyclicBarrier(threadCount);
+ Queue<Throwable> errors = new ConcurrentLinkedQueue<>();
+
+ try {
+ for (int i = 0; i < threadCount; i++) {
+ executorService.submit(() -> {
+ try {
+ startBarrier.await();
+ RuleUnit<HelloWorldUnit> ruleUnit =
provider.getRuleUnit(new HelloWorldUnit());
+ assertThat(ruleUnit).isNotNull();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ errors.add(e);
+ } catch (Throwable t) {
+ errors.add(t);
+ }
+ });
+ }
+
+ executorService.shutdown();
+ assertThat(executorService.awaitTermination(30,
TimeUnit.SECONDS)).isTrue();
+ } finally {
+ executorService.shutdownNow();
+ }
+
+ assertThat(errors)
+ .withFailMessage(() -> "Unexpected errors while loading rule
units concurrently: " + errors)
+ .isEmpty();
+ }
+}
diff --git a/drools-ruleunits/pom.xml b/drools-ruleunits/pom.xml
index 587640b825..390d32d634 100644
--- a/drools-ruleunits/pom.xml
+++ b/drools-ruleunits/pom.xml
@@ -59,4 +59,32 @@
<scope>test</scope>
</dependency>
</dependencies>
+
+ <profiles>
+ <profile>
+ <id>runTurtleTests</id>
+ <activation>
+ <property>
+ <name>runTurtleTests</name>
+ </property>
+ </activation>
+ <build>
+ <pluginManagement>
+ <plugins>
+ <plugin>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <systemProperties>
+ <property>
+ <name>runTurtleTests</name>
+ <value>true</value>
+ </property>
+ </systemProperties>
+ </configuration>
+ </plugin>
+ </plugins>
+ </pluginManagement>
+ </build>
+ </profile>
+ </profiles>
</project>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
