You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "Rikkola (via GitHub)" <gi...@apache.org> on 2026/05/19 08:25:06 UTC
[PR] [incubator-kie-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Rikkola opened a new pull request, #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707
https://github.com/apache/incubator-kie-drools/issues/6422#issuecomment-4157177207
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3279390870
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
@Rikkola Can you please give more context about the fix? I'm not familiar with this code. 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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3282843049
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
I will make the test more clear on what the issue is. That way it is stored also for future needs.
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer merged PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#issuecomment-4488302071
So the fix does not use contains(), not sure if this how big the list can get and if this optimatisation is worth it.
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3322882663
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
Ok done
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3305084503
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
@Rikkola Thank you, I guess I understood that. A couple of questions:
- In which conditions those orphan tuples appear? Should investigate how to prevent that or it is somehow expected?
- It is safe to have those orphan in memory? Your fix doesn't remove it from the involevd linked list, but it will contain to stay in memory, I guess. Should we consider to remove that tuple?
Apologies if I'm so picky, I'm just asking to learn more :)
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3317887390
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
I think it makes sense. Especially now with AI I think there is someone who actually reads all the logs 😄
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3309875111
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
No ask away. I have already learned more while finding the explanations.
What you are asking is a reproducer for this bug with DRL. So we would all like to know it and that would give the actual data :-(
Best bet is `removeActiveTupe` in `RuleExecutor`. Tuple is doing a direct DELETE and then adding to dormant tuples would be wrong. However in some other location we still try to clean it up.
```
public void removeActiveTuple(RuleTerminalNodeLeftTuple tuple) {
tuple.setQueued(false);
activeMatches.remove(tuple);
if (tuple.getStagedType() != Tuple.DELETE) {
addDormantTuple(tuple);
}
if (queue != null) {
removeQueuedLeftTuple(tuple);
}
}
```
The risk here is we are not fixing the real bug, but I trust the reporters have tests to make sure the results the rules give are correct. Another way would be, we give the user with the issues a setup that collects as much data as possible when this issue occurs.
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3310052224
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
@Rikkola If you think that is not needed / not worth, please feel free to merge.
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3279424323
##########
drools-core/src/test/java/org/drools/core/phreak/RuleExecutorDormantTupleTest.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * 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.core.phreak;
+
+import org.drools.base.base.SalienceInteger;
+import org.drools.base.definitions.rule.impl.RuleImpl;
+import org.drools.core.reteoo.RuleTerminalNodeLeftTuple;
+import org.drools.core.reteoo.Tuple;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatNoException;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Targeted regression test for the guard in {@link RuleExecutor#removeDormantTuple} added for
+ * issue 6422. The integration repro (see DormantTupleNPETest) exercises the bug end-to-end
+ * through a multi-rule DRL, but the NPE itself reduces to a list-state invariant: a tuple
+ * passed to {@code removeDormantTuple} may already have been unlinked from {@code dormantMatches}
+ * by another path (e.g. {@code modifyActiveTuple}), in which case its {@code previous} pointer
+ * is null and the unguarded {@code LinkedList.remove()} dereferences it.
Review Comment:
The class referenced in the Javadoc ("DormantTupleNPETest") doesn't exist in the test sources, so this comment is misleading. Either add the referenced integration test or remove/replace the reference with the actual test/class/issue link that provides the end-to-end reproduction.
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3309948104
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
@Rikkola The only point you can consider, if is possible, is to add a debug log in case of an orphan tuple (In the else block), telling the info about the orphan tuple. In case the root cause is not solved, that info can help us to know more about. WDYT?
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#issuecomment-4485826356
Previous rejected PR https://github.com/apache/incubator-kie-drools/pull/6631
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3309948104
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
@Rikkola The only point you can consider, if is possible, is to add a debug log in case of an orphan tuple (In the else block), telling the info about the orphan tuple. In case the root cause is not solved, that info can help us to know more about it. WDYT?
--
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-drools-6422] Fix NPE in RuleExecutor.removeDormantTuple [incubator-kie-drools]
Posted by "Rikkola (via GitHub)" <gi...@apache.org>.
Rikkola commented on code in PR #6707:
URL: https://github.com/apache/incubator-kie-drools/pull/6707#discussion_r3296766281
##########
drools-core/src/main/java/org/drools/core/phreak/RuleExecutor.java:
##########
@@ -311,7 +311,9 @@ public void removeDormantTuple(RuleTerminalNodeLeftTuple tuple) {
throw new IllegalStateException();
}
}
- dormantMatches.remove(tuple);
+ if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
+ dormantMatches.remove(tuple);
+ }
Review Comment:
Ok I can not really make the code as clear as this explanation.
#### Drools custom LinkedLists
We have two LinkedLists, `TupleList` also extends `org.drools.core.util.LinkedList`. These contain `Tuples`, the `Tuples` themselves contain the links to previous and next, there are no wrappers inside the Lists.
```java
private final TupleList activeMatches; // queued, not yet fired
private final LinkedList<TupleImpl> dormantMatches; // already fired, kept for cancellation
```
`Tuple` has to be either in `activeMatches` or in `dormantMatches` due to the linking it can not be in both.
#### The NPE
Let's take a look at the original stacktrace:
`Caused by: 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`
`tuple.getPrevious()` is `null`, meaning it would have to be the first in either `LinkedList`. It is not. Problem `tuple` is likely an orphan. Either we do double delete or never added it to the `dormantList`.
#### The fix
So the fix checks if `getPrevious() != null`, this is what causes the NPE. That alone is not enough, since there is one legal target in the list in situations when `getPrevious()` is null and that is when it is the first item in `dormantMatches`.
```
if (tuple.getPrevious() != null || dormantMatches.getFirst() == tuple) {
dormantMatches.remove(tuple);
}
```
#### LinkedList internals
Then the location where this happens `org.drools.core.util.LinkedList`. In `RuleExecutor` the `T node` is `RuleTerminalNodeLeftTuple tuple`.
```
public void remove(final T node) {
if ( this.firstNode == node ) { // <-- Problem tuple is not first
removeFirst();
} else if ( this.lastNode == node ) { // <-- Problem tuple is not last
removeLast();
} else { // <-- We try to remove from middle
node.getPrevious().setNext( node.getNext() ); // <-- NPE
node.getNext().setPrevious( node.getPrevious() );
this.size--;
node.setPrevious( null );
node.setNext( null );
}
}
```
#### Why not contains()?
The check used is O(1) when `dormantMatches.contains()` would be O(n).
--
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