vamsizzz commented on issue #7125: URL: https://github.com/apache/incubator-kie/issues/7125#issuecomment-5788669328
> Hi [@vamsizzz](https://github.com/vamsizzz), thank you for reporting. > > In general I would suggest you to use a newer Apache KIE version, 8.44 is very old and no longer supported. Can you please doublecheck if the same reproduces with latest 10.2 version? Thanks for pointing us at 10.2 — testing there turned out to be the thing that showed us we were holding it wrong. **Short version: this is our bug, not yours.** Our KieBases are `sequential="true"`, and our generated rules were calling `update($ctx)` on a fact that other patterns bind via `from $ctx.…`. That's philosophically incoherent: sequential mode's whole contract is *insert once, fire in salience order, never re-evaluate*, and `update()` asks for exactly the re-evaluation that contract rules out. Because sequential mode builds `FromNode`s with `tupleMemoryEnabled=false`, the left-tuple memory is never allocated — and then the update/delete propagation paths dereference it. We were writing a forward-chaining idiom into an engine configuration that had opted out of forward chaining, and we'd been doing it 7,811 times across 5,225 DRLs without realizing it. ### What 10.x changed for us 8.44 tolerated the mistake almost completely; 10.2 does not. All numbers below are from the same ruleset and the same request, with each artifact compiled by the same KIE version that runs it: | Scenario (`sequential="true"`) | 8.44.0.Final | 10.2.0 | |---|---:|---:| | Cold `KieContainer`, no incremental updates | pass | **fail, every run** | | Chain of 4 in-place `updateToVersion()` upgrades | fails only at the last step | **fails from the 2nd step onward** | | Same chain, `update($ctx)` removed (insert-only) | pass | **pass, 0 failures** | | Cold container, `sequential=false`, `update()` kept | pass | **pass** | Two failure signatures showed up on 10.2 depending on which path was hit first: ``` NPE: DoubleLinkedEntry.getPrevious() is null at org.drools.core.util.LinkedList.remove(LinkedList.java:178) at org.drools.core.util.index.TupleList.remove(TupleList.java:51) at org.drools.core.phreak.PhreakFromNode.doLeftDeletes(PhreakFromNode.java:207) ``` ``` NPE: "previousMatches" is null at org.drools.core.phreak.PhreakFromNode.doLeftUpdates(PhreakFromNode.java:163) ``` The second one is the same NPE 8.44 produced (at `:154` there). So 10.2 didn't introduce a new defect so much as stop hiding an existing misuse — on 8.44 it took accumulated incremental-update history to surface; on 10.2 a first-load container trips it immediately. Removing `update($ctx)` entirely fixes it on **both** versions, and we're keeping `sequential="true"`. ### Feature request: make this misuse detectable The reason this survived years in production is that **nothing anywhere told us.** It compiles clean on 8.44 and 10.2 — zero errors, zero warnings — and then fails at runtime as an NPE deep inside internal linked-list structures, which points nowhere near the actual mistake. Three things would each have saved us the entire investigation: 1. **Build-time validation (most valuable).** When a `KieBase` is `sequential`, have `KieBuilder` emit a warning — or an opt-in error — for any rule whose RHS calls `update()`/`modify()` on a fact bound through `from`. This is statically detectable at compile time, and it's unambiguously a bug in every case we can construct. Even a plain warning would have caught it on day one. 2. **A guard with a real message.** Where the code currently dereferences the un-allocated tuple memory, a null check that throws something like `"update()/modify() is not supported for from-bound facts in a sequential KieBase — rule X"` would turn a 200-frame internal NPE into a self-explaining error. 3. **A documentation note.** The sequential-mode docs describe the performance characteristics but don't state that RHS fact mutation is incompatible with it. `update($ctx)` is idiomatic Drools in every tutorial; it isn't obvious that it's invalid specifically under `sequential="true"`. Happy to open a separate issue for any of these, or to test a patch — we have the full reproduction environment and can bisect or instrument on request. -- 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]
