You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "tkobayas (via GitHub)" <gi...@apache.org> on 2025/09/19 09:10:20 UTC
[PR] [incubator-kie-issues-2105] Conditional named consequence with break … [incubator-kie-drools]
tkobayas opened a new pull request, #6461:
URL: https://github.com/apache/incubator-kie-drools/pull/6461
…doesn't break
- https://github.com/apache/incubator-kie-issues/issues/2105
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues-2105] Conditional named consequence with break … [incubator-kie-drools]
Posted by "mariofusco (via GitHub)" <gi...@apache.org>.
mariofusco commented on code in PR #6461:
URL: https://github.com/apache/incubator-kie-drools/pull/6461#discussion_r2362453428
##########
drools-core/src/main/java/org/drools/core/phreak/PhreakBranchNode.java:
##########
@@ -166,11 +166,8 @@ public void doLeftUpdates(ConditionalBranchNode branchNode,
if (branchTuples.mainLeftTuple != null) {
normalizeStagedTuples( stagedLeftTuples, branchTuples.mainLeftTuple );
- if (breaking && !NodeTypeEnums.isTerminalNode(branchTuples.mainLeftTuple.getSink())) {
- // child exist, new one does not, so delete
- trgLeftTuples.addDelete(branchTuples.mainLeftTuple);
- } else {
- // child exist, new one does, so update
+ if (!breaking) {
+ // default consequence will also be executed
trgLeftTuples.addUpdate(branchTuples.mainLeftTuple);
}
Review Comment:
I agree, I believe that A) is the right thing to do here, great job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues-2105] Conditional named consequence with break … [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6461:
URL: https://github.com/apache/incubator-kie-drools/pull/6461#discussion_r2362373118
##########
drools-core/src/main/java/org/drools/core/phreak/PhreakBranchNode.java:
##########
@@ -166,11 +166,8 @@ public void doLeftUpdates(ConditionalBranchNode branchNode,
if (branchTuples.mainLeftTuple != null) {
normalizeStagedTuples( stagedLeftTuples, branchTuples.mainLeftTuple );
- if (breaking && !NodeTypeEnums.isTerminalNode(branchTuples.mainLeftTuple.getSink())) {
- // child exist, new one does not, so delete
- trgLeftTuples.addDelete(branchTuples.mainLeftTuple);
- } else {
- // child exist, new one does, so update
+ if (!breaking) {
+ // default consequence will also be executed
trgLeftTuples.addUpdate(branchTuples.mainLeftTuple);
}
Review Comment:
@mariofusco
The current behaviour `if (breaking && !NodeTypeEnums.isTerminalNode(branchTuples.mainLeftTuple.getSink()))` updates `branchTuples.mainLeftTuple`, so both a default consequence and a named consequence are executed. This is the originally reported issue and the test case reproduces that.
If I remove the condition `&& !NodeTypeEnums.isTerminalNode(branchTuples.mainLeftTuple.getSink())` (introduced by https://github.com/apache/incubator-kie-drools/commit/87b2f656c495ef61c048db7cc3aedf2a0ed821bd), it deletes `branchTuples.mainLeftTuple`. Then the next issue arises. If the branch condition is evaluated as `default` -> `break` -> `default` (like the test case). The deleted (= `removeDormantTuple` is called) `branchTuples.mainLeftTuple` is updated, so `modifyActiveTuple` causes NPE because it calls `removeDormantTuple` to a tuple which was already removed from dormantMatches LinkedList.
```
java.lang.NullPointerException: Cannot invoke "org.drools.core.util.DoubleLinkedEntry.setNext(org.drools.core.util.SingleLinkedEntry)" because the return value of "org.drools.core.util.DoubleLinkedEntry.getPrevious()" is null
at org.drools.core.util.LinkedList.remove(LinkedList.java:178)
at org.drools.core.phreak.RuleExecutor.removeDormantTuple(RuleExecutor.java:318)
at org.drools.core.phreak.RuleExecutor.modifyActiveTuple(RuleExecutor.java:338)
at org.drools.core.phreak.PhreakRuleTerminalNode.doLeftTupleUpdate(PhreakRuleTerminalNode.java:223)
at org.drools.core.phreak.PhreakRuleTerminalNode.doLeftUpdates(PhreakRuleTerminalNode.java:165)
at org.drools.core.phreak.PhreakRuleTerminalNode.doNode(PhreakRuleTerminalNode.java:56)
at org.drools.core.phreak.RuleNetworkEvaluator.innerEval(RuleNetworkEvaluator.java:335)
at org.drools.core.phreak.RuleNetworkEvaluator.outerEval(RuleNetworkEvaluator.java:189)
```
stacktrace with `DEBUG_DORMANT_TUPLE = true`
```
java.lang.IllegalStateException
at org.drools.core.phreak.RuleExecutor.removeDormantTuple(RuleExecutor.java:315)
at org.drools.core.phreak.RuleExecutor.modifyActiveTuple(RuleExecutor.java:338)
at org.drools.core.phreak.PhreakRuleTerminalNode.doLeftTupleUpdate(PhreakRuleTerminalNode.java:223)
at org.drools.core.phreak.PhreakRuleTerminalNode.doLeftUpdates(PhreakRuleTerminalNode.java:165)
at org.drools.core.phreak.PhreakRuleTerminalNode.doNode(PhreakRuleTerminalNode.java:56)
at org.drools.core.phreak.RuleNetworkEvaluator.innerEval(RuleNetworkEvaluator.java:335)
at org.drools.core.phreak.RuleNetworkEvaluator.outerEval(RuleNetworkEvaluator.java:189)
at org.drools.core.phreak.RuleNetworkEvaluator.evaluateNetwork(RuleNetworkEvaluator.java:147)
```
Possible fixes:
A) Don't delete `mainLeftTuple`.
Update `branchTuples.mainLeftTuple` when `breaking == false`. Otherwise, just leave it, so it can be updated when the condition results in `default` in the future.
B) Completely delete `mainLeftTuple`.
`trgLeftTuples.addDelete(branchTuples.mainLeftTuple)` doesn't seem to be enough. Probably we can remove the `mainLeftTuple` reference from `branchTuples`.
Currently my fix is A). But if you think B) is the right direction, feel free to let me know. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues-2105] Conditional named consequence with break … [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #6461:
URL: https://github.com/apache/incubator-kie-drools/pull/6461#discussion_r2362373118
##########
drools-core/src/main/java/org/drools/core/phreak/PhreakBranchNode.java:
##########
@@ -166,11 +166,8 @@ public void doLeftUpdates(ConditionalBranchNode branchNode,
if (branchTuples.mainLeftTuple != null) {
normalizeStagedTuples( stagedLeftTuples, branchTuples.mainLeftTuple );
- if (breaking && !NodeTypeEnums.isTerminalNode(branchTuples.mainLeftTuple.getSink())) {
- // child exist, new one does not, so delete
- trgLeftTuples.addDelete(branchTuples.mainLeftTuple);
- } else {
- // child exist, new one does, so update
+ if (!breaking) {
+ // default consequence will also be executed
trgLeftTuples.addUpdate(branchTuples.mainLeftTuple);
}
Review Comment:
@mariofusco
The current behaviour `if (breaking && !NodeTypeEnums.isTerminalNode(branchTuples.mainLeftTuple.getSink()))` updates `branchTuples.mainLeftTuple`, so both a default consequence and a named consequence are executed. This is the originally reported issue and the test case reproduces that.
If I remove the condition `&& !NodeTypeEnums.isTerminalNode(branchTuples.mainLeftTuple.getSink())`, it deletes `branchTuples.mainLeftTuple`. Then the next issue arises. If the branch condition is evaluated as `default` -> `break` -> `default` (like the test case). The deleted (= `removeDormantTuple` is called) `branchTuples.mainLeftTuple` is updated, so `modifyActiveTuple` causes NPE because it calls `removeDormantTuple` to a tuple which was already removed from dormantMatches LinkedList.
```
java.lang.NullPointerException: Cannot invoke "org.drools.core.util.DoubleLinkedEntry.setNext(org.drools.core.util.SingleLinkedEntry)" because the return value of "org.drools.core.util.DoubleLinkedEntry.getPrevious()" is null
at org.drools.core.util.LinkedList.remove(LinkedList.java:178)
at org.drools.core.phreak.RuleExecutor.removeDormantTuple(RuleExecutor.java:318)
at org.drools.core.phreak.RuleExecutor.modifyActiveTuple(RuleExecutor.java:338)
at org.drools.core.phreak.PhreakRuleTerminalNode.doLeftTupleUpdate(PhreakRuleTerminalNode.java:223)
at org.drools.core.phreak.PhreakRuleTerminalNode.doLeftUpdates(PhreakRuleTerminalNode.java:165)
at org.drools.core.phreak.PhreakRuleTerminalNode.doNode(PhreakRuleTerminalNode.java:56)
at org.drools.core.phreak.RuleNetworkEvaluator.innerEval(RuleNetworkEvaluator.java:335)
at org.drools.core.phreak.RuleNetworkEvaluator.outerEval(RuleNetworkEvaluator.java:189)
```
stacktrace with `DEBUG_DORMANT_TUPLE = true`
```
java.lang.IllegalStateException
at org.drools.core.phreak.RuleExecutor.removeDormantTuple(RuleExecutor.java:315)
at org.drools.core.phreak.RuleExecutor.modifyActiveTuple(RuleExecutor.java:338)
at org.drools.core.phreak.PhreakRuleTerminalNode.doLeftTupleUpdate(PhreakRuleTerminalNode.java:223)
at org.drools.core.phreak.PhreakRuleTerminalNode.doLeftUpdates(PhreakRuleTerminalNode.java:165)
at org.drools.core.phreak.PhreakRuleTerminalNode.doNode(PhreakRuleTerminalNode.java:56)
at org.drools.core.phreak.RuleNetworkEvaluator.innerEval(RuleNetworkEvaluator.java:335)
at org.drools.core.phreak.RuleNetworkEvaluator.outerEval(RuleNetworkEvaluator.java:189)
at org.drools.core.phreak.RuleNetworkEvaluator.evaluateNetwork(RuleNetworkEvaluator.java:147)
```
Possible fixes:
A) Don't delete `mainLeftTuple`.
Update `branchTuples.mainLeftTuple` when `breaking == false`. Otherwise, just leave it, so it can be updated when the condition results in `default` in the future.
B) Completely delete `mainLeftTuple`.
`trgLeftTuples.addDelete(branchTuples.mainLeftTuple)` doesn't seem to be enough. Probably we can remove the `mainLeftTuple` reference from `branchTuples`.
Currently my fix is A). But if you think B) is the right direction, feel free to let me know. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues-2105] Conditional named consequence with break … [incubator-kie-drools]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on PR #6461:
URL: https://github.com/apache/incubator-kie-drools/pull/6461#issuecomment-3345854266
Still learning this area, but for what I understand ATM this is fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues-2105] Conditional named consequence with break … [incubator-kie-drools]
Posted by "mariofusco (via GitHub)" <gi...@apache.org>.
mariofusco merged PR #6461:
URL: https://github.com/apache/incubator-kie-drools/pull/6461
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org