tkobayas commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6359936104
Thanks for the update. Re-reviewed by AI. Thanks!
---
- **Fixed:** the multi-anchor veto loop, the binary compatibility of
`DSL.not()` (now untouched, with `absent()` added instead), the nested XNOR
initialization, the nested XOR reversion (consistent with the new veto
semantics), and the AND + XNOR recovery.
- **Full reset works as described:** after a veto, `oneOf` / `noneOrAll` /
later-step state is cleared, and a fresh attempt behaves like a new sequence.
One new blocker and a few questions remain.
### 1. [Blocker] A veto raised during `activate()` is not handled
If a `oneOf` has two or more children that are already satisfied at
activation time (e.g. two `noneOrAll`, which match with zero inputs), the XOR
over-matches while the step is still being activated, and `VetoSignalProcessor`
runs from inside `LogicCircuitStep.activate()`.
`ev(name)` below is `pattern(declarationOf(Event.class)).expr("is-" + name,
e -> e.getName().equals(name))`.
```java
// step 0: veto -> re-activate step 0 -> veto -> ... => StackOverflowError
sequence(oneOf(noneOrAll(ev("a"), ev("b")), noneOrAll(ev("c"), ev("e"))),
ev("d"))
// later step: inserting s, d, s, d => ArrayIndexOutOfBoundsException at
Sequence.java:176
sequence(ev("s"), oneOf(noneOrAll(ev("a"), ev("b")), noneOrAll(ev("c"),
ev("e"))), ev("d"))
```
```text
StackOverflowError
at LogicGate.activate(LogicGate.java:172)
at LogicCircuitStep.activate(LogicCircuitStep.java:41)
at VetoSignalProcessor.consume(VetoSignalProcessor.java:89)
at LogicGate.consume(LogicGate.java:118)
at
LogicGateOutputSignalProcessor.consume(LogicGateOutputSignalProcessor.java:35)
at LogicGate.propagate(LogicGate.java:148)
at LogicGate.activate(LogicGate.java:172)
...
```
In the later-step case, after the veto resets to step 0, the `activate()`
loop of the vetoed step keeps going and registers adapters for a step that is
no longer current. That corrupts the sequence state.
Such a `oneOf` can never succeed, so rejecting it at build time is probably
the simplest fix. Alternatively, defer a veto raised during activation until
the step's activation has finished.
### 2. [Pre-existing, probably a separate issue]
`SequenceNode.AlphaAdapter.assertObject` can skip filters
The loop in `SequenceNode.AlphaAdapter.assertObject` caches `next` before
delivering. If delivering to one filter causes an anchor to leave its step, the
anchor's now-empty filters are removed from the active list. When one of them
is the cached `next`, `LinkedList.remove()` nulls its pointers and the loop
ends early. Filters further down the list never see the fact.
```java
sequence(ev("s"),
absent(ev("x")),
pattern(declarationOf(Event.class)).expr("d-own", anchor, (e, a) ->
e.getName().equals("d") && a.equals(e.getOwner())),
ev("x"))
// anchor1, anchor2; insert s (both -> step 1); insert d owned by anchor2
(anchor2 -> step 2);
// insert x: anchor1 is vetoed, and anchor2 never receives x => expected 1
firing, got 0
```
The same thing happens without any veto, with `or(ev("x"), d-own)` in place
of `absent(ev("x"))`. So this looks like an existing v1 bug, since
`SequenceNode` is not touched by this PR. The veto adds a new way to hit it,
though. Maybe fix it here, or file a separate issue?
### 3. Questions on semantics
- **`oneOf` under `or`:** with `and(or(oneOf(a, b), c), d)` and arrivals a,
c, b, d, the OR is already satisfied by `c`. Still, the `oneOf` over-match on
`b` vetoes the whole sequence, so there are 0 firings. Is that intended? If so,
it may be worth documenting, or rejecting `oneOf` under `or`.
- **Does the vetoing fact start a new attempt?** With `sequence(ev("s"),
absent(ev("s")), ev("d"))` and arrivals s, s, d, there are 0 firings: the
second `s` vetoes but doesn't restart the sequence. Right now that outcome
comes from the early exit in item 2 and the order of the filters, not from an
explicit rule. It would be good to define it.
### Nit
- A top-level bare `oneOf()` / `noneOrAll()` throws
`UnsupportedOperationException` in `KiePackagesBuilder`. The other validation
errors in this PR throw `IllegalArgumentException`.
--
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]
