[I] Race condition between UnicastProcessor.onNext and onComplete in InMemoryBufferEventListener [polaris]

["visit2rahul (via GitHub)" <gi...@apache.org>]

visit2rahul opened a new issue, #4497:
URL: https://github.com/apache/polaris/issues/4497

   ## Summary
   
   `InMemoryBufferEventListener` has a race between `UnicastProcessor.onNext()` (called from `processEvent`) and `UnicastProcessor.onComplete()` (called from the Caffeine eviction listener and shutdown). At the eviction boundary, events can be silently dropped.
   
   ## Background
   
   Surfaced during review of #4487 by @nandorKollar, who noted that `UnicastProcessor.onNext()` is declared `public synchronized void` in smallrye-mutiny but `onComplete()` is not synchronized. @adutra agreed it warrants its own PR.
   
   ## The race
   
   When a per-realm processor is evicted after ~1 hour of inactivity:
   
   1. Caffeine's eviction listener fires `processor.onComplete()` on its cleanup thread
   2. Concurrently, a new event may arrive for the same realm
   3. `processEvent` retrieves the (about-to-be-evicted) processor reference and calls `processor.onNext(event)`
   
   `onNext` is method-synchronized so it acquires the processor's intrinsic monitor. `onComplete` is not synchronized and runs without acquiring that monitor. The two methods can therefore interleave; an event arriving at the wrong moment can be silently dropped (either by `onNext`'s `isDoneOrCancelled` check seeing `done = true` from a concurrent `onComplete`, or by a mid-drain termination).
   
   ## Practical impact
   
   Race window is small (microseconds at eviction boundary), only manifests after ~1 hour of inactivity per realm. But silent drops on an audit/event-log path are unacceptable.
   
   ## Proposed fix
   
   Wrap both `onComplete()` call sites (the Caffeine eviction listener and the `shutdown()` loop) in `synchronized (processor) { ... }` blocks so they acquire the same intrinsic monitor that `onNext()` uses. `processEvent` itself does not change.
   
   I will open a PR with this fix shortly.


-- 
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: issues-unsubscribe@polaris.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Race condition between UnicastProcessor.onNext and onComplete in InMemoryBufferEventListener [polaris]

["adutra (via GitHub)" <gi...@apache.org>]

adutra closed issue #4497: Race condition between UnicastProcessor.onNext and onComplete in InMemoryBufferEventListener
URL: https://github.com/apache/polaris/issues/4497


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Race condition between UnicastProcessor.onNext and onComplete in InMemoryBufferEventListener [polaris]

["visit2rahul (via GitHub)" <gi...@apache.org>]

visit2rahul commented on issue #4497:
URL: https://github.com/apache/polaris/issues/4497#issuecomment-4493030528

   Thank you @nandorKollar for finding this and @adutra for steering it to its own PR. The fix is up at #4498 - wraps both onComplete call sites (eviction listener + shutdown) in synchronized(processor) blocks so they acquire the same intrinsic monitor that onNext uses.
   
   Please review as your time permits.


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org