[jboss-jira] [JBoss JIRA] Commented: (JBRULES-2465) Corruption of Rete when removing complex NotNodes

[Edson Tirelli (JIRA)]

    [ https://jira.jboss.org/jira/browse/JBRULES-2465?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12526615#action_12526615 ] 

Edson Tirelli commented on JBRULES-2465:
----------------------------------------

Hi Andreas

Thanks for reviewing this. Your questions:

1. Due to dependencies on other features, Drools never removes the Rete node (that is the network root), EntryPointNodes (because there might be external references to them and ObjectTypeNodes (because drools uses them to pre-sort facts in the ksession).

2. Yes, there was a related problem with node ID re-use. I fixed that as part of this fix.

3. That is just a practice. We don't use assertions in the core codebase. Maybe we should, but right now there are none, so I did not included your assertions.

4. Yes, I missed that sysout in there. Will remove. Thanks.

Edson

> Corruption of Rete when removing complex NotNodes
> -------------------------------------------------
>
>                 Key: JBRULES-2465
>                 URL: https://jira.jboss.org/jira/browse/JBRULES-2465
>             Project: Drools
>          Issue Type: Bug
>      Security Level: Public(Everyone can see) 
>          Components: drools-core
>    Affects Versions: 5.0.1.FINAL
>            Reporter: Andreas Kohn
>            Assignee: Edson Tirelli
>             Fix For: 5.1.0.M2
>
>         Attachments: drools-core-betanode-remove.diff, drools-core-betanode-remove.diff
>
>
> while working further on our drools integration we came across an odd
> exception when removing a particular rule:
> java.lang.IllegalArgumentException: Cannot remove a sink, when the list of sinks is null
>         at org.drools.reteoo.ObjectSource.removeObjectSink(ObjectSource.java:159)
>         at org.drools.reteoo.RightInputAdapterNode.doRemove(RightInputAdapterNode.java:217)
>         at org.drools.common.BaseNode.remove(BaseNode.java:95)
>         at org.drools.reteoo.BetaNode.doRemove(BetaNode.java:275)
>         at org.drools.common.BaseNode.remove(BaseNode.java:95)
>         at org.drools.reteoo.BetaNode.doRemove(BetaNode.java:280)
>         at org.drools.common.BaseNode.remove(BaseNode.java:95)
>         at org.drools.reteoo.RuleTerminalNode.doRemove(RuleTerminalNode.java:387)
>         at org.drools.common.BaseNode.remove(BaseNode.java:95)
>         at org.drools.reteoo.ReteooBuilder.removeRule(ReteooBuilder.java:237)
>         at org.drools.reteoo.ReteooRuleBase.removeRule(ReteooRuleBase.java:371)
>         at org.drools.common.AbstractRuleBase.removeRule(AbstractRuleBase.java:746)
> While stepping through the code it looked like the network was corrupt (there was indeed no 
> sinks on the ObjectSource, but the node calling removeObjectSink was still linked to it 
> and claiming it as source). 
> The rule itself contains multiple NotNodes, checking a condition that looks like this:
> not(not(Foo.v = X) and not(Foo.v = Y))
> I could track this down to some sort of "loop" in the rete that triggers this when the outer
> not node is removed.
> When removing BetaNode#doRemove() first walks along 'rightInput':
>         this.rightInput.remove( context,
>                                 builder,
>                                 this,
>                                 workingMemories );
> and eventually _in that call_ it also hits a node that is the direct 'leftInput' of the original beta node. 
> The removal marks that node as visited in the removal context, and when the 'rightInput.remove' returns to the
> beta node it does not visit the leftInput due to this condition in BetaNode#doRemove():
>         if ( !context.alreadyVisited( this.leftInput ) ) {
>             this.leftInput.remove( context,
>                                    builder,
>                                    this,
>                                    workingMemories );
>         }
> In other words: before the remove the BetaNode had another node that was both referenced directly as 'leftInput', 
> as well as an input to the 'rightInput'.
> The first removal of the rule "worked", and no exceptions happened. But: any further attempt to re-add the same rule and remove
> it again lead to the exception above.
> I was able to fix it with the attached patch, reproduced here:
> +        boolean needRemoveFromLeft = !context.alreadyVisited( this.leftInput );
>          this.rightInput.remove( context,
>                                  builder,
>                                  this,
>                                  workingMemories );
> -        if ( !context.alreadyVisited( this.leftInput ) ) {
> +        if ( needRemoveFromLeft ) {
>              this.leftInput.remove( context,
>                                     builder,
>                                     this,
>                                     workingMemories );
>          }
> With this patch applied I could add/delete/add the particular rule repeatedly without problems.
> The attached patch also adds an assert in ObjectSource#removeObjectSink(): when removing a sink from an object source with
> only one sink the sink was unconditionally replaced with an empty sink, although the argument ObjectSink could be a different
> sink than the one in the ObjectSource. For CompositeObjectSinkAdapters this case is checked, but not for single sinks.
> I originally suspected that place to be responsible for the problem I observed but the assertion never fired in my tests.
> (taken from rules-dev mail "Bug in BetaNode#doRemove()?"

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira