From 508d06b4a485ff7e0b176a3e802345c6e71e8986 Mon Sep 17 00:00:00 2001 From: mariofusco Date: Wed, 11 Dec 2024 18:30:32 +0100 Subject: [PATCH 1/5] [KIE-DROOLS-6190] fix removal of detached tuples during incremental compilation --- .../drools/core/common/DefaultFactHandle.java | 54 +------------------ .../core/phreak/RuleNetworkEvaluator.java | 2 +- .../AddRemoveGenerated2RulesEvalTest.java | 1 - .../AddRemoveGenerated2RulesIntegerTest.java | 1 - ...dRemoveGenerated2RulesMapContainsTest.java | 1 - ...emoveGenerated2RulesStringIntegerTest.java | 1 - .../AddRemoveGenerated2RulesStringTest.java | 1 - .../CasesFromGeneratedRulesTest.java | 37 +++++++++++++ 8 files changed, 40 insertions(+), 58 deletions(-) create mode 100644 drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java diff --git a/drools-core/src/main/java/org/drools/core/common/DefaultFactHandle.java b/drools-core/src/main/java/org/drools/core/common/DefaultFactHandle.java index 51365d30d7c..27d40527f9c 100644 --- a/drools-core/src/main/java/org/drools/core/common/DefaultFactHandle.java +++ b/drools-core/src/main/java/org/drools/core/common/DefaultFactHandle.java @@ -507,30 +507,6 @@ public void addLastLeftTuple( TupleImpl leftTuple) { lastLeftTuple = leftTuple; } - private void addLastTuple(TupleImpl tuple, boolean left) { - if (left) { - addLastLeftTuple(tuple); - } else { - addLastRightTuple(tuple); - } - } - - private void setFirstTuple(TupleImpl tuple, boolean left) { - if (left) { - firstLeftTuple = tuple; - } else { - firstRightTuple = tuple; - } - } - - private void setLastTuple(TupleImpl tuple, boolean left) { - if (left) { - lastLeftTuple = tuple; - } else { - lastRightTuple = tuple; - } - } - @Override public void removeLeftTuple( TupleImpl leftTuple ) { TupleImpl previous = leftTuple.getHandlePrevious(); @@ -681,21 +657,8 @@ public TupleImpl detachLeftTupleAfter(RuleBasePartitionId partitionId, ObjectTyp } if (detached != null) { - if (firstLeftTuple == detached) { - firstLeftTuple = null; - } - - if (lastLeftTuple == detached) { - lastLeftTuple = null; - } - - if (detached.getHandlePrevious() != null) { - lastLeftTuple = detached.getHandlePrevious(); - detached.setHandlePrevious(null); - lastLeftTuple.setHandleNext(null); - } + removeLeftTuple(detached); } - return detached; } @@ -709,21 +672,8 @@ public TupleImpl detachRightTupleAfter(RuleBasePartitionId partitionId, ObjectTy } if (detached != null) { - if (firstRightTuple == detached) { - firstRightTuple = null; - } - - if (lastRightTuple == detached) { - lastRightTuple = null; - } - - if (detached.getHandlePrevious() != null) { - lastRightTuple = detached.getHandlePrevious(); - detached.setHandlePrevious(null); - lastRightTuple.setHandleNext(null); - } + removeRightTuple(detached); } - return detached; } diff --git a/drools-core/src/main/java/org/drools/core/phreak/RuleNetworkEvaluator.java b/drools-core/src/main/java/org/drools/core/phreak/RuleNetworkEvaluator.java index eef6b8ad1b8..8e8c12b4c43 100644 --- a/drools-core/src/main/java/org/drools/core/phreak/RuleNetworkEvaluator.java +++ b/drools-core/src/main/java/org/drools/core/phreak/RuleNetworkEvaluator.java @@ -273,7 +273,7 @@ public void innerEval(PathMemory pmem, } boolean emptySrcTuples = srcTuples.isEmpty(); - if ( !(NodeTypeEnums.isBetaNode(node) && ((BetaNode)node).isRightInputIsRiaNode() ) ) { + if ( !(NodeTypeEnums.isBetaNode(node) && node.isRightInputIsRiaNode() ) ) { // The engine cannot skip a ria node, as the dirty might be several levels deep if ( emptySrcTuples && smem.getDirtyNodeMask() == 0) { // empty sources and segment is not dirty, skip to non empty src tuples or dirty segment. diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesEvalTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesEvalTest.java index 52875fb88ba..9470289315c 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesEvalTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesEvalTest.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.Disabled; -@Disabled("It gets stuck. See issue #6190") public class AddRemoveGenerated2RulesEvalTest extends AbstractAddRemoveGenerated2RulesTest { public static Stream parameters() { diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesIntegerTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesIntegerTest.java index 4c88cf1b0ea..2ed797c431f 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesIntegerTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesIntegerTest.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.Disabled; -@Disabled("It gets stuck. See issue #6190") public class AddRemoveGenerated2RulesIntegerTest extends AbstractAddRemoveGenerated2RulesTest { public static Stream parameters() { diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesMapContainsTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesMapContainsTest.java index d849893844a..f2188f23da7 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesMapContainsTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesMapContainsTest.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.Disabled; -@Disabled("It gets stuck. See issue #6190") public class AddRemoveGenerated2RulesMapContainsTest extends AbstractAddRemoveGenerated2RulesTest { public static Stream parameters() { diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesStringIntegerTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesStringIntegerTest.java index 9d2d3bdd8fa..e2b49a60133 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesStringIntegerTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesStringIntegerTest.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.Disabled; -@Disabled("It gets stuck. See issue #6190") public class AddRemoveGenerated2RulesStringIntegerTest extends AbstractAddRemoveGenerated2RulesTest { public static Stream parameters() { diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesStringTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesStringTest.java index fd97191f78d..28bc702b4de 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesStringTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesStringTest.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.Disabled; -@Disabled("It gets stuck. See issue #6190") public class AddRemoveGenerated2RulesStringTest extends AbstractAddRemoveGenerated2RulesTest { public static Stream parameters() { diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java new file mode 100644 index 00000000000..c6c4a6058e3 --- /dev/null +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java @@ -0,0 +1,37 @@ +package org.drools.compiler.integrationtests.incrementalcompilation; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +public class CasesFromGeneratedRulesTest { + + @Test + @Timeout(40000) + public void testInsertFactsFireRulesRemoveRulesReinsertRulesRevertedRules() { + String rule1 = """ + package com.rules; + global java.util.List list + rule R1 + when + Integer() + then + list.add('R1'); + end + """; + + String rule2 = """ + package com.rules; + global java.util.List list + rule R2 + when + Integer() + exists(Integer() and Integer()) + exists(Integer() and Integer()) + then + list.add('R2'); + end + """; + + AddRemoveTestCases.insertFactsFireRulesRemoveRulesReinsertRules1(rule1, rule2, TestUtil.RULE1_NAME, TestUtil.RULE2_NAME, null, 1, 2); + } +} From e6696d8e19f86e862f55fc3f819959a133028557 Mon Sep 17 00:00:00 2001 From: mariofusco Date: Wed, 11 Dec 2024 18:35:48 +0100 Subject: [PATCH 2/5] add missing license --- .../CasesFromGeneratedRulesTest.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java index c6c4a6058e3..2462e2165ad 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java @@ -1,3 +1,21 @@ +/** + * 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.compiler.integrationtests.incrementalcompilation; import org.junit.jupiter.api.Test; From d43f2d5d3b22248dafb9cf9e1c28d97552170561 Mon Sep 17 00:00:00 2001 From: mariofusco Date: Thu, 12 Dec 2024 09:42:10 +0100 Subject: [PATCH 3/5] wip --- .../drools/core/common/DefaultFactHandle.java | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/drools-core/src/main/java/org/drools/core/common/DefaultFactHandle.java b/drools-core/src/main/java/org/drools/core/common/DefaultFactHandle.java index 27d40527f9c..88a05d6e053 100644 --- a/drools-core/src/main/java/org/drools/core/common/DefaultFactHandle.java +++ b/drools-core/src/main/java/org/drools/core/common/DefaultFactHandle.java @@ -657,7 +657,20 @@ public TupleImpl detachLeftTupleAfter(RuleBasePartitionId partitionId, ObjectTyp } if (detached != null) { - removeLeftTuple(detached); + if (firstLeftTuple == detached) { + firstLeftTuple = null; + lastLeftTuple = null; + } + + if (lastLeftTuple == detached) { + lastLeftTuple = null; + } + + if (detached.getHandlePrevious() != null) { + lastLeftTuple = detached.getHandlePrevious(); + detached.setHandlePrevious(null); + lastLeftTuple.setHandleNext(null); + } } return detached; } @@ -672,7 +685,20 @@ public TupleImpl detachRightTupleAfter(RuleBasePartitionId partitionId, ObjectTy } if (detached != null) { - removeRightTuple(detached); + if (firstRightTuple == detached) { + firstRightTuple = null; + lastRightTuple = null; + } + + if (lastRightTuple == detached) { + lastRightTuple = null; + } + + if (detached.getHandlePrevious() != null) { + lastRightTuple = detached.getHandlePrevious(); + detached.setHandlePrevious(null); + lastRightTuple.setHandleNext(null); + } } return detached; } From 6f5b24aff223ff5ff4670e88312689841f55c9c0 Mon Sep 17 00:00:00 2001 From: mariofusco Date: Thu, 12 Dec 2024 19:13:51 +0100 Subject: [PATCH 4/5] wip --- .../core/phreak/EagerPhreakBuilder.java | 23 ++----------- .../AddRemoveGenerated2RulesNotNotTest.java | 1 - .../AddRemoveGenerated2RulesNotTest.java | 1 - .../CasesFromGeneratedRulesTest.java | 32 +++++++++++++++++++ 4 files changed, 34 insertions(+), 23 deletions(-) diff --git a/drools-core/src/main/java/org/drools/core/phreak/EagerPhreakBuilder.java b/drools-core/src/main/java/org/drools/core/phreak/EagerPhreakBuilder.java index 2c4e58d806f..15478b1a2b7 100644 --- a/drools-core/src/main/java/org/drools/core/phreak/EagerPhreakBuilder.java +++ b/drools-core/src/main/java/org/drools/core/phreak/EagerPhreakBuilder.java @@ -151,8 +151,6 @@ public void removeRule(TerminalNode tn, Collection wms, I } } - Set smemsToNotify = new HashSet<>(); - if (exclBranchRoots.isEmpty()) { LeftTupleNode lian = tn.getPathNodes()[0]; processLeftTuples(lian, false, tn, wms); @@ -163,7 +161,7 @@ public void removeRule(TerminalNode tn, Collection wms, I // Process existing branches from the split points Set visited = new HashSet<>(); - exclBranchRoots.forEach(pair -> Remove.processMerges(pair.parent, tn, kBase, wms, visited, smemsToNotify)); + exclBranchRoots.forEach(pair -> Remove.processMerges(pair.parent, tn, kBase, wms, visited)); } for (InternalWorkingMemory wm : wms) { @@ -174,8 +172,6 @@ public void removeRule(TerminalNode tn, Collection wms, I pmem.getRuleAgendaItem().dequeue(); } } - - smemsToNotify.forEach(pair -> pair.sm.notifyRuleLinkSegment(pair.wm)); } public static void notifyImpactedSegments(SegmentMemory smem, InternalWorkingMemory wm, Set segmentsToNotify) { @@ -736,8 +732,7 @@ private static void removeExistingPaths(List exclBranchRoots, TerminalNode } } - - private static void processMerges(LeftTupleNode splitNode, TerminalNode tn, InternalRuleBase kBase, Collection wms, Set visited, Set smemsToNotify) { + private static void processMerges(LeftTupleNode splitNode, TerminalNode tn, InternalRuleBase kBase, Collection wms, Set visited) { // it's possible for a rule to have multiple exclBranches, pointing to the same parent. So need to ensure it's processed once. if ( !visited.add(splitNode.getId())) { return; @@ -766,10 +761,7 @@ private static void processMerges(LeftTupleNode splitNode, TerminalNode tn, Inte } SegmentPrototype proto2 = kBase.getSegmentPrototype(ltn); - mergeSegments(proto1, proto2, kBase, wms); - - notifyImpactedSegments(wms, proto1, smemsToNotify); } } @@ -1443,17 +1435,6 @@ private static void updatePaths(SegmentPrototype proto, Collection wms, SegmentPrototype proto1, Set smemsToNotify) { - // any impacted segments must be notified for potential linking - for (InternalWorkingMemory wm : wms) { - Memory mem1 = wm.getNodeMemories().peekNodeMemory(proto1.getRootNode()); - if (mem1 != null && mem1.getSegmentMemory() != null) { - // there was a split segment, both need notifying. - notifyImpactedSegments(mem1.getSegmentMemory(), wm, smemsToNotify); - } - } - } - private static void setNodeTypes(SegmentPrototype proto, LeftTupleNode[] protoNodes) { int nodeTypesInSegment = 0; for ( LeftTupleNode node : protoNodes) { diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesNotNotTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesNotNotTest.java index 77fae44b931..161c91cd728 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesNotNotTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesNotNotTest.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.Disabled; -@Disabled("It gets stuck. See issue #6190") public class AddRemoveGenerated2RulesNotNotTest extends AbstractAddRemoveGenerated2RulesTest { public static Stream parameters() { diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesNotTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesNotTest.java index 8e8a40e7595..e83554563b4 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesNotTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/AddRemoveGenerated2RulesNotTest.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.Disabled; -@Disabled("It gets stuck. See issue #6190") public class AddRemoveGenerated2RulesNotTest extends AbstractAddRemoveGenerated2RulesTest { public static Stream parameters() { diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java index 2462e2165ad..13696bba1a3 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/incrementalcompilation/CasesFromGeneratedRulesTest.java @@ -52,4 +52,36 @@ public void testInsertFactsFireRulesRemoveRulesReinsertRulesRevertedRules() { AddRemoveTestCases.insertFactsFireRulesRemoveRulesReinsertRules1(rule1, rule2, TestUtil.RULE1_NAME, TestUtil.RULE2_NAME, null, 1, 2); } + + @Test + @Timeout(40000) + public void testInsertFactsRemoveRulesFireRulesRemoveRules() { + String rule1 = """ + package com.rules; + global java.util.List list + rule R1 + when + exists(Integer()) + not(Double() and Double()) + Integer() not(Double() and Double()) + then + list.add('R1'); + end + """; + + String rule2 = """ + package com.rules; + global java.util.List list + rule R2 + when + exists(Integer()) + not(Double() and Double()) + exists(Integer()) + then + list.add('R2'); + end + """; + + AddRemoveTestCases.insertFactsRemoveRulesFireRulesRemoveRules2(rule1, rule2, TestUtil.RULE1_NAME, TestUtil.RULE2_NAME, null, 1); + } } From 48039f910a843fcc52a9c120310c3bef498c4538 Mon Sep 17 00:00:00 2001 From: mariofusco Date: Thu, 12 Dec 2024 19:17:44 +0100 Subject: [PATCH 5/5] wip --- .../src/main/java/org/drools/core/reteoo/ReteooBuilder.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drools-core/src/main/java/org/drools/core/reteoo/ReteooBuilder.java b/drools-core/src/main/java/org/drools/core/reteoo/ReteooBuilder.java index 2d7ada53d35..4300fca8ce2 100644 --- a/drools-core/src/main/java/org/drools/core/reteoo/ReteooBuilder.java +++ b/drools-core/src/main/java/org/drools/core/reteoo/ReteooBuilder.java @@ -193,9 +193,7 @@ public void removeTerminalNode(RuleRemovalContext context, TerminalNode tn, Coll tn.visitLeftTupleNodes(n -> n.removeAssociatedTerminal(tn)); - BaseNode node = (BaseNode) tn; - removeNodeAssociation(node, context.getRule(), new HashSet<>(), context); - + removeNodeAssociation((BaseNode) tn, context.getRule(), new HashSet<>(), context); resetMasks(removeNodes((AbstractTerminalNode)tn, workingMemories, context)); }