eatmoreduck commented on issue #7247:
URL: https://github.com/apache/incubator-kie/issues/7247#issuecomment-6679859407
Confirmed the race by reading the current code — and it's a bit wider than
the guard: the whole lazy-init chain is unsynchronised check-then-act.
`getOrCreateSegmentMemory` (`SegmentMemorySupportImpl.java:96`) checks
`memory.getSegmentMemory() != null` and creates, then
`initializeChildSegmentsIfNeeded` checks `isEmpty()` and populates. The guard's
own comment ("this can happen when multiple threads are trying to initialize
the segment") shows the double-entry was expected — the check just isn't the
barrier.
Concretely, two threads that both pass `isEmpty()` each run the full
population loop, so every child segment is `add`ed twice into the segment
chain, and `sm.getFirst()` back in `LeftInputAdapterNode.doInsertObject` may
point at a child built by the losing thread. With `sinkPropagator.size() > 1`
there is also a real window between the first and second `add` where one thread
iterates a half-populated chain.
On the fix shape — since `ThreadSafeOption.NO` exists precisely to keep
`insert()` off the `NamedEntryPoint` lock, I'd avoid anything that puts a lock
back on the hot path once segments are initialised. Three directions I can see:
1. **One-shot CAS + volatile publication.** Keep the `isEmpty()` fast path;
the first thread to `compareAndSet` builds the child list locally and publishes
it; losers discard their build and re-read. Cheap after initialisation (one
volatile read), but the discard path needs care: `createChildSegmentLazily`
attaches `Memory` objects via `nodeMemories`, so a losing thread has created
state that is partly shared, and knowing what is safe to release needs a call
on ownership semantics.
2. **Fold child initialisation into `getOrCreateSegmentMemory`'s creation
step**, so there is a single check-then-act window per segment instead of two.
Doesn't remove the race by itself, but shrinks it and makes option 1 simpler
(one CAS guards the whole build).
3. **Declare it out of scope for `ThreadSafeOption.NO`** and document that
sessions without the entry-point lock require external synchronisation at least
until first initialisation. If that's the intended contract, the fix is
documentation plus maybe a debug-mode assertion rather than code.
Happy to implement whichever direction you prefer — if it's 1 or 2, I'd
start by writing the failing concurrent test (two threads inserting into a
fresh session with a split LIA under `ThreadSafeOption.NO`) so we can measure
which builds actually interleave on the window.
--
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]
