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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
