This is an automated email from the ASF dual-hosted git repository.
mariofusco 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 1ab862d3aa [incubator-kie-drools-5974] Possible deadlock with
KieRepositoryImpl$KieModuleRepo and KieRepositoryScannerImpl (#6006)
1ab862d3aa is described below
commit 1ab862d3aa1ed426c22ed8ff5ce55e6b6192f525
Author: Toshiya Kobayashi <[email protected]>
AuthorDate: Mon Jul 8 17:50:28 2024 +0900
[incubator-kie-drools-5974] Possible deadlock with
KieRepositoryImpl$KieModuleRepo and KieRepositoryScannerImpl (#6006)
---
.../kie/builder/impl/KieRepositoryImpl.java | 176 +++++++++++++--------
.../kie/builder/impl/KieModuleRepoTest.java | 12 +-
kie-ci/pom.xml | 21 ++-
.../scanner/concurrent/ConcurrentBuildTest.java | 118 ++++++++++++++
4 files changed, 257 insertions(+), 70 deletions(-)
diff --git
a/drools-compiler/src/main/java/org/drools/compiler/kie/builder/impl/KieRepositoryImpl.java
b/drools-compiler/src/main/java/org/drools/compiler/kie/builder/impl/KieRepositoryImpl.java
index cd20a202a5..104beb7325 100644
---
a/drools-compiler/src/main/java/org/drools/compiler/kie/builder/impl/KieRepositoryImpl.java
+++
b/drools-compiler/src/main/java/org/drools/compiler/kie/builder/impl/KieRepositoryImpl.java
@@ -30,6 +30,7 @@ import java.util.Map;
import java.util.NavigableMap;
import java.util.TreeMap;
import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.ReentrantLock;
import org.drools.compiler.compiler.io.memory.MemoryFileSystem;
import org.drools.compiler.kproject.models.KieModuleModelImpl;
@@ -70,6 +71,8 @@ public class KieRepositoryImpl
private final KieModuleRepo kieModuleRepo;
+ private final ReentrantLock lock = new ReentrantLock();
+
public static void setInternalKieScanner(InternalKieScanner scanner) {
synchronized (KieScannerHolder.class) {
KieScannerHolder.kieScanner = scanner;
@@ -98,7 +101,7 @@ public class KieRepositoryImpl
}
public KieRepositoryImpl() {
- kieModuleRepo = new KieModuleRepo();
+ kieModuleRepo = new KieModuleRepo(lock);
}
public void setDefaultGAV(ReleaseId releaseId) {
@@ -192,7 +195,12 @@ public class KieRepositoryImpl
}
private KieModule loadKieModuleFromMavenRepo(ReleaseId releaseId, PomModel
pomModel) {
- return KieScannerHolder.kieScanner.loadArtifact( releaseId, pomModel );
+ try {
+ lock.lock();
+ return KieScannerHolder.kieScanner.loadArtifact(releaseId,
pomModel);
+ } finally {
+ lock.unlock();
+ }
}
private static class DummyKieScanner
@@ -329,6 +337,7 @@ public class KieRepositoryImpl
= Integer.parseInt(System.getProperty(CACHE_VERSIONS_MAX_PROPERTY,
"10"));
// FIELDS
-----------------------------------------------------------------------------------------------------------------
+ private final ReentrantLock lock;
// kieModules evicts based on access-time, not on insertion-time
public final Map<String, NavigableMap<ComparableVersion, KieModule>>
kieModules
@@ -348,44 +357,59 @@ public class KieRepositoryImpl
};
// METHODS
----------------------------------------------------------------------------------------------------------------
+ public KieModuleRepo(ReentrantLock lock) {
+ this.lock = lock;
+ }
- public synchronized KieModule remove(ReleaseId releaseId) {
- KieModule removedKieModule = null;
- String ga = releaseId.getGroupId() + ":" +
releaseId.getArtifactId();
- ComparableVersion comparableVersion = new
ComparableVersion(releaseId.getVersion());
+ public KieModule remove(ReleaseId releaseId) {
+ try {
+ lock.lock();
+
+ KieModule removedKieModule = null;
+ String ga = releaseId.getGroupId() + ":" +
releaseId.getArtifactId();
+ ComparableVersion comparableVersion = new
ComparableVersion(releaseId.getVersion());
- NavigableMap<ComparableVersion, KieModule> artifactMap =
kieModules.get(ga);
- if (artifactMap != null) {
- removedKieModule = artifactMap.remove(comparableVersion);
- if (artifactMap.isEmpty()) {
- kieModules.remove(ga);
+ NavigableMap<ComparableVersion, KieModule> artifactMap =
kieModules.get(ga);
+ if (artifactMap != null) {
+ removedKieModule = artifactMap.remove(comparableVersion);
+ if (artifactMap.isEmpty()) {
+ kieModules.remove(ga);
+ }
+ oldKieModules.remove(releaseId);
}
- oldKieModules.remove(releaseId);
- }
- return removedKieModule;
+ return removedKieModule;
+ } finally {
+ lock.unlock();
+ }
}
- public synchronized void store(KieModule kieModule) {
- ReleaseId releaseId = kieModule.getReleaseId();
- String ga = releaseId.getGroupId() + ":" +
releaseId.getArtifactId();
- ComparableVersion comparableVersion = new
ComparableVersion(releaseId.getVersion());
+ public void store(KieModule kieModule) {
+ try {
+ lock.lock();
+
+ ReleaseId releaseId = kieModule.getReleaseId();
+ String ga = releaseId.getGroupId() + ":" +
releaseId.getArtifactId();
+ ComparableVersion comparableVersion = new
ComparableVersion(releaseId.getVersion());
- NavigableMap<ComparableVersion, KieModule> artifactMap =
kieModules.get(ga);
- if( artifactMap == null ) {
- artifactMap = createNewArtifactMap();
- kieModules.put(ga, artifactMap);
- }
+ NavigableMap<ComparableVersion, KieModule> artifactMap =
kieModules.get(ga);
+ if( artifactMap == null ) {
+ artifactMap = createNewArtifactMap();
+ kieModules.put(ga, artifactMap);
+ }
- KieModule oldReleaseIdKieModule = oldKieModules.get(releaseId);
- // variable used in order to test race condition
- if (oldReleaseIdKieModule == null) {
- KieModule oldKieModule = artifactMap.get(comparableVersion);
- if (oldKieModule != null) {
- oldKieModules.put( releaseId, oldKieModule );
+ KieModule oldReleaseIdKieModule = oldKieModules.get(releaseId);
+ // variable used in order to test race condition
+ if (oldReleaseIdKieModule == null) {
+ KieModule oldKieModule =
artifactMap.get(comparableVersion);
+ if (oldKieModule != null) {
+ oldKieModules.put( releaseId, oldKieModule );
+ }
}
+ artifactMap.put( comparableVersion, kieModule );
+ } finally {
+ lock.unlock();
}
- artifactMap.put( comparableVersion, kieModule );
}
/**
@@ -421,56 +445,72 @@ public class KieRepositoryImpl
return newArtifactMap;
}
- synchronized KieModule loadOldAndRemove(ReleaseId releaseId) {
- return oldKieModules.remove(releaseId);
+ KieModule loadOldAndRemove(ReleaseId releaseId) {
+ try {
+ lock.lock();
+ return oldKieModules.remove(releaseId);
+ } finally {
+ lock.unlock();
+ }
}
- synchronized KieModule load(InternalKieScanner kieScanner, ReleaseId
releaseId) {
- return load(kieScanner, releaseId, new
VersionRange(releaseId.getVersion()));
+ KieModule load(InternalKieScanner kieScanner, ReleaseId releaseId) {
+ try {
+ lock.lock();
+ return load(kieScanner, releaseId, new
VersionRange(releaseId.getVersion()));
+ } finally {
+ lock.unlock();
+ }
}
- synchronized KieModule load(InternalKieScanner kieScanner, ReleaseId
releaseId, VersionRange versionRange) {
- String ga = releaseId.getGroupId() + ":" +
releaseId.getArtifactId();
+ KieModule load(InternalKieScanner kieScanner, ReleaseId releaseId,
VersionRange versionRange) {
+ try {
+ lock.lock();
- NavigableMap<ComparableVersion, KieModule> artifactMap =
kieModules.get(ga);
- if ( artifactMap == null || artifactMap.isEmpty() ) {
- return null;
- }
- KieModule kieModule = artifactMap.get(new
ComparableVersion(releaseId.getVersion()));
-
- if (versionRange.fixed) {
- if ( kieModule != null && releaseId.isSnapshot() ) {
- String oldSnapshotVersion =
((ReleaseIdImpl)kieModule.getReleaseId()).getSnapshotVersion();
- if ( oldSnapshotVersion != null ) {
- String currentSnapshotVersion =
kieScanner.getArtifactVersion(releaseId);
- if (currentSnapshotVersion != null &&
- new
ComparableVersion(currentSnapshotVersion).compareTo(new
ComparableVersion(oldSnapshotVersion)) > 0) {
- // if the snapshot currently available on the
maven repo is newer than the cached one
- // return null to enforce the building of this
newer version
- return null;
+ String ga = releaseId.getGroupId() + ":" +
releaseId.getArtifactId();
+
+ NavigableMap<ComparableVersion, KieModule> artifactMap =
kieModules.get(ga);
+ if ( artifactMap == null || artifactMap.isEmpty() ) {
+ return null;
+ }
+ KieModule kieModule = artifactMap.get(new
ComparableVersion(releaseId.getVersion()));
+
+ if (versionRange.fixed) {
+ if ( kieModule != null && releaseId.isSnapshot() ) {
+ String oldSnapshotVersion =
((ReleaseIdImpl)kieModule.getReleaseId()).getSnapshotVersion();
+ if ( oldSnapshotVersion != null ) {
+ String currentSnapshotVersion =
kieScanner.getArtifactVersion(releaseId);
+ if (currentSnapshotVersion != null &&
+ new
ComparableVersion(currentSnapshotVersion).compareTo(new
ComparableVersion(oldSnapshotVersion)) > 0) {
+ // if the snapshot currently available on the
maven repo is newer than the cached one
+ // return null to enforce the building of this
newer version
+ return null;
+ }
}
}
+ return kieModule;
}
- return kieModule;
- }
- Map.Entry<ComparableVersion, KieModule> entry =
- versionRange.upperBound == null ?
- artifactMap.lastEntry() :
- versionRange.upperInclusive ?
- artifactMap.floorEntry(new
ComparableVersion(versionRange.upperBound)) :
- artifactMap.lowerEntry(new
ComparableVersion(versionRange.upperBound));
+ Map.Entry<ComparableVersion, KieModule> entry =
+ versionRange.upperBound == null ?
+ artifactMap.lastEntry() :
+ versionRange.upperInclusive ?
+ artifactMap.floorEntry(new
ComparableVersion(versionRange.upperBound)) :
+ artifactMap.lowerEntry(new
ComparableVersion(versionRange.upperBound));
- if ( entry == null ) {
- return null;
- }
+ if ( entry == null ) {
+ return null;
+ }
- if ( versionRange.lowerBound == null ) {
- return entry.getValue();
- }
+ if ( versionRange.lowerBound == null ) {
+ return entry.getValue();
+ }
- int comparison = entry.getKey().compareTo(new
ComparableVersion(versionRange.lowerBound));
- return comparison > 0 || (comparison == 0 &&
versionRange.lowerInclusive) ? entry.getValue() : null;
+ int comparison = entry.getKey().compareTo(new
ComparableVersion(versionRange.lowerBound));
+ return comparison > 0 || (comparison == 0 &&
versionRange.lowerInclusive) ? entry.getValue() : null;
+ } finally {
+ lock.unlock();
+ }
}
}
diff --git
a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/mvel/compiler/kie/builder/impl/KieModuleRepoTest.java
b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/mvel/compiler/kie/builder/impl/KieModuleRepoTest.java
index 089286228a..23afb8e649 100644
---
a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/mvel/compiler/kie/builder/impl/KieModuleRepoTest.java
+++
b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/mvel/compiler/kie/builder/impl/KieModuleRepoTest.java
@@ -30,6 +30,7 @@ import java.util.concurrent.BrokenBarrierException;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
+import java.util.concurrent.locks.ReentrantLock;
import org.drools.compiler.kie.builder.impl.BuildContext;
import org.drools.compiler.kie.builder.impl.InternalKieModule;
@@ -89,7 +90,7 @@ public class KieModuleRepoTest {
@Before
public void before() throws Exception {
- kieModuleRepo = new KieModuleRepo();
+ kieModuleRepo = new KieModuleRepo(new ReentrantLock());
// store the original values as we need to restore them after the test
maxSizeGaCacheOrig = KieModuleRepo.MAX_SIZE_GA_CACHE;
@@ -139,6 +140,15 @@ public class KieModuleRepoTest {
kieModuleRepoField.setAccessible(true);
kieModuleRepoField.set(kieRepository, kieModuleRepo);
kieModuleRepoField.setAccessible(false);
+ // share the lock between KieModuleRepo and KieRepository
+ final Field KieModuleRepoLockField =
KieModuleRepo.class.getDeclaredField("lock");
+ KieModuleRepoLockField.setAccessible(true);
+ Object lock = KieModuleRepoLockField.get(kieModuleRepo);
+ KieModuleRepoLockField.setAccessible(false);
+ final Field kieRepositoryImplLockField =
KieRepositoryImpl.class.getDeclaredField("lock");
+ kieRepositoryImplLockField.setAccessible(true);
+ kieRepositoryImplLockField.set(kieRepository, lock);
+ kieRepositoryImplLockField.setAccessible(false);
// kie container
final KieContainerImpl kieContainerImpl = new
KieContainerImpl(mockKieProject, kieRepository);
diff --git a/kie-ci/pom.xml b/kie-ci/pom.xml
index e4a2f76a29..4e5c85613b 100644
--- a/kie-ci/pom.xml
+++ b/kie-ci/pom.xml
@@ -36,6 +36,7 @@
<properties>
<java.module.name>org.kie.ci</java.module.name>
+
<excludedGroups>org.kie.test.testcategory.TurtleTestCategory</excludedGroups>
</properties>
<dependencies>
@@ -158,7 +159,12 @@
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<scope>test</scope>
- </dependency>
+ </dependency>
+ <dependency>
+ <groupId>org.kie</groupId>
+ <artifactId>kie-test-util</artifactId>
+ <scope>test</scope>
+ </dependency>
</dependencies>
<build>
@@ -235,4 +241,17 @@
</plugins>
</build>
+ <profiles>
+ <profile>
+ <id>runTurtleTests</id>
+ <activation>
+ <property>
+ <name>runTurtleTests</name>
+ </property>
+ </activation>
+ <properties>
+ <excludedGroups/>
+ </properties>
+ </profile>
+ </profiles>
</project>
diff --git
a/kie-ci/src/test/java/org/kie/scanner/concurrent/ConcurrentBuildTest.java
b/kie-ci/src/test/java/org/kie/scanner/concurrent/ConcurrentBuildTest.java
new file mode 100644
index 0000000000..f551e52196
--- /dev/null
+++ b/kie-ci/src/test/java/org/kie/scanner/concurrent/ConcurrentBuildTest.java
@@ -0,0 +1,118 @@
+/**
+ * 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.kie.scanner.concurrent;
+
+import java.io.IOException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import org.drools.compiler.kie.builder.impl.InternalKieModule;
+import org.drools.core.util.FileManager;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.kie.api.KieServices;
+import org.kie.api.builder.ReleaseId;
+import org.kie.scanner.AbstractKieCiTest;
+import org.kie.scanner.KieMavenRepository;
+import org.kie.test.testcategory.TurtleTestCategory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.junit.Assert.assertTrue;
+import static org.kie.scanner.KieMavenRepository.getKieMavenRepository;
+
+@Category(TurtleTestCategory.class)
+public class ConcurrentBuildTest extends AbstractKieCiTest {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConcurrentBuildTest.class);
+
+ private FileManager fileManager;
+
+ @Before
+ public void setUp() throws Exception {
+ this.fileManager = new FileManager();
+ this.fileManager.setUp();
+ ReleaseId releaseId =
KieServices.Factory.get().newReleaseId("org.kie", "scanner-test",
"1.0-SNAPSHOT");
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ this.fileManager.tearDown();
+ }
+
+ // This is TurtleTest. You can run this test with -PrunTurtleTests
+ @Test(timeout=600000)
+ public void concurrentBuildWithDependency() throws Exception {
+ KieServices ks = KieServices.Factory.get();
+ KieMavenRepository repository = getKieMavenRepository();
+
+ final int testLoop = 10;
+ for (int m = 0; m < testLoop; m++) {
+ System.out.println("===== test loop " + m + " start");
+
+ // test-dep-a exists in KieRepositoryImpl$KieModuleRepo
+ // To resolve test-dep-a, KieRepositoryImpl$KieModuleRepo.load ->
KieRepositoryScannerImpl.getArtifactVersion
+ // , so the lock order is kieModuleRepo -> kieScanner
+ System.out.println("===== dep A start");
+ ReleaseId releaseIdDepA = ks.newReleaseId("org.kie", "test-dep-a",
"1.0-SNAPSHOT");
+ InternalKieModule kJarDepA = createKieJar(ks, releaseIdDepA,
false, "ruleA");
+ repository.installArtifact(releaseIdDepA, kJarDepA,
createKPom(fileManager, releaseIdDepA));
+
+ // test-dep-b does not exist in KieRepositoryImpl$KieModuleRepo.
Instead, it is installed in local Maven repository
+ // To resolve test-dep-b,
KieRepositoryImpl.loadKieModuleFromMavenRepo ->
KieRepositoryImpl$KieModuleRepo.load
+ // , so the lock order is kieScanner -> kieModuleRepo
+ // Note: This deadlock scenario happens only in 7.x (before
RHDM-2028), because since Drools 8,
+ // ReleaseIdImpl usage is refactored and we no longer use
ReleaseIdImpl.setSnapshotVersion.
+ // But we keep this test to detect a regression.
+ System.out.println("===== dep B start");
+ ReleaseId releaseIdDepB = ks.newReleaseId("org.kie", "test-dep-b",
"1.0-SNAPSHOT");
+ InternalKieModule kJarDepB = createKieJarWithDependencies(ks,
releaseIdDepB, false, "ruleB", releaseIdDepA); // test-dep-b depends on
test-dep-a
+ repository.installArtifact(releaseIdDepB, kJarDepB,
createKPom(fileManager, releaseIdDepB, releaseIdDepA));
+
KieServices.Factory.get().getRepository().removeKieModule(releaseIdDepB);
+
+ System.out.println("===== dep artifacts are ready. Start
concurrent build");
+
+ final int maxThread = 20;
+ ExecutorService executor = Executors.newFixedThreadPool(maxThread);
+
+ for (int n = 0; n < maxThread; n++) {
+ final int i = n;
+ executor.execute(() -> {
+ ReleaseId releaseId = ks.newReleaseId("org.kie", "test-" +
i, "1.0-SNAPSHOT");
+ try {
+ ReleaseId myDependencyId = i % 2 == 0 ? releaseIdDepA
: releaseIdDepB;
+ InternalKieModule kJar =
createKieJarWithDependencies(ks, releaseId, false, "rule" + i, myDependencyId);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ });
+ }
+
+ executor.shutdown();
+ executor.awaitTermination(300, TimeUnit.SECONDS);
+
+ // cleanup
+
KieServices.Factory.get().getRepository().removeKieModule(releaseIdDepA);
+
KieServices.Factory.get().getRepository().removeKieModule(releaseIdDepB);
+ }
+ assertTrue(true); // no deadlock
+ }
+}
\ No newline at end of file
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
