Copilot commented on code in PR #7135:
URL: https://github.com/apache/incubator-kie/pull/7135#discussion_r4192774136
##########
drools-core/src/main/java/org/drools/core/phreak/SegmentMemorySupportImpl.java:
##########
@@ -213,14 +213,19 @@ public SegmentMemory
createChildSegmentLazily(LeftTupleNode node) {
@Override
public void initializeChildSegmentsIfNeeded(SegmentMemory smem) {
LeftTupleSinkPropagator sinkPropagator =
smem.getTipNode().getSinkPropagator();
- if (!smem.isEmpty()) {
- return; // this can happen when multiple threads are trying to
initialize the segment
+ if (!smem.isEmpty() && sinkPropagator.size() == 1) {
+ return; // threading guard: single-sink child already initialized
}
+ // When the tip node has multiple sinks (segment split by a sharing
rule), some children
+ // may already have been added to smem while sibling children are
still missing.
+ // Iterate all sinks, create any missing child segment, and add it
only if not already present.
for (LeftTupleSinkNode sink = sinkPropagator.getFirstLeftTupleSink();
sink != null; sink = sink
.getNextLeftTupleSinkNode()) {
SegmentMemory childSmem = PhreakBuilder.isEagerSegmentCreation() ?
createChildSegment(sink)
: createChildSegmentLazily(sink);
- smem.add(childSmem);
+ if (childSmem != null && !smem.contains(childSmem)) {
+ smem.add(childSmem);
Review Comment:
The new `contains`/`add` pair is not atomic, even though this method
explicitly supports concurrent initialization. With multiple sinks, two threads
can both observe the same child as absent and add the same `SegmentMemory`
twice, corrupting the intrusive linked list. Serialize the multi-sink
initialization (with the emptiness/contains checks inside the critical section)
or provide an atomic add-if-absent operation.
##########
drools-core/src/main/java/org/drools/core/reteoo/SingleObjectSinkAdapter.java:
##########
@@ -120,17 +121,36 @@ public void doUnlinkSubnetwork(ReteEvaluator
reteEvaluator) {
public static void staticDoUnlinkSubnetwork(ObjectSink sink, ReteEvaluator
reteEvaluator) {
BetaMemory bm;
+ BetaNode betaNode;
if ( sink.getType() == NodeTypeEnums.AccumulateRightAdapterNode ) {
AccumulateNode accnode = ((AccumulateRight)sink).getBetaNode();
AccumulateMemory accMem = ( AccumulateMemory )
reteEvaluator.getNodeMemory( accnode );
bm = accMem.getBetaMemory();
- } else {
- BetaNode betaNode = ((RightInputAdapterNode) sink).getBetaNode();
+ betaNode = accnode;
+ } else {
+ betaNode = ((RightInputAdapterNode) sink).getBetaNode();
bm = RightInputAdapterNode.getBetaMemoryFromRightInput(betaNode,
reteEvaluator);
}
- if (sink.getType() == NodeTypeEnums.NotNode) {
- bm.linkNode( ( BetaNode ) sink, reteEvaluator );
+ if (betaNode.getType() == NodeTypeEnums.NotNode) {
+ bm.linkNode(betaNode, reteEvaluator);
Review Comment:
This recovery path still depends on `SubnetworkPathMemory.doUnlinkRule()`
being called, but its inherited `PathMemory.unlinkedSegment()` only calls
`doUnlinkRule()` when the whole path changes from linked to unlinked. If
another segment has already made the path unlinked, the downstream sinks are
never notified and this new relinking logic is not reached. The stated
`TupleToObjectNode.unlinkedSegment` fix is absent from this change, so that
failure mode remains.
##########
drools-core/src/main/java/org/drools/core/reteoo/SingleObjectSinkAdapter.java:
##########
@@ -120,17 +121,36 @@ public void doUnlinkSubnetwork(ReteEvaluator
reteEvaluator) {
public static void staticDoUnlinkSubnetwork(ObjectSink sink, ReteEvaluator
reteEvaluator) {
BetaMemory bm;
+ BetaNode betaNode;
if ( sink.getType() == NodeTypeEnums.AccumulateRightAdapterNode ) {
AccumulateNode accnode = ((AccumulateRight)sink).getBetaNode();
AccumulateMemory accMem = ( AccumulateMemory )
reteEvaluator.getNodeMemory( accnode );
bm = accMem.getBetaMemory();
- } else {
- BetaNode betaNode = ((RightInputAdapterNode) sink).getBetaNode();
+ betaNode = accnode;
+ } else {
+ betaNode = ((RightInputAdapterNode) sink).getBetaNode();
bm = RightInputAdapterNode.getBetaMemoryFromRightInput(betaNode,
reteEvaluator);
}
- if (sink.getType() == NodeTypeEnums.NotNode) {
- bm.linkNode( ( BetaNode ) sink, reteEvaluator );
+ if (betaNode.getType() == NodeTypeEnums.NotNode) {
+ bm.linkNode(betaNode, reteEvaluator);
+ // Stage left tuples that have no match records (contextObject ==
null) as INSERT.
+ // insertLeft in PhreakSubnetworkNotExistsNode only creates a
child when contextObject
+ // is null, so this is a no-op for tuples that were already
correctly handled by the
+ // normal deleteRight path (which leaves an empty-but-non-null
TupleList as contextObject).
+ // This rescues cross-package broken-path cases where deleteRight
never ran for a rule.
+ SegmentMemory smem = bm.getSegmentMemory();
+ if (smem != null) {
+ TupleMemory ltm = bm.getLeftTupleMemory();
+ if (ltm != null && ltm.size() > 0) {
+ FastIterator<TupleImpl> it = ltm.fullFastIterator();
+ for (TupleImpl lt = BetaNode.getFirstTuple(ltm, it); lt !=
null; lt = it.next(lt)) {
+ if (lt.getStagedType() == Tuple.NONE &&
lt.getContextObject() == null) {
+ smem.getStagedLeftTuples().addInsert(lt);
+ }
Review Comment:
These tuples come directly from `bm.getLeftTupleMemory()`, so staging them
as inserts causes `PhreakSubnetworkNotExistsNode.insertLeft()` to call
`ltm.add(leftTuple)` on an entry that is already linked in that memory. For an
unindexed `TupleList`, re-adding the tail links it to itself and increments the
size; this can corrupt iteration, and it can also create a second child for a
normal no-match tuple whose `contextObject` is null. Re-evaluate existing
tuples without re-inserting them into left memory, and distinguish tuples that
already have a child.
--
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]
